| Message ID | alpine.LSU.2.20.1704241315510.24645@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
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). > > Anyway, most controversical part(?) below. > > Any comments on this particular patch (and the overall proposal)? > > Cleaning up the options is probably a no-brainer anyways. > > Thanks, > Richard. > > 2017-04-24 Richard Biener <rguenther@suse.de> > > * common.opt (fstrict-overflow): Enable by default. > * opts.c (default_options_table): Remove OPT_fstrict_overflow entry. Presumably when this work is complete -fno-strict-overflow will give some kind of reasonable error message to the user directing them to the new option that most likely does what they were looking for? Jeff
On April 25, 2017 4:39:49 PM GMT+02:00, Jeff Law <law@redhat.com> 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). >> >> Anyway, most controversical part(?) below. >> >> Any comments on this particular patch (and the overall proposal)? >> >> Cleaning up the options is probably a no-brainer anyways. >> >> Thanks, >> Richard. >> >> 2017-04-24 Richard Biener <rguenther@suse.de> >> >> * common.opt (fstrict-overflow): Enable by default. >> * opts.c (default_options_table): Remove OPT_fstrict_overflow entry. >Presumably when this work is complete -fno-strict-overflow will give >some kind of reasonable error message to the user directing them to the > >new option that most likely does what they were looking for? No, it will simply map to -fwrapv. Richard. >Jeff
On 04/25/2017 09:09 AM, Richard Biener wrote: > On April 25, 2017 4:39:49 PM GMT+02:00, Jeff Law <law@redhat.com> 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). >>> >>> Anyway, most controversical part(?) below. >>> >>> Any comments on this particular patch (and the overall proposal)? >>> >>> Cleaning up the options is probably a no-brainer anyways. >>> >>> Thanks, >>> Richard. >>> >>> 2017-04-24 Richard Biener <rguenther@suse.de> >>> >>> * common.opt (fstrict-overflow): Enable by default. >>> * opts.c (default_options_table): Remove OPT_fstrict_overflow entry. >> Presumably when this work is complete -fno-strict-overflow will give >> some kind of reasonable error message to the user directing them to the >> >> new option that most likely does what they were looking for? > > No, it will simply map to -fwrapv. That's fine as well. As long as we do something sane ;-) jeff
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)? Martin > > Anyway, most controversical part(?) below. > > Any comments on this particular patch (and the overall proposal)? > > Cleaning up the options is probably a no-brainer anyways. > > Thanks, > Richard. > > 2017-04-24 Richard Biener <rguenther@suse.de> > > * common.opt (fstrict-overflow): Enable by default. > * opts.c (default_options_table): Remove OPT_fstrict_overflow entry. > > Index: gcc/common.opt > =================================================================== > --- gcc/common.opt (revision 247091) > +++ gcc/common.opt (working copy) > @@ -2342,7 +2342,7 @@ Common Report Var(flag_strict_aliasing) > Assume strict aliasing rules apply. > > fstrict-overflow > -Common Report Var(flag_strict_overflow) Optimization > +Common Report Var(flag_strict_overflow) Init(1) Optimization > Treat signed overflow as undefined. > > fsync-libcalls > Index: gcc/opts.c > =================================================================== > --- gcc/opts.c (revision 247091) > +++ gcc/opts.c (working copy) > @@ -496,7 +496,6 @@ static const struct default_options defa > { OPT_LEVELS_2_PLUS, OPT_fschedule_insns2, NULL, 1 }, > #endif > { OPT_LEVELS_2_PLUS, OPT_fstrict_aliasing, NULL, 1 }, > - { OPT_LEVELS_2_PLUS, OPT_fstrict_overflow, NULL, 1 }, > { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_freorder_blocks_algorithm_, NULL, > REORDER_BLOCKS_ALGORITHM_STC }, > { OPT_LEVELS_2_PLUS, OPT_freorder_functions, NULL, 1 }, >
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. We do have -fsanitize=undefined now to better catch all these cases. 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). Richard. > Martin > > > > > Anyway, most controversical part(?) below. > > > > Any comments on this particular patch (and the overall proposal)? > > > > Cleaning up the options is probably a no-brainer anyways. > > > > Thanks, > > Richard. > > > > 2017-04-24 Richard Biener <rguenther@suse.de> > > > > * common.opt (fstrict-overflow): Enable by default. > > * opts.c (default_options_table): Remove OPT_fstrict_overflow entry. > > > > Index: gcc/common.opt > > =================================================================== > > --- gcc/common.opt (revision 247091) > > +++ gcc/common.opt (working copy) > > @@ -2342,7 +2342,7 @@ Common Report Var(flag_strict_aliasing) > > Assume strict aliasing rules apply. > > > > fstrict-overflow > > -Common Report Var(flag_strict_overflow) Optimization > > +Common Report Var(flag_strict_overflow) Init(1) Optimization > > Treat signed overflow as undefined. > > > > fsync-libcalls > > Index: gcc/opts.c > > =================================================================== > > --- gcc/opts.c (revision 247091) > > +++ gcc/opts.c (working copy) > > @@ -496,7 +496,6 @@ static const struct default_options defa > > { OPT_LEVELS_2_PLUS, OPT_fschedule_insns2, NULL, 1 }, > > #endif > > { OPT_LEVELS_2_PLUS, OPT_fstrict_aliasing, NULL, 1 }, > > - { OPT_LEVELS_2_PLUS, OPT_fstrict_overflow, NULL, 1 }, > > { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_freorder_blocks_algorithm_, NULL, > > REORDER_BLOCKS_ALGORITHM_STC }, > > { OPT_LEVELS_2_PLUS, OPT_freorder_functions, NULL, 1 }, > > > >
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 (); } Martin
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.
On 04/27/2017 03:16 AM, Richard Biener wrote: > 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. After some more thought I can see an argument against warning on this case (if it ends up optimized). It's more subtle than the constant example above and I suppose it's also more likely that it might be the result of transformations that could obscure the true origin of the range in the source code. > > 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. I agree. Both simplifying it and making -Wall point out more signed overflow problems are worthwhile goals. Martin
Index: gcc/common.opt =================================================================== --- gcc/common.opt (revision 247091) +++ gcc/common.opt (working copy) @@ -2342,7 +2342,7 @@ Common Report Var(flag_strict_aliasing) Assume strict aliasing rules apply. fstrict-overflow -Common Report Var(flag_strict_overflow) Optimization +Common Report Var(flag_strict_overflow) Init(1) Optimization Treat signed overflow as undefined. fsync-libcalls Index: gcc/opts.c =================================================================== --- gcc/opts.c (revision 247091) +++ gcc/opts.c (working copy) @@ -496,7 +496,6 @@ static const struct default_options defa { OPT_LEVELS_2_PLUS, OPT_fschedule_insns2, NULL, 1 }, #endif { OPT_LEVELS_2_PLUS, OPT_fstrict_aliasing, NULL, 1 }, - { OPT_LEVELS_2_PLUS, OPT_fstrict_overflow, NULL, 1 }, { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_freorder_blocks_algorithm_, NULL, REORDER_BLOCKS_ALGORITHM_STC }, { OPT_LEVELS_2_PLUS, OPT_freorder_functions, NULL, 1 },
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). Anyway, most controversical part(?) below. Any comments on this particular patch (and the overall proposal)? Cleaning up the options is probably a no-brainer anyways. Thanks, Richard. 2017-04-24 Richard Biener <rguenther@suse.de> * common.opt (fstrict-overflow): Enable by default. * opts.c (default_options_table): Remove OPT_fstrict_overflow entry.