[PATCH] avoid issuing -Wrestrict from folder (PR 93519)
Martin Sebor
msebor@gmail.com
Thu Feb 6 23:08:00 GMT 2020
On 2/6/20 6:16 AM, Richard Biener wrote:
> On Thu, Feb 6, 2020 at 2:00 PM Jeff Law <law@redhat.com> wrote:
>>
>> On Wed, 2020-02-05 at 09:19 +0100, Richard Biener wrote:
>>> On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor <msebor@gmail.com> wrote:
>>>> On 2/4/20 2:31 PM, Jeff Law wrote:
>>>>> On Tue, 2020-02-04 at 13:08 -0700, Martin Sebor wrote:
>>>>>> On 2/4/20 12:15 PM, Richard Biener wrote:
>>>>>>> On February 4, 2020 5:30:42 PM GMT+01:00, Jeff Law <law@redhat.com> wrote:
>>>>>>>> On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
>>>>>>>>> On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor <msebor@gmail.com> wrote:
>>>>>>>>>> PR 93519 reports a false positive -Wrestrict issued for an inlined
>>>>>>>> call
>>>>>>>>>> to strcpy that carefully guards against self-copying. This is
>>>>>>>> caused
>>>>>>>>>> by the caller's arguments substituted into the call during inlining
>>>>>>>> and
>>>>>>>>>> before dead code elimination.
>>>>>>>>>>
>>>>>>>>>> The attached patch avoids this by removing -Wrestrict from the
>>>>>>>> folder
>>>>>>>>>> and deferring folding perfectly overlapping (and so undefined)
>>>>>>>> calls
>>>>>>>>>> to strcpy (and mempcpy, but not memcpy) until much later. Calls to
>>>>>>>>>> perfectly overlapping calls to memcpy are still folded early.
>>>>>>>>>
>>>>>>>>> Why do we bother to warn at all for this case? Just DWIM here.
>>>>>>>> Warnings like
>>>>>>>>> this can be emitted from the analyzer?
>>>>>>>> They potentially can, but the analyzer is and will almost always
>>>>>>>> certainly be considerably slower. I would not expect it to be used
>>>>>>>> nearly as much as the core compiler.
>>>>>>>>
>>>>>>>> WHether or not a particular warning makes sense in the core compiler or
>>>>>>>> analyzer would seem to me to depend on whether or not we can reasonably
>>>>>>>> issue warnings without interprocedural analysis. double-free
>>>>>>>> realistically requires interprocedural analysis to be effective. I'm
>>>>>>>> not sure Wrestrict really does.
>>>>>>>>
>>>>>>>>
>>>>>>>>> That is, I suggest to simply remove the bogus warning code from
>>>>>>>> folding
>>>>>>>>> (and _not_ fail the folding).
>>>>>>>> I haven't looked at the patch, but if we can get the warning out of the
>>>>>>>> folder that's certainly preferable. And we could investigate deferring
>>>>>>>> self-copy removal.
>>>>>>>
>>>>>>> I think the issue is as usual, warning for code we'll later remove as dead. Warning at folding is almost always premature.
>>>>>>
>>>>>> In this instance the code is reachable (or isn't obviously unreachable).
>>>>>> GCC doesn't remove it, but provides benign (and reasonable) semantics
>>>>>> for it(*). To me, that's one aspect of quality. Letting the user know
>>>>>> that the code is buggy is another. I view that as at least as important
>>>>>> as folding the ill-effects away because it makes it possible to fix
>>>>>> the problem so the code works correctly even with compilers that don't
>>>>>> provide these benign semantics.
>>>>> If you look at the guts of what happens at the point where we issue the
>>>>> warning from within gimple_fold_builtin_strcpy we have:
>>>>>
>>>>>> DCH_to_char (char * in, char * out, int collid)
>>>>>> {
>>>>>> int type;
>>>>>> char * D.2148;
>>>>>> char * dest;
>>>>>> char * num;
>>>>>> long unsigned int _4;
>>>>>> char * _5;
>>>>>>
>>>>>> ;; basic block 2, loop depth 0
>>>>>> ;; pred: ENTRY
>>>>>> ;; succ: 4
>>>>>>
>>>>>> ;; basic block 4, loop depth 0
>>>>>> ;; pred: 2
>>>>>> ;; succ: 5
>>>>>>
>>>>>> ;; basic block 5, loop depth 0
>>>>>> ;; pred: 4
>>>>>> ;; succ: 6
>>>>>>
>>>>>> ;; basic block 6, loop depth 0
>>>>>> ;; pred: 5
>>>>>> if (0 != 0)
>>>>>> goto <bb 7>; [53.47%]
>>>>>> else
>>>>>> goto <bb 8>; [46.53%]
>>>>>> ;; succ: 7
>>>>>> ;; 8
>>>>>>
>>>>>> ;; basic block 7, loop depth 0
>>>>>> ;; pred: 6
>>>>>> strcpy (out_1(D), out_1(D));
>>>>>> ;; succ: 8
>>>>>>
>>>>>> ;; basic block 8, loop depth 0
>>>>>> ;; pred: 6
>>>>>> ;; 7
>>>>>> _4 = __builtin_strlen (out_1(D));
>>>>>> _5 = out_1(D) + _4;
>>>>>> __builtin_memcpy (_5, "foo", 4);
>>>>>> ;; succ: 3
>>>>>>
>>>>>> ;; basic block 3, loop depth 0
>>>>>> ;; pred: 8
>>>>>> return;
>>>>>> ;; succ: EXIT
>>>>>>
>>>>>> }
>>>>>>
>>>>>
>>>>> Which shows the code is obviously unreachable in the case we're warning
>>>>> about. You can't see this in the dumps because it's exposed by
>>>>> inlining, then cleaned up before writing the dump file.
>>>>
>>>> In the specific case of the bug the code is of course eliminated
>>>> because it's guarded by the if (s != d). I was referring to
>>>> the general (unguarded) case of:
>>>>
>>>> char *s = "", *p;
>>>>
>>>> int main (void)
>>>> {
>>>> p = strcpy (s, s);
>>>> puts (p);
>>>> }
>>>>
>>>> where GCC folds the assignment 'p = strcpy(s, s);' to effectively
>>>> p = s; That's perfectly reasonable but it could equally as well
>>>> leave the call alone, as it does when s is null, for instance.
>>>>
>>>> I think folding it away is not only reasonable but preferable to
>>>> making the invalid call, but it's done only rarely. Most of
>>>> the time GCC does emit the undefined access (it does that with
>>>> calls to library functions as well as with direct stores and
>>>> reads). (I am hoping we can change that in the future so that
>>>> these kinds of problems are handled with some consistency.)
>>>>
>>>>> ISTM this would be a case we could handle with the __builtin_warning
>>>>> stuff.
>>>>>
>>>>> I think the question is do we want to do anything about it this cycle?
>>>>>
>>>>>
>>>>> If so, I think Martin's approach is quite reasonable. It disables
>>>>> folding away the self-copies from gimple-fold and moves the warning
>>>>> into the expander. So if there's such a call in the IL at expansion
>>>>> time we get a warning (-O0).
>>>>>
>>>>> I'd hazard a guess that the diagnostic was added to the strlen pass to
>>>>> capture the missed warning when we're optimizing and the self-copy has
>>>>> survived until that point. There's a couple issues that raises though.
>>>>>
>>>>> First, it's insufficient. DSE (for example) can do self-copy removal,
>>>>> so it needs the same handling. There may be others places too.
>>>>>
>>>>> Second, if the code becomes unreachable after strlen, then we've got
>>>>> new false positive issues.
>>>>>
>>>>> It's the classic problems we have with all middle end based warnings.
>>>>>
>>>>> But I could live with those if we can show that using __builtin_warning
>>>>> to handle this stuff in gcc-11 works... ISTM we emit the
>>>>> __builtin_warning call before any self-copy like that, whenever we
>>>>> happen to spot them. They'll naturally get removed if the path becomes
>>>>> unreachable. We'd warn during expansion for calls to
>>>>> __builtin_warning. We could even optionally warn when removing a call
>>>>> to __bulitin_warning.
>>>>>
>>>>> Thoughts?
>>>>
>>>> The patch has pretty much the same effect as emitting __builtin_warning
>>>> from the folder would. It defers the folding until much later, and if
>>>> the code isn't eliminated, it issues a warning and folds the call away.
>>>
>>> But it affects subsequent optimizations - the call is more expensive
>>> in any size heuristics, it posses an (alias-set zero) memory write
>>> barrier (unless you start to optimize no-op copies in the alias oracle),
>>> it is a _call_ - passes like the vectorizer are not happy about a call.
>>> It prevents SRA of the accessed object, ...
>> Yes, it does affect subsequent optimizations, but realistically how
>> often do we have self-assignments? And how often to we have them via
>> the str* functions.
>>
>> Additionally these are undefined except for memmove. I find it hard to
>> believe these are appearing it performance sensitive code.
>>
>> So I'd claim it's equivalent from a practical standpoint.
>
> But if the situation is so rare why does emitting the diagnostic matter then...
Because by their very nature bugs are rare, and often lurk in rare,
convoluted, tricky, or just plain stupid code. Buggy code may be
worth transforming into harmless code as a defensive strategy but
it's not worth optimizing. In my mind, it is always worth pointing
out and fixing, even if it's (in all likelihood) benign like exactly
overlapping copies.
To get some data I instrumented GCC to report instances of exactly
overlapping calls to strcpy and memcpy/mempcpy and built a bunch
of code with it. It found none in any of it, including GCC itself,
Glibc, Binutils/GDB, the Linux kernel, and a handful of others.
Does that mean it never happens and so isn't worth detecting? I'd
say not, because -Wrestrict has uncovered 16 instances of overlapping
sprintf calls in the kernel (where the destination is also an argument
to the first %s directive).
>
> Also it's not only optimization but eventually further diagnostic that only
> gets uncovered by optimizing such self-assignments.
I can't think of any examples where folding exactly overlapping copies
might expose other problems down the line but I'm all for folding them
away earlier rather than later (it's benign) as long as they're also
diagnosed. Replacing them with __builtin_warning might be one way to
do that (though no one looked at the patch when I posted it).
>
>> I guess I could do a Fedora build with a diagnostic to tell us how
>> often this happens. Would you be willing to reconsider if the data
>> shows this just doesn't happen often enough to matter?
>
> From my side its more on principle.
What principle is that? (Sincerely.)
There are many examples where GCC intentionally emits invalid code
even though folding it away (or replacing it with something like
a trap or unreachable) would be far a better, safer, or friendlier
choice. Calling library functions with invalid arguments (e.g.,
null pointers) is a prime but not the only example. It's pretty
much a rule that they don't get touched. I think it's a decision
worth revisiting throughout the compiler, not just in the folder,
while providing users with a choice of how to consistently respond
to such cases, e.g., via a command line option.
Martin
>
> But see my two proposed patches to address the actual testcase
> (second one doing a PRE walk for fold_marked_stmts is the one I prefer).
>
> Richard.
>
>> Jeff
>>>
>>
More information about the Gcc-patches
mailing list