avoid issuing -Wrestrict from folder (PR 93519)

Message ID 6d253aa7-7f51-895b-b5db-d32ddb79cefc@gmail.com
State New
Headers show
Series
  • avoid issuing -Wrestrict from folder (PR 93519)
Related show

Commit Message

Martin Sebor Feb. 4, 2020, 12:44 a.m.
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.

Tested on x86_64-linux.

Martin

Comments

Richard Biener Feb. 4, 2020, 9:34 a.m. | #1
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?

That is, I suggest to simply remove the bogus warning code from folding
(and _not_ fail the folding).

Richard.

> Tested on x86_64-linux.

>

> Martin
Martin Sebor Feb. 4, 2020, 3:45 p.m. | #2
On 2/4/20 2:34 AM, 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?

> 

> That is, I suggest to simply remove the bogus warning code from folding

> (and _not_ fail the folding).


The overlapping copy is ultimately folded into a no-op but the warning
points out that code that relies on it is undefined.  The code should
be fixed.  This is in line with one of the strategies we discussed and
(at least those of us in the room) agreed on for undefined behavior
back in Manchester: try to do the least harm but warn.

Martin
Jeff Law Feb. 4, 2020, 4:30 p.m. | #3
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.

Jeff
Richard Biener Feb. 4, 2020, 7:15 p.m. | #4
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. 

Richard. 

>Jeff
Martin Sebor Feb. 4, 2020, 8:08 p.m. | #5
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.

Martin

[*] This is in contrast to the usual (though arguably inferior) strategy
GCC employs when dealing with undefined calls to built-in functions:
call the library function even when it's certain the call will crash.
Jeff Law Feb. 4, 2020, 9:31 p.m. | #6
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.

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?

Jeff
Martin Sebor Feb. 4, 2020, 10:02 p.m. | #7
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.


Martin
Richard Biener Feb. 5, 2020, 8:19 a.m. | #8
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, ...

So no, leaving in the call is _not_ equivalent to sticking in a
__builtin_warning() call (or however we actually implement it,
I'd prefer a stmt in the "debug" category so it's simply ignored
or elided by most passes by means of existing code).

