On 11/07/2018 02:28 PM, Jeff Law wrote:
On 10/20/18 6:01 PM, Martin Sebor wrote:
The warning only triggers when the bound is less than or equal
to the length of the constant source string (i.e, when strncpy
truncates). So IIUC, your suggestion would defer folding only
such strncpy calls and let gimple_fold_builtin_strncpy fold
those with a constant bound that's greater than the length of
the constant source string. That would be fine with me, but
since strncpy calls with a bound that's greater than the length
of the source are pointless I don't think they are important
enough to worry about folding super early. The constant ones
that serve any purpose (and that are presumably important to
optimize) are those that truncate.
I was focused exclusively on the case where we have to look for a
subsequent statement that handled termination. The idea was to only
leave in the cases that we might need to warn for because we couldn't
search subsequent statement for the termination.
Splitting up was primarily meant to get the warning out of the folder
with a minimal impact on code generation. But if the common case would
result in deferral of folding, then I'd fully expect Richi to object.
The test case from the bug is:
struct S {
char dest[5];
};
const char src[] = "1234567890";
void f (struct S *s)
{
strncpy (s->dest, src, sizeof (s->dest) - 1);
s->dest [sizeof (s->dest) - 1] = '\0';
}
The strncpy call truncates but it's safe because of the assignment.
This is representative of the use case that the fix is needed for
(one with a constant source string) and that needs folding to be
deferred. I don't think this use case is a pervasive one, or even
terribly common among all strncpy uses, but it is the only one that
the early folding code handles. By deferring the folding, this use
case will be transformed to memcpy slightly later than it is now
(during forwprop1 to be precise, so after early folding).
As a data point, in a build of the Linux kernel where I expect
strncpy is used as intended more than in most other code, of
the nearly 500 distinct strncpy calls, 21 instances are folded
before the CFG is complete. The effect of the patch would fold
the 21 instances later. I.e., just 4% of all calls. Most of
these calls (12 out of the 21) are in SCSI drivers, in code
that responds to the INQUIRY command with things like vendor
and product names, hardly something that matters for efficiency.
But at -O1 even they still are ultimately folded to memcpy.
That said, when optimization isn't enabled, I don't think users
expect calls to library functions to be transformed to calls to
other functions, or inlined. Yet that's just what GCC does.
For example, besides triggering the warning, the following:
I don't think we should drag this into the issue at hand. Though I do
generally agree that folding this stuff into low level memory operations
is not what most would expect at -O0.
Folding calls to library functions so early that the CFG hasn't
been constructed yet is the root cause of the issue. I'm not
suggesting to prevent it for all functions as part of this fix,
but if agree that this early folding is a poor choice in general
then we should not be uncomfortable with a patch that defers it
for just a subset of use cases of a single function.
FWIW, I would view it as entirely appropriate to do our due
diligence before making a decision about the early folding of
all library functions, but I'm finding it hard to justify to
myself spending this much time and effort on either the false
positive or on the question of whether something that's by all
measures as inconsequential for efficiency as strncpy should
be transformed to memcpy at point 1 or point 2.
What do you suggest next?
Martin
