On Wed, 26 Apr 2017, Martin Sebor wrote:
> On 04/26/2017 01:59 AM, Richard Biener wrote:
> > On Tue, 25 Apr 2017, Martin Sebor wrote:
> >
> > > On 04/24/2017 05:25 AM, Richard Biener wrote:
> > > >
> > > > The following makes signed overflow undefined for all (non-)optimization
> > > > levels. The intent is to remove -fno-strict-overflow signed overflow
> > > > behavior as that is not a sensible option to the user (it ends up
> > > > with the worst of both -fwrapv and -fno-wrapv). The implementation
> > > > details need to be preserved for the forseeable future to not wreck
> > > > UBSAN with either associating (-fwrapv behavior) or optimizing
> > > > (-fno-wrapv behavior).
> > > >
> > > > The other choice would be to make -fwrapv the default for -O[01].
> > > >
> > > > A second patch in this series would unify -f[no-]wrapv, -f[no-]trapv
> > > > and -f[no-]strict-overflow with a
> > > > -fsigned-integer-overflow={undefined,wrapping,trapping[,sanitized]}
> > > > option, making conflicts amongst the options explicit (and reduce
> > > > the number of flag_ variables). 'sanitized' would essentially map
> > > > to todays flag_strict_overflow = 0. There's another sole user
> > > > of flag_strict_overflow, POINTER_TYPE_OVERFLOW_UNDEFINED - not sure
> > > > what to do about that, apart from exposing it as different flag
> > > > alltogether.
> > > >
> > > > Further patches in the series would remove -Wstrict-overflow (and
> > > > cleanup VRP for example).
> > >
> > > Minimizing the differences between the guarantees provided at
> > > different optimization levels is a good thing. It will help
> > > uncover bugs that would go undetected during development (with
> > > -O0) and only manifest when building with optimization (which
> > > may be less frequent).
> > >
> > > I find the -Wstrict-overflow warning with all its levels over-
> > > engineered but I'm not sure I'm in favor of completely eliminating
> > > it. It has helped illuminate the signed integer overflow problem
> > > for many users who were otherwise completely unaware of it. I'd
> > > be concerned that by getting rid of it users might be lulled back
> > > into assuming that it has the same wrapping semantics as common
> > > hardware (or simply doesn't happen). It sounds like you'd like
> > > to get rid of it to simplify GCC code. Would it make sense to
> > > preserve it for at least the most egregious instances of overflow
> > > (like in 'if (i + 1 < i)' and similar)?
> >
> > Such cases can (and should!) be certainly warned for but in the
> > frontend. The current implementation which warns at the point
> > of simplification isn't too useful given it warns even when the
> > above doesn't appear literally but through complex optimization.
> >
> > The most complaint about the warning is that it warns about
> > perfectly valid code -- there's nothing invalid in a program doing
> >
> > if (i + 1 < i)
> >
> > and the compiler optimizing this is good.
> >
> > The warning was supposed to be a hint that maybe the programmer
> > wasn't aware of undefined signed integer overflow and thus the
> > code didn't do what he expected.
>
> Exactly. The example above clearly indicates an error on the part
> of its author: an assumption that signed overflow has wrapping
> semantics. It's a common mistake that people make based on their
> experience with popular hardware.
>
> I agree with optimizing such code, but it shouldn't be done
> silently, especially not on targets where it does have the
> expected semantics. Warning only in the front end will miss
> the more subtle instances of the problem.
>
> That said, I certainly share your concern about false positives
> for code that results from prior transformations. But I'm hopeful
> there is a solution to the problem other than limiting warnings
> to little more than pattern matchers in the front end.
>
> >
> > We do have -fsanitize=undefined now to better catch all these
> > cases.
>
> The sanitizers are very helpful as a complementary tool to
> warnings, but not as a substitute for them. Unlike warnings,
> they're not available to all projects, depend on every code
> path being exercised, have a relatively high runtime overhead,
> and tend to catch bugs late in the development cycle (after
> they have been injected into the code base).
>
> >
> > Btw, the above should warn under one of the various -W...
> > that warn about expressions always evaluating to true/false
> > (didn't spot one that's right on, so maybe we need to add a new one -
> > or re-use -Wstrict-overflow).
>
> That would make sense. -Wtautological-compare seems close.
> But to catch more than just the trivial cases it would need
> to be extended to the middle-end. For instance:
>
> void bar (int i, int j)
> {
> if (j < 1 || 2 < j)
> j = 1;
>
> if (i + j < i) // could be optimized (with a warning)
> foo ();
> }
This is a case I'd rather not warn about.
Btw, one issue with the current warning is its implementation -- while
"powerful" in the sense that it triggers from random places (and thus
has to be silenced everywhere) via machinery in the "optimization"
(aka folding) I dislike it very much. It looks like only a single
case was transitioned when moving foldings from fold-const.c to match.pd,
which also hints at very low testsuite coverage for the warning.
IMHO it's over-engineered which is also the reason for the different
warning levels and only warning for _very_ few cases by default at
-Wall.
Richard.