That said, I'd prefer to not do anything about this bug.  Iff then
in the inliner try doing a CFG cleanup before folding stmts
(it's doing delayed folding anyway).  But not for GCC 10.
One could also mark stmts with no-warning before the inliner
folds them (and then mark back) to avoid those classes of
folding warnings.

Richard.

>

> Martin
Martin Sebor Feb. 5, 2020, 3:55 p.m. | #9
On 2/5/20 1:19 AM, 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, ...


This is a strcpy call copying over itself.  It's undefined code,
and so hardly anything that's common or so performance sensitive
to make a noticeable difference.

> So no, leaving in the call is _not_ equivalent to sticking in a

> __builtin_warning() call (or however we actually implement it,

> I'd prefer a stmt in the "debug" category so it's simply ignored

> or elided by most passes by means of existing code).

> 

> That said, I'd prefer to not do anything about this bug.  Iff then

> in the inliner try doing a CFG cleanup before folding stmts

> (it's doing delayed folding anyway).  But not for GCC 10.

> One could also mark stmts with no-warning before the inliner

> folds them (and then mark back) to avoid those classes of

> folding warnings.


I think this would very unfortunate for GCC 10.  The user's code
is clearly correct -- they take pains to avoid the overlapping
copy by guarding against it just before it -- and GCC simply emits
an invalid warning because of how it does inlining.  All that will
be accomplished by not fixing it is we will release a worse quality
compiler than we otherwise can, unnecessarily eroding our users'
confidence in the value of GCC's diagnostic.

I'll look into your suggestions for the inliner in stage 1 but
please reconsider for GCC 10.

Martin
Jeff Law Feb. 5, 2020, 6:39 p.m. | #10
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.

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?

Jeff
>
Richard Biener Feb. 6, 2020, 10:06 a.m. | #11
On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor <msebor@gmail.com> wrote:
>

> On 2/5/20 1:19 AM, 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, ...

>

> This is a strcpy call copying over itself.  It's undefined code,

> and so hardly anything that's common or so performance sensitive

> to make a noticeable difference.

>

> > So no, leaving in the call is _not_ equivalent to sticking in a

> > __builtin_warning() call (or however we actually implement it,

> > I'd prefer a stmt in the "debug" category so it's simply ignored

> > or elided by most passes by means of existing code).

> >

> > That said, I'd prefer to not do anything about this bug.  Iff then

> > in the inliner try doing a CFG cleanup before folding stmts

> > (it's doing delayed folding anyway).  But not for GCC 10.

> > One could also mark stmts with no-warning before the inliner

> > folds them (and then mark back) to avoid those classes of

> > folding warnings.

>

> I think this would very unfortunate for GCC 10.  The user's code

> is clearly correct -- they take pains to avoid the overlapping

> copy by guarding against it just before it -- and GCC simply emits

> an invalid warning because of how it does inlining.  All that will

> be accomplished by not fixing it is we will release a worse quality

> compiler than we otherwise can, unnecessarily eroding our users'

> confidence in the value of GCC's diagnostic.

>

> I'll look into your suggestions for the inliner in stage 1 but

> please reconsider for GCC 10.


One possibility is the attached but that adds an extra CFG cleanup
(also untested but on the testcase).  Another possibility would be
to re-do fold_marked_statements in terms of cleanup_control_flow_pre (),
recognizing unreachable regions on the way and simply avoid folding
stmts in blocks that will be then removed by the pending CFG cleanup.

I'll see whether I can cook that up quickly.

> Martin
Richard Biener Feb. 6, 2020, 10:33 a.m. | #12
On Thu, Feb 6, 2020 at 11:06 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor <msebor@gmail.com> wrote:

> >

> > On 2/5/20 1:19 AM, 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, ...

> >

> > This is a strcpy call copying over itself.  It's undefined code,

> > and so hardly anything that's common or so performance sensitive

> > to make a noticeable difference.

> >

> > > So no, leaving in the call is _not_ equivalent to sticking in a

> > > __builtin_warning() call (or however we actually implement it,

> > > I'd prefer a stmt in the "debug" category so it's simply ignored

> > > or elided by most passes by means of existing code).

> > >

> > > That said, I'd prefer to not do anything about this bug.  Iff then

> > > in the inliner try doing a CFG cleanup before folding stmts

> > > (it's doing delayed folding anyway).  But not for GCC 10.

> > > One could also mark stmts with no-warning before the inliner

> > > folds them (and then mark back) to avoid those classes of

> > > folding warnings.

> >

> > I think this would very unfortunate for GCC 10.  The user's code

> > is clearly correct -- they take pains to avoid the overlapping

> > copy by guarding against it just before it -- and GCC simply emits

> > an invalid warning because of how it does inlining.  All that will

> > be accomplished by not fixing it is we will release a worse quality

> > compiler than we otherwise can, unnecessarily eroding our users'

> > confidence in the value of GCC's diagnostic.

> >

> > I'll look into your suggestions for the inliner in stage 1 but

> > please reconsider for GCC 10.

>

> One possibility is the attached but that adds an extra CFG cleanup

> (also untested but on the testcase).  Another possibility would be

> to re-do fold_marked_statements in terms of cleanup_control_flow_pre (),

> recognizing unreachable regions on the way and simply avoid folding

> stmts in blocks that will be then removed by the pending CFG cleanup.

>

> I'll see whether I can cook that up quickly.


Like this.  Indenting not fixed and untested apart from on the testcase.
Disadvantage is we're walking _all_ blocks (at least skipping stmts).
Technically the iteration scheme should probably change to push
edges rather than edge iterators so we can avoid the repeated find_taken_edge
call.

Anyway, just a prototype.

Note SSA propagators / VN avoid to fold stmts that are obviously
unreachable so it somewhat makes sense to beat the inliner to do
the same.

Richard.

> > Martin
Richard Biener Feb. 6, 2020, 11:14 a.m. | #13
On Thu, Feb 6, 2020 at 11:33 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Thu, Feb 6, 2020 at 11:06 AM Richard Biener

> <richard.guenther@gmail.com> wrote:

> >

> > On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor <msebor@gmail.com> wrote:

> > >

> > > On 2/5/20 1:19 AM, 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, ...

> > >

> > > This is a strcpy call copying over itself.  It's undefined code,

> > > and so hardly anything that's common or so performance sensitive

> > > to make a noticeable difference.

> > >

> > > > So no, leaving in the call is _not_ equivalent to sticking in a

> > > > __builtin_warning() call (or however we actually implement it,

> > > > I'd prefer a stmt in the "debug" category so it's simply ignored

> > > > or elided by most passes by means of existing code).

> > > >

> > > > That said, I'd prefer to not do anything about this bug.  Iff then

> > > > in the inliner try doing a CFG cleanup before folding stmts

> > > > (it's doing delayed folding anyway).  But not for GCC 10.

> > > > One could also mark stmts with no-warning before the inliner

> > > > folds them (and then mark back) to avoid those classes of

> > > > folding warnings.

> > >

> > > I think this would very unfortunate for GCC 10.  The user's code

> > > is clearly correct -- they take pains to avoid the overlapping

> > > copy by guarding against it just before it -- and GCC simply emits

> > > an invalid warning because of how it does inlining.  All that will

> > > be accomplished by not fixing it is we will release a worse quality

> > > compiler than we otherwise can, unnecessarily eroding our users'

> > > confidence in the value of GCC's diagnostic.

> > >

> > > I'll look into your suggestions for the inliner in stage 1 but

> > > please reconsider for GCC 10.

> >

> > One possibility is the attached but that adds an extra CFG cleanup

> > (also untested but on the testcase).  Another possibility would be

> > to re-do fold_marked_statements in terms of cleanup_control_flow_pre (),

> > recognizing unreachable regions on the way and simply avoid folding

> > stmts in blocks that will be then removed by the pending CFG cleanup.

> >

> > I'll see whether I can cook that up quickly.

>

> Like this.  Indenting not fixed and untested apart from on the testcase.

> Disadvantage is we're walking _all_ blocks (at least skipping stmts).

> Technically the iteration scheme should probably change to push

> edges rather than edge iterators so we can avoid the repeated find_taken_edge

> call.

>

> Anyway, just a prototype.

>

> Note SSA propagators / VN avoid to fold stmts that are obviously

> unreachable so it somewhat makes sense to beat the inliner to do

> the same.


I'm testing this variant but I'm quite sure the cgraph verifier will trip on
us not updating cgraph edges for "unreachable" but changed (for example
indirect -> direct call) calls.  Somewhat moot of course but it appears
we want to keep it happy at all cost ...(?)  We even do "special"

  delete_unreachable_blocks_update_callgraph (id.dst_node, false);

  if (flag_checking)
    id.dst_node->verify ();

rather than relying on CFG cleanup we'll do afterwards anyway.
Maybe that's for a reason?  Honza?  Might be for the early inliner?
I see (see first posted variant) that regular delete_basic_block
doesn't bother to remove cgraph edges.

Anyhow, will report back if the simple PRE walk skipping unreachable
regions "works" in practice  (bootstrap/regtest).

Richard.

> Richard.

>

> > > Martin
Richard Biener Feb. 6, 2020, 11:57 a.m. | #14
On Thu, Feb 6, 2020 at 12:14 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>

> On Thu, Feb 6, 2020 at 11:33 AM Richard Biener

> <richard.guenther@gmail.com> wrote:

> >

> > On Thu, Feb 6, 2020 at 11:06 AM Richard Biener

> > <richard.guenther@gmail.com> wrote:

> > >

> > > On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor <msebor@gmail.com> wrote:

> > > >

> > > > On 2/5/20 1:19 AM, 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, ...

> > > >

> > > > This is a strcpy call copying over itself.  It's undefined code,

> > > > and so hardly anything that's common or so performance sensitive

> > > > to make a noticeable difference.

> > > >

> > > > > So no, leaving in the call is _not_ equivalent to sticking in a

> > > > > __builtin_warning() call (or however we actually implement it,

> > > > > I'd prefer a stmt in the "debug" category so it's simply ignored

> > > > > or elided by most passes by means of existing code).

> > > > >

> > > > > That said, I'd prefer to not do anything about this bug.  Iff then

> > > > > in the inliner try doing a CFG cleanup before folding stmts

> > > > > (it's doing delayed folding anyway).  But not for GCC 10.

> > > > > One could also mark stmts with no-warning before the inliner

> > > > > folds them (and then mark back) to avoid those classes of

> > > > > folding warnings.

> > > >

> > > > I think this would very unfortunate for GCC 10.  The user's code

> > > > is clearly correct -- they take pains to avoid the overlapping

> > > > copy by guarding against it just before it -- and GCC simply emits

> > > > an invalid warning because of how it does inlining.  All that will

> > > > be accomplished by not fixing it is we will release a worse quality

> > > > compiler than we otherwise can, unnecessarily eroding our users'

> > > > confidence in the value of GCC's diagnostic.

> > > >

> > > > I'll look into your suggestions for the inliner in stage 1 but

> > > > please reconsider for GCC 10.

> > >

> > > One possibility is the attached but that adds an extra CFG cleanup

> > > (also untested but on the testcase).  Another possibility would be

> > > to re-do fold_marked_statements in terms of cleanup_control_flow_pre (),

> > > recognizing unreachable regions on the way and simply avoid folding

> > > stmts in blocks that will be then removed by the pending CFG cleanup.

> > >

> > > I'll see whether I can cook that up quickly.

> >

> > Like this.  Indenting not fixed and untested apart from on the testcase.

> > Disadvantage is we're walking _all_ blocks (at least skipping stmts).

> > Technically the iteration scheme should probably change to push

> > edges rather than edge iterators so we can avoid the repeated find_taken_edge

> > call.

> >

> > Anyway, just a prototype.

> >

> > Note SSA propagators / VN avoid to fold stmts that are obviously

> > unreachable so it somewhat makes sense to beat the inliner to do

> > the same.

>

> I'm testing this variant but I'm quite sure the cgraph verifier will trip on

> us not updating cgraph edges for "unreachable" but changed (for example

> indirect -> direct call) calls.  Somewhat moot of course but it appears

> we want to keep it happy at all cost ...(?)  We even do "special"

>

>   delete_unreachable_blocks_update_callgraph (id.dst_node, false);

>

>   if (flag_checking)

>     id.dst_node->verify ();

>

> rather than relying on CFG cleanup we'll do afterwards anyway.

> Maybe that's for a reason?  Honza?  Might be for the early inliner?

> I see (see first posted variant) that regular delete_basic_block

> doesn't bother to remove cgraph edges.


It seems to work fine on

void foo() {}

static inline void bar (int i, int j, void (*p)())
{
  if (i != j)
    p();
}

void baz(int i)
{
  bar (i, i, foo);
}

where I see early inlining inlining bar, changing the call to foo()
(and not noticing it will be unreachable - for simpler if (0) cases
the inliner replaces the call to foo with a call to __builtin_unreachable!),
not folding it and passing verification.  Guess the cgraph edge updating
code in copy_bb deals with this so we should be safe not folding
stmts.

That said, testing still running.

Richard.

> Anyhow, will report back if the simple PRE walk skipping unreachable

> regions "works" in practice  (bootstrap/regtest).

>

> Richard.

>

> > Richard.

> >

> > > > Martin
Richard Biener Feb. 6, 2020, 1:16 p.m. | #15
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...

Also it's not only optimization but eventually further diagnostic that only
gets uncovered by optimizing such self-assignments.

> 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.

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

> >

>
Martin Sebor Feb. 6, 2020, 11:08 p.m. | #16
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

>>>

>>
Richard Biener Feb. 7, 2020, 9:09 a.m. | #17
On Fri, Feb 7, 2020 at 12:08 AM Martin Sebor <msebor@gmail.com> wrote:
>

> 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.)


The principle that GCC is an optimizing compiler not a static analyzer.

I'm actually fine with diagnosing things (and if the implementation
constraints require that even from folders), but not doing or delaying
optimizations just because of diagnostics starts to cross the line
to a static analysis tool.

> 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.


To me it's a QOI question that depends on the actual case.
Turning memcpy (p, p, N) into a no-op is the correct thing,
even though with (too) large N it might trap.  Folding
a read from outside of an object to zero might be OK
(it's undefined), but the choice of zero is arbitrary and it's
clearly not what the writer of the code intended.  Folding
it do a NaT and do further optimizations based on that
would be even worse.  Currently we leave the undefined
code in place but IMHO another sensible choice would
be to replace it with an explicit trap or unreachable
(and the choice between those could be dependent on
a command-line option).

Richard.

> 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

> >>>

> >>

>
Jeff Law Feb. 7, 2020, 4:30 p.m. | #18
On Thu, 2020-02-06 at 14:16 +0100, 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...

Vulnerabilities are often on uncommon paths.

In the case of a self-assignment I'm not terribly worried because it's
a nop and the potential for exploitation is minimal -- so eliminating
it at a reasonable point and potentially losing the diagnostic seems
reasonable.

A non-perfect overlap is much more of a concern and definitely worth a
diagnostic IMHO.

> 

> Also it's not only optimization but eventually further diagnostic that only

> gets uncovered by optimizing such self-assignments.

Absolutely.  As I' fond of saying there's a natural tension here. 
There are no easy answers and most of the time the choices involve
trading off one against another.

> 

> > 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.

> 

> 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).

They crossed in the ether.  Outgoing messages on my laptop had been
queued up due to a network issue.

jeff
Joseph Myers Feb. 7, 2020, 10:50 p.m. | #19
On Fri, 7 Feb 2020, Richard Biener wrote:

> To me it's a QOI question that depends on the actual case.

> Turning memcpy (p, p, N) into a no-op is the correct thing,

> even though with (too) large N it might trap.  Folding

> a read from outside of an object to zero might be OK

> (it's undefined), but the choice of zero is arbitrary and it's

> clearly not what the writer of the code intended.  Folding

> it do a NaT and do further optimizations based on that

> would be even worse.  Currently we leave the undefined

> code in place but IMHO another sensible choice would

> be to replace it with an explicit trap or unreachable

> (and the choice between those could be dependent on

> a command-line option).


Calling va_arg with a type changed by the default argument promotions 
(float, char, etc.) is another case where there would be a reasonably safe 
fallback (making gimplify_va_arg_expr adjust the type itself to the one it 
mentions in the warning message) as an alternative to generating a trap, 
possibly depending on command-line options to say how the user wishes to 
handle such cases.

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch

PR middle-end/93519 - bogus -Wrestrict for strcpy(d, s) call guarded by d != s

gcc/testsuite/ChangeLog:

	PR middle-end/93519
	* gcc.dg/Wrestrict-14.c: Remove an xfail.
	* gcc.dg/Wrestrict-21.c: New test.
	* gcc.dg/Wrestrict-22.c: New test.
	* gcc.dg/strlenopt-92.c: New test.
	* gcc.dg/strlenopt.h: Declare more functions.

gcc/ChangeLog:

	PR middle-end/93519
	* builtins.c (expand_builtin_mempcpy): Diagnose perfectly overlapping
	copies and return destination.
	(expand_builtin_strcpy): Same.  Defer declaring locals until their
	initial value is known for better readability.
	(expand_builtin_strncpy): Same.
	* gimple-fold.c (gimple_fold_builtin_memory_op): Avoid folding
	perfectly overlapping calls to mempcpy.
	(gimple_fold_builtin_strcpy): Avoid folding perfectly overlapping
	calls..
	* gimple-ssa-warn-restrict.c (maybe_diag_equal_operands): New function.
	(check_bounds_or_overlap): Call it.
	* gimple-ssa-warn-restrict.h (maybe_diag_equal_operands): Declare.
	* tree-ssa-strlen.c (maybe_handle_store_to_self): New function.
	(handle_builtin_strcpy): Call it.
	(handle_builtin_memcpy): Same.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index e4a8694054e..46cb3f8ae10 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -4253,6 +4253,22 @@  expand_builtin_mempcpy (tree exp, rtx target)
   if (!check_memop_access (exp, dest, src, len))
     return NULL_RTX;
 
+  if (operand_equal_p (dest, src, 0))
+    {
+      if (!TREE_NO_WARNING (exp))
+	{
+	  location_t loc = EXPR_LOCATION (exp);
+	  tree func = get_callee_fndecl (exp);
+	  warning_at (loc, OPT_Wrestrict,
+			  "%qD source argument is the same as destination",
+		      func);
+	}
+
+      /* Replace perfectly overlapping calls with DST + LEN.  */
+      tree res = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (dest), dest, len);
+      return expand_normal (res);
+    }
+
   return expand_builtin_mempcpy_args (dest, src, len,
 				      target, exp, /*retmode=*/ RETURN_END);
 }
@@ -4474,6 +4490,21 @@  expand_builtin_strcpy (tree exp, rtx target)
 		    src, destsize);
     }
 
+  if (operand_equal_p (dest, src, 0))
+    {
+      if (!TREE_NO_WARNING (exp))
+	{
+	  location_t loc = EXPR_LOCATION (exp);
+	  tree func = get_callee_fndecl (exp);
+	  warning_at (loc, OPT_Wrestrict,
+			  "%qD source argument is the same as destination",
+		      func);
+	}
+
+      /* Replace perfectly overlapping calls with the destination.  */
+      return expand_normal (dest);
+    }
+
   if (rtx ret = expand_builtin_strcpy_args (exp, dest, src, target))
     {
       /* Check to see if the argument was declared attribute nonstring
@@ -4810,6 +4841,22 @@  expand_builtin_strncpy (tree exp, rtx target)
     return NULL_RTX;
   tree dest = CALL_EXPR_ARG (exp, 0);
   tree src = CALL_EXPR_ARG (exp, 1);
+
+  if (operand_equal_p (dest, src, 0))
+    {
+      if (!TREE_NO_WARNING (exp))
+	{
+	  location_t loc = EXPR_LOCATION (exp);
+	  tree func = get_callee_fndecl (exp);
+	  warning_at (loc, OPT_Wrestrict,
+			  "%qD source argument is the same as destination",
+		      func);
+	}
+
+      /* Replace perfectly overlapping calls with the destination.  */
+      return expand_normal (dest);
+    }
+
   /* The number of bytes to write (not the maximum).  */
   tree len = CALL_EXPR_ARG (exp, 2);
 
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index ed225922269..50b1d627760 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -725,6 +725,12 @@  gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
      DEST{,+LEN,+LEN-1}.  */
   if (operand_equal_p (src, dest, 0))
     {
+      /* Fail for mempcpy (but not memcpy, and certainly not memmove)
+	 if SRC and DEST are the same.  Handle the overlap later.  */
+      if ((code == BUILT_IN_MEMPCPY || code == BUILT_IN_MEMPCPY_CHK)
+	  && operand_equal_p (src, dest, 0))
+	return false;
+
       /* Avoid diagnosing exact overlap in calls to __builtin_memcpy.
 	 It's safe and may even be emitted by GCC itself (see bug
 	 32667).  */
@@ -1747,37 +1753,17 @@  static bool
 gimple_fold_builtin_strcpy (gimple_stmt_iterator *gsi,
 			    tree dest, tree src)
 {
-  gimple *stmt = gsi_stmt (*gsi);
-  location_t loc = gimple_location (stmt);
-  tree fn;
-
-  /* If SRC and DEST are the same (and not volatile), return DEST.  */
+  /* Fail if SRC and DEST are the same.  Handle the overlap later.  */
   if (operand_equal_p (src, dest, 0))
-    {
-      /* Issue -Wrestrict unless the pointers are null (those do
-	 not point to objects and so do not indicate an overlap;
-	 such calls could be the result of sanitization and jump
-	 threading).  */
-      if (!integer_zerop (dest) && !gimple_no_warning_p (stmt))
-	{
-	  tree func = gimple_call_fndecl (stmt);
-
-	  warning_at (loc, OPT_Wrestrict,
-		      "%qD source argument is the same as destination",
-		      func);
-	}
-
-      replace_call_with_value (gsi, dest);
-      return true;
-    }
-
-  if (optimize_function_for_size_p (cfun))
     return false;
 
-  fn = builtin_decl_implicit (BUILT_IN_MEMCPY);
+  tree fn  = builtin_decl_implicit (BUILT_IN_MEMCPY);
   if (!fn)
     return false;
 
+  gimple *stmt = gsi_stmt (*gsi);
+  location_t loc = gimple_location (stmt);
+
   /* Set to non-null if ARG refers to an unterminated array.  */
   tree nonstr = NULL;
   tree len = get_maxval_strlen (src, SRK_STRLEN, &nonstr);
@@ -1794,6 +1780,9 @@  gimple_fold_builtin_strcpy (gimple_stmt_iterator *gsi,
   if (!len)
     return false;
 
+  if (optimize_function_for_size_p (cfun))
+    return false;
+
   len = fold_convert_loc (loc, size_type_node, len);
   len = size_binop_loc (loc, PLUS_EXPR, len, build_int_cst (size_type_node, 1));
   len = force_gimple_operand_gsi (gsi, len, true,
diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
index 2c582a670eb..46e9cc8e75e 100644
--- a/gcc/gimple-ssa-warn-restrict.c
+++ b/gcc/gimple-ssa-warn-restrict.c
@@ -2006,6 +2006,56 @@  wrestrict_dom_walker::check_call (gimple *call)
 
 } /* anonymous namespace */
 
+
+/* If DST and SRC point to the same location issues -Wrestrict.  */
+
+bool
+maybe_diag_equal_operands (gimple *stmt, tree dst, tree src)
+{
+  if (integer_zerop (dst) || gimple_no_warning_p (stmt))
+    return false;
+
+  location_t loc = gimple_location (stmt);
+  tree func = gimple_call_fndecl (stmt);
+  if (!warning_at (loc, OPT_Wrestrict,
+		   "%G%qD source argument is the same as destination",
+		   stmt, func))
+    return false;
+
+  gimple_set_no_warning (stmt, true);
+
+  /* The operands may be entirely different in the source code but
+     end up being equal as a result of inlining.  To make the warning
+     easier to understand reference the underlying decl they point to
+     in a note.  */
+  tree base = src;
+  if (TREE_CODE (base) == ADDR_EXPR)
+    base = TREE_OPERAND (base, 0);
+
+  poly_int64 poff;
+  base = get_addr_base_and_unit_offset (base, &poff);
+
+  /* Use the underlying SSA_NAME variable to point to the function
+     argument the operands were derived from.  */
+  if (TREE_CODE (base) == SSA_NAME)
+    {
+      base = SSA_NAME_VAR (base);
+      if (!base)
+	return true;
+    }
+  if (TREE_CODE (base) == PARM_DECL || DECL_P (base))
+    loc = DECL_SOURCE_LOCATION (base);
+  else if (EXPR_HAS_LOCATION (base))
+    loc = EXPR_LOCATION (base);
+  else
+    return true;
+
+  if (loc != UNKNOWN_LOCATION)
+    inform (loc, "accessing %qE declared here", base);
+
+  return true;
+}
+
 /* Attempt to detect and diagnose invalid offset bounds and (except for
    memmove) overlapping copy in a call expression EXPR from SRC to DST
    and DSTSIZE and SRCSIZE bytes, respectively.  Both DSTSIZE and
@@ -2072,14 +2122,8 @@  check_bounds_or_overlap (gimple *call, tree dst, tree src, tree dstsize,
 	 not point to objects and so do not indicate an overlap;
 	 such calls could be the result of sanitization and jump
 	 threading).  */
-      if (!integer_zerop (dst) && !gimple_no_warning_p (call))
-	{
-	  warning_at (loc, OPT_Wrestrict,
-		      "%G%qD source argument is the same as destination",
-		      call, func);
-	  gimple_set_no_warning (call, true);
-	  return OPT_Wrestrict;
-	}
+      if (maybe_diag_equal_operands (call, dst, src))
+	return OPT_Wrestrict;
 
       return 0;
     }
diff --git a/gcc/gimple-ssa-warn-restrict.h b/gcc/gimple-ssa-warn-restrict.h
index 7bae95a9ad1..91d9da9f126 100644
--- a/gcc/gimple-ssa-warn-restrict.h
+++ b/gcc/gimple-ssa-warn-restrict.h
@@ -23,4 +23,6 @@ 
 extern int check_bounds_or_overlap (gimple *, tree, tree, tree, tree,
 				    bool = false, bool = true);
 
+extern bool maybe_diag_equal_operands (gimple *, tree, tree);
+
 #endif /* GIMPLE_SSA_WARN_RESTRICT_H */
diff --git a/gcc/testsuite/gcc.dg/Wrestrict-14.c b/gcc/testsuite/gcc.dg/Wrestrict-14.c
index b919fa644d3..58c502a8731 100644
--- a/gcc/testsuite/gcc.dg/Wrestrict-14.c
+++ b/gcc/testsuite/gcc.dg/Wrestrict-14.c
@@ -69,7 +69,7 @@  void test_mempcpy (char *p, struct S *q, size_t n)
   mempcpy (q->p, q->p, 1);        /* { dg-warning "\\\[-Wrestrict]" } */
   sink (q);
 
-  mempcpy (&q->a[0], q->a, n);    /* { dg-warning "\\\[-Wrestrict]" "bug ????" { xfail *-*-* } } */
+  mempcpy (&q->a[0], q->a, n);    /* { dg-warning "\\\[-Wrestrict]" } */
   sink (q);
 
   mempcpy (q, q->a, n);           /* { dg-warning "\\\[-Wrestrict]" "bug ????" { xfail *-*-* } } */
diff --git a/gcc/testsuite/gcc.dg/Wrestrict-21.c b/gcc/testsuite/gcc.dg/Wrestrict-21.c
new file mode 100644
index 00000000000..4af814ff430
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wrestrict-21.c
@@ -0,0 +1,185 @@ 
+/* PR middle-end/93519 -bogus -Wrestrict for strcpy(d, s) call guarded by d != s
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+static inline void memcpy_nowarn (void *d, const void *s)
+{
+  if (s != d)
+    __builtin_memcpy (d, s, 32);
+}
+
+static inline void memcpy_warn (void *d, const void *s)
+{
+  /* Not diagnosed because GCC itself emits such overlapping calls and,
+     for the most part, handles them gracefully.  (Calls made by programs
+     should still be diagnosed but they're indistinguishable from GCC's
+     own.)  */
+  __builtin_memcpy (d, s, 32);          // { dg-warning "\\\[-Wrestrict]" "" { xfail *-*-* } }
+}
+
+void test_memcpy (void *s)              // { dg-message "declared here" "" { xfail *-*-* } }
+{
+  memcpy_nowarn (s, s);
+
+  memcpy_warn (s, s);
+
+  extern char memcpy_buf[];             // { dg-message "declared here" "" { xfail *-*-* } }
+  void *d = memcpy_buf;
+  s = memcpy_buf;
+
+  memcpy_warn (d, s);
+}
+
+
+static inline void* mempcpy_nowarn (void *d, const void *s)
+{
+  if (s != d)
+    return __builtin_mempcpy (d, s, 32);
+  return 0;
+}
+
+static inline void* mempcpy_warn (void *d, const void *s)
+{
+  return __builtin_mempcpy (d, s, 32);  // { dg-warning "\\\[-Wrestrict]" }
+}
+
+void test_mempcpy (void *s)             // { dg-message "declared here" }
+{
+  mempcpy_nowarn (s, s);
+
+  mempcpy_warn (s, s);
+
+  extern char mempcpy_buf[];            // { dg-message "declared here" }
+  void *d = mempcpy_buf;
+  s = mempcpy_buf;
+
+  mempcpy_warn (d, s);
+}
+
+
+static inline void strcat_nowarn (char *d, const char *s)
+{
+  if (s != d)
+    __builtin_strcat (d, s);
+}
+
+static inline void strcat_warn (char *d, const char *s)
+{
+  __builtin_strcat (d, s);              // { dg-warning "\\\[-Wrestrict]" }
+}
+
+void test_strcat (char *s)              // { dg-message "declared here" }
+{
+  strcat_nowarn (s, s);
+
+  strcat_warn (s, s);
+
+  extern char strcat_buf[];            // { dg-message "declared here" }
+  char *d = strcat_buf;
+  s = strcat_buf;
+
+  strcat_warn (d, s);
+}
+
+
+static inline char* stpcpy_nowarn (char *d, const char *s)
+{
+  if (s != d)
+    return __builtin_stpcpy (d, s);
+  return 0;
+}
+
+static inline char* stpcpy_warn (char *d, const char *s)
+{
+  return __builtin_stpcpy (d, s);       // { dg-warning "\\\[-Wrestrict]" }
+}
+
+void test_stpcpy (char *s)              // { dg-message "declared here" }
+{
+  stpcpy_nowarn (s, s);
+
+  stpcpy_warn (s, s);
+
+  extern char stpcpy_buf[];            // { dg-message "declared here" }
+  char *d = stpcpy_buf;
+  s = stpcpy_buf;
+
+  stpcpy_warn (d, s);
+}
+
+
+static inline char* stpncpy_nowarn (char *d, const char *s)
+{
+  if (s != d)
+    return __builtin_stpncpy (d, s, 32);
+  return 0;
+}
+
+static inline char* stpncpy_warn (char *d, const char *s)
+{
+  return __builtin_stpncpy (d, s, 32);  // { dg-warning "\\\[-Wrestrict]" }
+}
+
+void test_stpncpy (char *s)             // { dg-message "declared here" }
+{
+  stpncpy_nowarn (s, s);
+
+  stpncpy_warn (s, s);
+
+  extern char stpncpy_buf[];            // { dg-message "declared here" }
+  char *d = stpncpy_buf;
+  s = stpncpy_buf;
+
+  stpncpy_warn (d, s);
+}
+
+
+static inline void strcpy_nowarn (char *d, const char *s)
+{
+  if (s != d)
+    __builtin_strcpy (d, s);            // { dg-bogus "\\\[-Wrestrict]" }
+}
+
+static inline void strcpy_warn (char *d, const char *s)
+{
+  __builtin_strcpy (d, s);              // { dg-warning "\\\[-Wrestrict]" }
+}
+
+void test_strcpy (char *s)              // { dg-message "declared here" }
+{
+  strcpy_nowarn (s, s);
+
+  strcpy_warn (s, s);
+
+  extern char strcpy_buf[];             // { dg-message "declared here" }
+  char *d = strcpy_buf;
+  s = strcpy_buf;
+
+  strcpy_warn (d, s);
+}
+
+
+static inline void strncpy_nowarn (char *d, const char *s)
+{
+  if (s != d)
+    __builtin_strncpy (d, s, 32);
+}
+
+static inline void strncpy_warn (char *d, const char *s)
+{
+  __builtin_strncpy (d, s, 32);         // { dg-warning "\\\[-Wrestrict]" }
+}
+
+void test_strncpy (char *s)             // { dg-message "declared here" }
+{
+  strncpy_nowarn (s, s);
+
+  strncpy_warn (s, s);
+
+  extern char strncpy_buf[];            // { dg-message "declared here" }
+  char *d = strncpy_buf;
+  s = strncpy_buf;
+
+  strncpy_nowarn (d, s);
+  strncpy_warn (d, s);
+}
diff --git a/gcc/testsuite/gcc.dg/Wrestrict-22.c b/gcc/testsuite/gcc.dg/Wrestrict-22.c
new file mode 100644
index 00000000000..cd2577a415b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wrestrict-22.c
@@ -0,0 +1,116 @@ 
+/* { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* memcpy (void*, const void*, size_t);
+extern void* mempcpy (void*, const void*, size_t);
+
+struct A { char a[3]; };
+struct B { struct A a; char b[2]; };
+struct C { struct B b; char c[2]; };
+
+
+void* memcpy_member_to_A (struct A *p)
+{
+  char *q = p->a;
+  memcpy (p, q, sizeof *p);             // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+void* memcpy_member_to_B (struct B *p)
+{
+  void *q = &p->a;
+  memcpy (p, q, sizeof *p);             // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+void memcpy_member_to_C (struct C *p)
+{
+  void *q = &p->b;
+  memcpy (p, q, sizeof *p);             // { dg-warning "\\\[-Wrestrict" }
+}
+
+
+void* memcpy_member_plus_to_A (struct A *p)
+{
+  char *q = p->a + 1;
+  memcpy (p, q, sizeof *p);             // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+void* memcpy_member_plus_to_B (struct B *p)
+{
+  char *q = (char*)&p->a + 1;
+  memcpy (p, q, sizeof *p);             // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+void* memcpy_member_plus_to_C (struct C *p)
+{
+  char *q = (char*)&p->b + 1;
+  memcpy (p, q, sizeof *p);             // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+
+void* memcpy_member_to_A_plus (struct A *p)
+{
+  char *q = (char*)p + 1;
+  memcpy (q, p->a, sizeof *p);          // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+
+void* memcpy_member_to_B_plus (struct B *p)
+{
+  char *q = (char*)p + 1;
+  memcpy (q, &p->a, sizeof *p);          // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+void* memcpy_member_to_C_plus (struct C *p)
+{
+  char *q = (char*)p + 1;
+  memcpy (p, &p->b, sizeof *p);          // { dg-warning "\\\[-Wrestrict" }
+  return q;
+}
+
+
+
+void* mempcpy_member_to_A (struct A *p)
+{
+  char *q = p->a;
+  return mempcpy (p, q, sizeof *p);     // { dg-warning "\\\[-Wrestrict" }
+}
+
+void* mempcpy_member_to_B (struct B *p)
+{
+  void *q = &p->a;
+  return mempcpy (p, q, sizeof *p);     // { dg-warning "\\\[-Wrestrict" }
+}
+
+void* mempcpy_member_to_C (struct C *p)
+{
+  void *q = &p->b;
+  return mempcpy (p, q, sizeof *p);     // { dg-warning "\\\[-Wrestrict" }
+}
+
+
+void* mempcpy_member_to_A_plus (struct A *p)
+{
+  char *q = (char*)p + 1;
+  return mempcpy (q, p->a, sizeof *p);  // { dg-warning "\\\[-Wrestrict" }
+}
+
+void* mempcpy_member_to_B_plus (struct B *p)
+{
+  char *q = (char*)p + 1;
+  return mempcpy (q, &p->a, sizeof *p); // { dg-warning "\\\[-Wrestrict" }
+}
+
+void* mempcpy_member_to_C_plus (struct C *p)
+{
+  char *q = (char*)p + 1;
+  return mempcpy (q, &p->b, sizeof *p); // { dg-warning "\\\[-Wrestrict" }
+}
diff --git a/gcc/testsuite/gcc.dg/strlenopt-92.c b/gcc/testsuite/gcc.dg/strlenopt-92.c
new file mode 100644
index 00000000000..97082388ce1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/strlenopt-92.c
@@ -0,0 +1,131 @@ 
+/* Verify that perfectly overlapping calls to raw memory and string
+   functions are eliminated (they are undefined but there's no point
+   in making the library calls).
+   { dg-do compile }
+   { dg-options "-O2 -Wall -fdump-tree-optimized" } */
+
+#define USE_GNU
+#include "strlenopt.h"
+
+void use (char*);
+
+extern char acpy[3];
+
+void test_mem_cpy_self_use (void)
+{
+  char *d = acpy, *s = acpy;
+  use (memcpy (d, s, sizeof acpy));
+}
+
+/* { dg-final { scan-tree-dump-times "use \\\(\\\&acpy" 1 "optimized" } }  */
+
+char* test_mem_cpy_self_ret (unsigned n)
+{
+  char *d = acpy, *s = acpy;
+  return memcpy (d, s, n);
+}
+
+/* { dg-final { scan-tree-dump-times "return \\\&acpy;" 1 "optimized" } }
+   { dg-final { scan-tree-dump-not "memcpy \\\(" "optimized" } }  */
+
+
+extern char apcpy[3];
+
+void test_mem_pcpy_self_use (void)
+{
+  char *d = apcpy, *s = apcpy;
+  use (mempcpy (d, s, sizeof apcpy));  // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "use \\\(\\\&apcpy" 1 "optimized" { xfail *-*-* } } }  */
+
+char* test_mem_pcpy_self_ret (unsigned n)
+{
+  char *d = apcpy, *s = apcpy;
+  return mempcpy (d, s, n);             // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "return \\\&apcpy;" 1 "optimized" { xfail *-*-* } } }
+   { dg-final { scan-tree-dump-not "mempcpy \\\(" "optimized" { xfail *-*-* } } }
+   { dg-final { scan-assembler-not "mempcpy" } } */
+
+
+extern char ascpy[3];
+
+void test_str_cpy_use (void)
+{
+  char *d = ascpy, *s = ascpy;
+  use (strcpy (d, s));                 // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "use \\\(\\\&ascpy" 1 "optimized" } } */
+
+char* test_str_cpy_ret (void)
+{
+  char *d = ascpy, *s = ascpy;
+  return strcpy (d, s);                 // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "return \\\&ascpy;" 1 "optimized" } }
+   { dg-final { scan-tree-dump-not "strcpy \\\(" "optimized" } } */
+
+
+extern char asncpy[3];
+
+void test_str_ncpy_use (void)
+{
+  char *d = asncpy, *s = asncpy;
+  use (strncpy (d, s, sizeof asncpy)); // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "use \\\(\\\&asncpy" 1 "optimized" { xfail *-*-* } } } */
+
+char* test_str_ncpy_ret (unsigned n)
+{
+  char *d = asncpy, *s = asncpy;
+  return strncpy (d, s, n);             // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "return \\\&asncpy;" 1 "optimized" { xfail *-*-* } } }
+   { dg-final { scan-tree-dump-not "strncpy \\\(" "optimized" { xfail *-*-* } } }
+   { dg-final { scan-assembler-not "strncpy" } } */
+
+
+extern char aspcpy[3];
+
+void test_stp_cpy_use (void)
+{
+  char *d = aspcpy, *s = aspcpy;
+  use (stpcpy (d, s));                 // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "use \\\(\\\&aspcpy" 1 "optimized" { xfail *-*-*} } } */
+
+char* test_stp_cpy_ret (void)
+{
+  char *d = aspcpy, *s = aspcpy;
+  return stpcpy (d, s);                 // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "return \\\&aspcpy;" 1 "optimized" { xfail *-*-* } } }
+   { dg-final { scan-tree-dump-not "stpcpy \\\(" "optimized" { xfail *-*-* } } } */
+
+
+extern char aspncpy[3];
+
+void test_stp_ncpy_use (void)
+{
+  char *d = aspcpy, *s = aspcpy;
+  use (stpncpy (d, s, 3));              // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "use \\\(\\\&aspncpy" 1 "optimized" { xfail *-*-*} } } */
+
+char* test_stp_ncpy_ret (unsigned n)
+{
+  char *d = aspcpy, *s = aspcpy;
+  return stpncpy (d, s, n);             // { dg-warning "\\\[-Wrestrict" }
+}
+
+/* { dg-final { scan-tree-dump-times "return \\\&aspncpy;" 1 "optimized" { xfail *-*-* } } }
+   { dg-final { scan-tree-dump-not "stpncpy \\\(" "optimized" { xfail *-*-* } } } */
diff --git a/gcc/testsuite/gcc.dg/strlenopt.h b/gcc/testsuite/gcc.dg/strlenopt.h
index 518d0cf08b2..ca13dc7a31a 100644
--- a/gcc/testsuite/gcc.dg/strlenopt.h
+++ b/gcc/testsuite/gcc.dg/strlenopt.h
@@ -13,6 +13,7 @@  size_t strnlen (const char *, size_t);
 void *memcpy (void *__restrict, const void *__restrict, size_t);
 void *memmove (void *, const void *, size_t);
 char *strcpy (char *__restrict, const char *__restrict);
+char *strncpy (char *__restrict, const char *__restrict, size_t);
 char *strcat (char *__restrict, const char *__restrict);
 char *strchr (const char *, int);
 int strcmp (const char *, const char *);
@@ -23,6 +24,7 @@  int strcmp (const char *, const char *);
 #ifdef USE_GNU
 void *mempcpy (void *__restrict, const void *__restrict, size_t);
 char *stpcpy (char *__restrict, const char *__restrict);
+char *stpncpy (char *__restrict, const char *__restrict, size_t);
 #endif
 
 int sprintf (char * __restrict, const char *__restrict, ...);
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index ad9e98973b1..7a94c9a2d90 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -2312,6 +2312,41 @@  maybe_warn_overflow (gimple *stmt, unsigned HOST_WIDE_INT len,
 		       si, plus_one, rawmem);
 }
 
+/* If DST is the same as SRC, diagnoses the overlapping call at GSI.
+   If SUB is not ERROR_MARK_NODE, replaces the call if its result is
+   used with SUB if nonnull or with DST otherwise.
+   Returns true if the call has been eliminated.  */
+
+static bool
+maybe_handle_store_to_self (gimple_stmt_iterator *gsi, tree dst, tree src,
+			    tree sub = NULL_TREE)
+{
+  if (!operand_equal_p (dst, src, 0))
+    return false;
+
+  gimple *stmt = gsi_stmt (*gsi);
+
+  maybe_diag_equal_operands (stmt, dst, src);
+
+  if (gimple_call_lhs (stmt))
+    {
+      if (sub == error_mark_node)
+	return false;
+
+      /* Replace the call with SUB if non-null or with DST otherwise.  */
+      replace_call_with_value (gsi, sub ? sub : dst);
+    }
+  else
+    {
+      /* Eliminate the call.  */
+      unlink_stmt_vdef (stmt);
+      gsi_remove (gsi, true);
+      release_defs (stmt);
+    }
+
+  return true;
+}
+
 /* Handle a strlen call.  If strlen of the argument is known, replace
    the strlen call with the known value, otherwise remember that strlen
    of the argument is stored in the lhs SSA_NAME.  */
@@ -2610,24 +2645,25 @@  static void
 handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
 		       const vr_values *rvals)
 {
-  int idx, didx;
-  tree src, dst, srclen, len, lhs, type, fn, oldlen;
-  bool success;
   gimple *stmt = gsi_stmt (*gsi);
-  strinfo *si, *dsi, *olddsi, *zsi;
-  location_t loc;
+  tree src = gimple_call_arg (stmt, 1);
+  tree dst = gimple_call_arg (stmt, 0);
 
-  src = gimple_call_arg (stmt, 1);
-  dst = gimple_call_arg (stmt, 0);
-  lhs = gimple_call_lhs (stmt);
-  idx = get_stridx (src);
-  si = NULL;
-  if (idx > 0)
-    si = get_strinfo (idx);
+  /* Diagnose exactly overlapping strcpy and either replace it with DST
+     or eliminate it.  Only diagnose overlapping stpcpy for now without
+     replacing its LHS (but eliminate it if the result is unused).
+     FIXME: Handle overlapping stpcpy result by replacing it with
+     DST + strlen (DST).  */
+  tree sub = (bcode == BUILT_IN_STRCPY || bcode == BUILT_IN_STRCPY_CHK
+	      ? NULL_TREE : error_mark_node);
+  if (maybe_handle_store_to_self (gsi, src, dst, sub))
+    return;
 
-  didx = get_stridx (dst);
-  olddsi = NULL;
-  oldlen = NULL_TREE;
+  int idx = get_stridx (src);
+  strinfo *si = idx > 0 ? get_strinfo (idx) : NULL;
+  int didx = get_stridx (dst);
+  strinfo *olddsi = NULL;
+  tree oldlen = NULL_TREE;
   if (didx > 0)
     olddsi = get_strinfo (didx);
   else if (didx < 0)
@@ -2636,7 +2672,7 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
   if (olddsi != NULL)
     adjust_last_stmt (olddsi, stmt, false);
 
-  srclen = NULL_TREE;
+  tree srclen = NULL_TREE;
   if (si != NULL)
     srclen = get_string_length (si);
   else if (idx < 0)
@@ -2647,7 +2683,8 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
   if (olddsi != NULL)
     adjust_last_stmt (olddsi, stmt, false);
 
-  loc = gimple_location (stmt);
+  tree lhs = gimple_call_lhs (stmt);
+  location_t loc = gimple_location (stmt);
   if (srclen == NULL_TREE)
     switch (bcode)
       {
@@ -2678,6 +2715,8 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
       if (didx == 0)
 	return;
     }
+
+  strinfo *dsi;
   if (olddsi != NULL)
     {
       oldlen = olddsi->nonzero_chars;
@@ -2769,8 +2808,8 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
   if (si != NULL)
     si->dont_invalidate = true;
 
-  fn = NULL_TREE;
-  zsi = NULL;
+  tree fn = NULL_TREE;
+  strinfo *zsi = NULL;
   switch (bcode)
     {
     case BUILT_IN_STRCPY:
@@ -2811,15 +2850,14 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
   if (zsi != NULL)
     zsi->dont_invalidate = true;
 
+  tree type = size_type_node;
   if (fn)
     {
       tree args = TYPE_ARG_TYPES (TREE_TYPE (fn));
       type = TREE_VALUE (TREE_CHAIN (TREE_CHAIN (args)));
     }
-  else
-    type = size_type_node;
 
-  len = fold_convert_loc (loc, type, unshare_expr (srclen));
+  tree len = fold_convert_loc (loc, type, unshare_expr (srclen));
   len = fold_build2_loc (loc, PLUS_EXPR, type, len, build_int_cst (type, 1));
 
   /* Set the no-warning bit on the transformed statement?  */
@@ -2843,6 +2881,8 @@  handle_builtin_strcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
       fprintf (dump_file, "Optimizing: ");
       print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
     }
+
+  bool success;
   if (gimple_call_num_args (stmt) == 2)
     success = update_gimple_call (gsi, fn, 3, dst, src, len);
   else
@@ -3421,6 +3461,17 @@  handle_builtin_memcpy (enum built_in_function bcode, gimple_stmt_iterator *gsi,
   tree src = gimple_call_arg (stmt, 1);
   tree dst = gimple_call_arg (stmt, 0);
 
+  /* Diagnose exactly overlapping memcpy and either replace it with DST
+     or eliminate it.  Only diagnose overlapping mempcpy for now without
+     replacing its LHS (but eliminate it if the result is unused).
+     FIXME: Handle overlapping mempcpy result by replacing it with
+     DST + LEN.
+     Also consider diagnosing overflow before overlapping copies.  */
+  tree sub = (bcode == BUILT_IN_MEMCPY || bcode == BUILT_IN_MEMCPY_CHK
+	      ? NULL_TREE : error_mark_node);
+  if (maybe_handle_store_to_self (gsi, src, dst, sub))
+    return;
+
   int didx = get_stridx (dst);
   strinfo *olddsi = NULL;
   if (didx > 0)