Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
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 jos...@codesourcery.com
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Thu, 2020-02-06 at 14:16 +0100, Richard Biener wrote:
> On Thu, Feb 6, 2020 at 2:00 PM Jeff Law wrote:
> > On Wed, 2020-02-05 at 09:19 +0100, Richard Biener wrote:
> > > On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> > > > > > > wrote:
> > > > > > > > On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > > > > > > > > On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor
> > > > > > > > > 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 ; [53.47%]
> > > > > >else
> > > > > > goto ; [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
> > > > > > ;;
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Fri, Feb 7, 2020 at 12:08 AM Martin Sebor wrote:
>
> On 2/6/20 6:16 AM, Richard Biener wrote:
> > On Thu, Feb 6, 2020 at 2:00 PM Jeff Law wrote:
> >>
> >> On Wed, 2020-02-05 at 09:19 +0100, Richard Biener wrote:
> >>> On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> >>> wrote:
> On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor
> > 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 ; [53.47%]
> >> else
> >> goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On 2/6/20 6:16 AM, Richard Biener wrote:
On Thu, Feb 6, 2020 at 2:00 PM Jeff Law wrote:
On Wed, 2020-02-05 at 09:19 +0100, Richard Biener wrote:
On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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 wrote:
On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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 ; [53.47%]
else
goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Thu, Feb 6, 2020 at 2:00 PM Jeff Law wrote:
>
> On Wed, 2020-02-05 at 09:19 +0100, Richard Biener wrote:
> > On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> > > > > > wrote:
> > > > > > > On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > > > > > > > On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor
> > > > > > > > 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 ; [53.47%]
> > > > >else
> > > > > goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Wed, 2020-02-05 at 09:19 +0100, Richard Biener wrote:
> On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> > > > > wrote:
> > > > > > On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > > > > > > On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor
> > > > > > > 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 ; [53.47%]
> > > >else
> > > > goto ; [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:
> >
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Thu, Feb 6, 2020 at 12:14 PM Richard Biener
wrote:
>
> On Thu, Feb 6, 2020 at 11:33 AM Richard Biener
> wrote:
> >
> > On Thu, Feb 6, 2020 at 11:06 AM Richard Biener
> > wrote:
> > >
> > > On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor wrote:
> > > >
> > > > On 2/5/20 1:19 AM, Richard Biener wrote:
> > > > > On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> > > > > wrote:
> > > > >> On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > > > >>> On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor
> > > > >>> 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 ; [53.47%]
> > > > else
> > > > goto ; [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
> > > >
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Thu, Feb 6, 2020 at 11:33 AM Richard Biener
wrote:
>
> On Thu, Feb 6, 2020 at 11:06 AM Richard Biener
> wrote:
> >
> > On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor wrote:
> > >
> > > On 2/5/20 1:19 AM, Richard Biener wrote:
> > > > On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> > > > wrote:
> > > >> On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > > >>> On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor
> > > >>> 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 ; [53.47%]
> > > else
> > > goto ; [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
> > >
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Thu, Feb 6, 2020 at 11:06 AM Richard Biener
wrote:
>
> On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor wrote:
> >
> > On 2/5/20 1:19 AM, Richard Biener wrote:
> > > On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> > > wrote:
> > >> On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > >>> On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor
> > >>> 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 ; [53.47%]
> > else
> > goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Wed, Feb 5, 2020 at 4:55 PM Martin Sebor wrote:
>
> On 2/5/20 1:19 AM, Richard Biener wrote:
> > On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> > wrote:
> >> On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> >>> On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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 ; [53.47%]
> else
> goto ; [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
> >>
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On 2/5/20 1:19 AM, Richard Biener wrote:
On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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 wrote:
On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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 ; [53.47%]
else
goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Tue, Feb 4, 2020 at 11:02 PM Martin Sebor 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
> >>> wrote:
> On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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 ; [53.47%]
> >>else
> >> goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
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 wrote:
On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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 ; [53.47%]
else
goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
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 wrote:
> > > On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote:
> > > > On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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 ; [53.47%]
> else
> goto ; [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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On 2/4/20 12:15 PM, Richard Biener wrote: On February 4, 2020 5:30:42 PM GMT+01:00, Jeff Law wrote: On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote: On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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.
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On February 4, 2020 5:30:42 PM GMT+01:00, Jeff Law wrote: >On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote: >> On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Tue, 2020-02-04 at 10:34 +0100, Richard Biener wrote: > On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On 2/4/20 2:34 AM, Richard Biener wrote: On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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
Re: [PATCH] avoid issuing -Wrestrict from folder (PR 93519)
On Tue, Feb 4, 2020 at 1:44 AM Martin Sebor 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
