| Message ID | CAESRpQAZT7syOqgusqXQEgigUj3UeTfi5iD9CHJKGCAnVfA7Ug@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 07/22/2015 09:29 AM, Manuel López-Ibáñez wrote: > While looking at PR c/16351, I noticed that all tests proposed for > -Wnull-attribute > (https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01715.html) could be > warned from the FEs by simply extending the existing Wnonnull. > > Bootstrapped and regression tested on x86_64-linux-gnu. > > OK? > > > gcc/ChangeLog: > > 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR c/16351 > * doc/invoke.texi (Wnonnull): Document behavior for > returns_nonnull. > > gcc/testsuite/ChangeLog: > > 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR c/16351 > * c-c++-common/wnonnull-1.c: New test. > > gcc/cp/ChangeLog: > > 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR c/16351 > * typeck.c (check_return_expr): Call maybe_warn_returns_nonnull. > > > gcc/c-family/ChangeLog: > > 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR c/16351 > * c-common.c (maybe_warn_returns_nonnull): New. > * c-common.h (maybe_warn_returns_nonnull): Declare. > > gcc/c/ChangeLog: > > 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> > > PR c/16351 > * c-typeck.c (c_finish_return): Call maybe_warn_returns_nonnull. FWIW, we have the usual tension here between warning in the front-end vs warning after optimization and exploiting dataflow analysis. Warning in the front-ends like this can generate false positives (such as a NULL return in an unreachable path and miss cases where the NULL has to be propagated into the return by later optimizations. However warning in the front-ends will tend to have more stable diagnostics from release to release. Warning after optimization/analysis will tend to generate fewer false positives and can pick up cases where the didn't explicitly appear in the return statement, but had to be propagated in by the optimizers. Of course, these warnings are less stable release-to-release and require the optimizers/analysis phases to be run. I've always preferred exploiting optimization and analysis to both reduce false positives and expose the non-trivial which show up via optimizations. But I also understand that's simply my preference and that others have a different preference. I'll tentatively approve for the trunk, but I think we still want warnings after optimization/analysis. Which will likely lead to a scheme like I proposed many years for uninitialized variables where we have multiple modes. One warns in the front-end like your implemention does, the other defers the warning until after analysis & optimization. So please keep 16351 open after committing. Jeff
On July 23, 2015 7:43:51 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: >On 07/22/2015 09:29 AM, Manuel López-Ibáñez wrote: >> While looking at PR c/16351, I noticed that all tests proposed for >> -Wnull-attribute >> (https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01715.html) could be >> warned from the FEs by simply extending the existing Wnonnull. >> >> Bootstrapped and regression tested on x86_64-linux-gnu. >> >> OK? >> >> >> gcc/ChangeLog: >> >> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >> >> PR c/16351 >> * doc/invoke.texi (Wnonnull): Document behavior for >> returns_nonnull. >> >> gcc/testsuite/ChangeLog: >> >> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >> >> PR c/16351 >> * c-c++-common/wnonnull-1.c: New test. >> >> gcc/cp/ChangeLog: >> >> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >> >> PR c/16351 >> * typeck.c (check_return_expr): Call maybe_warn_returns_nonnull. >> >> >> gcc/c-family/ChangeLog: >> >> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >> >> PR c/16351 >> * c-common.c (maybe_warn_returns_nonnull): New. >> * c-common.h (maybe_warn_returns_nonnull): Declare. >> >> gcc/c/ChangeLog: >> >> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >> >> PR c/16351 >> * c-typeck.c (c_finish_return): Call maybe_warn_returns_nonnull. >FWIW, we have the usual tension here between warning in the front-end >vs >warning after optimization and exploiting dataflow analysis. > >Warning in the front-ends like this can generate false positives (such >as a NULL return in an unreachable path and miss cases where the NULL >has to be propagated into the return by later optimizations. > >However warning in the front-ends will tend to have more stable >diagnostics from release to release. > >Warning after optimization/analysis will tend to generate fewer false >positives and can pick up cases where the didn't explicitly appear in >the return statement, but had to be propagated in by the optimizers. Of > >course, these warnings are less stable release-to-release and require >the optimizers/analysis phases to be run. > >I've always preferred exploiting optimization and analysis to both >reduce false positives and expose the non-trivial which show up via >optimizations. But I also understand that's simply my preference and >that others have a different preference. > >I'll tentatively approve for the trunk, but I think we still want >warnings after optimization/analysis. Which will likely lead to a >scheme like I proposed many years for uninitialized variables where we >have multiple modes. One warns in the front-end like your implemention > >does, the other defers the warning until after analysis & optimization. > >So please keep 16351 open after committing. -W{no-,}{f,m}e IIRC was proposed before. Won't help https://gcc.gnu.org/PR55035 though where struct stores just escape too much -- AFAIU -- but still.. Thanks,
On 07/23/2015 04:44 PM, Bernhard Reutner-Fischer wrote: > On July 23, 2015 7:43:51 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: >> On 07/22/2015 09:29 AM, Manuel López-Ibáñez wrote: >>> While looking at PR c/16351, I noticed that all tests proposed for >>> -Wnull-attribute >>> (https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01715.html) could be >>> warned from the FEs by simply extending the existing Wnonnull. >>> >>> Bootstrapped and regression tested on x86_64-linux-gnu. >>> >>> OK? >>> >>> >>> gcc/ChangeLog: >>> >>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>> >>> PR c/16351 >>> * doc/invoke.texi (Wnonnull): Document behavior for >>> returns_nonnull. >>> >>> gcc/testsuite/ChangeLog: >>> >>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>> >>> PR c/16351 >>> * c-c++-common/wnonnull-1.c: New test. >>> >>> gcc/cp/ChangeLog: >>> >>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>> >>> PR c/16351 >>> * typeck.c (check_return_expr): Call maybe_warn_returns_nonnull. >>> >>> >>> gcc/c-family/ChangeLog: >>> >>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>> >>> PR c/16351 >>> * c-common.c (maybe_warn_returns_nonnull): New. >>> * c-common.h (maybe_warn_returns_nonnull): Declare. >>> >>> gcc/c/ChangeLog: >>> >>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>> >>> PR c/16351 >>> * c-typeck.c (c_finish_return): Call maybe_warn_returns_nonnull. >> FWIW, we have the usual tension here between warning in the front-end >> vs >> warning after optimization and exploiting dataflow analysis. >> >> Warning in the front-ends like this can generate false positives (such >> as a NULL return in an unreachable path and miss cases where the NULL >> has to be propagated into the return by later optimizations. >> >> However warning in the front-ends will tend to have more stable >> diagnostics from release to release. >> >> Warning after optimization/analysis will tend to generate fewer false >> positives and can pick up cases where the didn't explicitly appear in >> the return statement, but had to be propagated in by the optimizers. Of >> >> course, these warnings are less stable release-to-release and require >> the optimizers/analysis phases to be run. >> >> I've always preferred exploiting optimization and analysis to both >> reduce false positives and expose the non-trivial which show up via >> optimizations. But I also understand that's simply my preference and >> that others have a different preference. >> >> I'll tentatively approve for the trunk, but I think we still want >> warnings after optimization/analysis. Which will likely lead to a >> scheme like I proposed many years for uninitialized variables where we >> have multiple modes. One warns in the front-end like your implemention >> >> does, the other defers the warning until after analysis & optimization. >> >> So please keep 16351 open after committing. > > -W{no-,}{f,m}e IIRC was proposed before. Won't help https://gcc.gnu.org/PR55035 though where struct stores just escape too much -- AFAIU -- but still.. > Things like uninitialized variable analysis are inherently going to cause false positives. It's just a fact of life. Looking at the reduced testcase in that PR, I'm pretty sure its a bogus reduction. We could have N == 0 when we encounter the head of the first loop. So no members of temp[] will be initialized. We then have the call to bar() which could change the value of N to 1. We then hit the second loop and read temp[0] which is uninitialized. The warning for the reduced testcase is correct. If the original source has the same overall structure (an unbound write between the key loops), then the warning is correct. If the writes can be proven to not clobber the loop bounds (such that the two key loops iterate over the same number of elements), then we'd have a false positive warning (and a failure of jump threading to discover the unexecutable path). Jeff
On July 24, 2015 7:30:03 AM GMT+02:00, Jeff Law <law@redhat.com> wrote: >On 07/23/2015 04:44 PM, Bernhard Reutner-Fischer wrote: >> On July 23, 2015 7:43:51 PM GMT+02:00, Jeff Law <law@redhat.com> >wrote: >>> On 07/22/2015 09:29 AM, Manuel López-Ibáñez wrote: >>>> While looking at PR c/16351, I noticed that all tests proposed for >>>> -Wnull-attribute >>>> (https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01715.html) could be >>>> warned from the FEs by simply extending the existing Wnonnull. >>>> >>>> Bootstrapped and regression tested on x86_64-linux-gnu. >>>> >>>> OK? >>>> >>>> >>>> gcc/ChangeLog: >>>> >>>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>>> >>>> PR c/16351 >>>> * doc/invoke.texi (Wnonnull): Document behavior for >>>> returns_nonnull. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>>> >>>> PR c/16351 >>>> * c-c++-common/wnonnull-1.c: New test. >>>> >>>> gcc/cp/ChangeLog: >>>> >>>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>>> >>>> PR c/16351 >>>> * typeck.c (check_return_expr): Call >maybe_warn_returns_nonnull. >>>> >>>> >>>> gcc/c-family/ChangeLog: >>>> >>>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>>> >>>> PR c/16351 >>>> * c-common.c (maybe_warn_returns_nonnull): New. >>>> * c-common.h (maybe_warn_returns_nonnull): Declare. >>>> >>>> gcc/c/ChangeLog: >>>> >>>> 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> >>>> >>>> PR c/16351 >>>> * c-typeck.c (c_finish_return): Call >maybe_warn_returns_nonnull. >>> FWIW, we have the usual tension here between warning in the >front-end >>> vs >>> warning after optimization and exploiting dataflow analysis. >>> >>> Warning in the front-ends like this can generate false positives >(such >>> as a NULL return in an unreachable path and miss cases where the >NULL >>> has to be propagated into the return by later optimizations. >>> >>> However warning in the front-ends will tend to have more stable >>> diagnostics from release to release. >>> >>> Warning after optimization/analysis will tend to generate fewer >false >>> positives and can pick up cases where the didn't explicitly appear >in >>> the return statement, but had to be propagated in by the optimizers. >Of >>> >>> course, these warnings are less stable release-to-release and >require >>> the optimizers/analysis phases to be run. >>> >>> I've always preferred exploiting optimization and analysis to both >>> reduce false positives and expose the non-trivial which show up via >>> optimizations. But I also understand that's simply my preference >and >>> that others have a different preference. >>> >>> I'll tentatively approve for the trunk, but I think we still want >>> warnings after optimization/analysis. Which will likely lead to a >>> scheme like I proposed many years for uninitialized variables where >we >>> have multiple modes. One warns in the front-end like your >implemention >>> >>> does, the other defers the warning until after analysis & >optimization. >>> >>> So please keep 16351 open after committing. >> >> -W{no-,}{f,m}e IIRC was proposed before. Won't help >https://gcc.gnu.org/PR55035 though where struct stores just escape too >much -- AFAIU -- but still.. >> >Things like uninitialized variable analysis are inherently going to >cause false positives. It's just a fact of life. > >Looking at the reduced testcase in that PR, I'm pretty sure its a bogus > >reduction. Yes, the original, smallish test case in comment #4 is different, AFAICS. > >We could have N == 0 when we encounter the head of the first loop. So >no members of temp[] will be initialized. We then have the call to >bar() which could change the value of N to 1. We then hit the second >loop and read temp[0] which is uninitialized. The warning for the >reduced testcase is correct. Agree. > >If the original source has the same overall structure (an unbound write > >between the key loops), then the warning is correct. If the writes can > >be proven to not clobber the loop bounds (such that the two key loops >iterate over the same number of elements), then we'd have a false >positive warning (and a failure of jump threading to discover the >unexecutable path). AFAIU the write in the testcase in comment #4 does not clobber loop bounds, so I think the warning is wrong. Thanks,
On 23 July 2015 at 19:43, Jeff Law <law@redhat.com> wrote: > Warning in the front-ends like this can generate false positives (such as a > NULL return in an unreachable path and miss cases where the NULL has to be > propagated into the return by later optimizations. False positives (for the warning as proposed right now) would be strange, since it would mean that a returns_nonnull function returns an explicit NULL in some code-path that is not meant to be executed. That sounds like a bug waiting to happen. Moreover, this warning works in exactly the same cases as __attribute__((nonnull)) does for function arguments, and so far those haven't been a source of false positives. > I've always preferred exploiting optimization and analysis to both reduce > false positives and expose the non-trivial which show up via optimizations. > But I also understand that's simply my preference and that others have a > different preference. I'm very much in favour of this, but only for things that cannot reliably be warned from the FE. Clang has shown that it is possible to improve much more the accuracy of warnings in the FE and still compile faster than GCC by performing some kind of fast CCP (and VRP?) in the FE (or make the CCP and VRP passes occur earlier and even without optimization): https://gcc.gnu.org/PR38470 is just one example that would be very much improved by some trivial VRP. > I'll tentatively approve for the trunk, but I think we still want warnings > after optimization/analysis. Which will likely lead to a scheme like I > proposed many years for uninitialized variables where we have multiple > modes. One warns in the front-end like your implemention does, the other > defers the warning until after analysis & optimization. Isn't this what we already have with -Wuninitialized and -Wmaybe-uninitialized? It would be OK to get warnings for returning NULL implicitly (either by propagation, inlining, VRP). I plan to leave 16351 open for that (or open a new PR). However, in this case, I also think it makes sense to follow what -Wnonnull already does and always warn for any explicit "return NULL", even if the code as currently written is never executed. The two things are not incompatible. > So please keep 16351 open after committing. Of course, there is also https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01860.html pending review. Cheers, Manuel.
On 07/24/2015 02:06 AM, Bernhard Reutner-Fischer wrote: >> >> Looking at the reduced testcase in that PR, I'm pretty sure its a bogus >> >> reduction. > > Yes, the original, smallish test case in comment #4 is different, AFAICS. Agreed. I should have been a bit more explicit. The test in c#5 is not a valid reduction. I glanced at the testcase in c#4 and I believe it is a valid reduction. I've reflected those findings as well as postulated on how the case from c#4 could be fixed in the BZ. Jeff
On 07/24/2015 07:45 AM, Manuel López-Ibáñez wrote: > On 23 July 2015 at 19:43, Jeff Law <law@redhat.com> wrote: >> Warning in the front-ends like this can generate false positives (such as a >> NULL return in an unreachable path and miss cases where the NULL has to be >> propagated into the return by later optimizations. > > False positives (for the warning as proposed right now) would be > strange, since it would mean that a returns_nonnull function returns > an explicit NULL in some code-path that is not meant to be executed. > That sounds like a bug waiting to happen. Depends on how you choose to look at things. It's quite common via macros & such to have unexecutable/unreachable paths. Whether or not to warn about something found on such a path is a matter of personal preference. > > Moreover, this warning works in exactly the same cases as > __attribute__((nonnull)) does for function arguments, and so far those > haven't been a source of false positives. I'm sure I could write one ;-) And given that a FE based version of this will only catch explicit NULLs in argument lists, I consider it so weak as to be virtually useless. In fact, when I wrote the patches around 16351 I certainly did find cases where NULL values were flowing into places where they shouldn't. Some were fairly complex and we fixed them -- there's no way the trivial front-end stuff would have found them. > >> I've always preferred exploiting optimization and analysis to both reduce >> false positives and expose the non-trivial which show up via optimizations. >> But I also understand that's simply my preference and that others have a >> different preference. > > I'm very much in favour of this, but only for things that cannot > reliably be warned from the FE. Clang has shown that it is possible to > improve much more the accuracy of warnings in the FE and still compile > faster than GCC by performing some kind of fast CCP (and VRP?) in the > FE (or make the CCP and VRP passes occur earlier and even without > optimization): And my assertion is that for things like we're discussing, you need the analysis & optimizations both to expose the non-trivial cases and prune away those which are false positives. I consider exposing the non-trivial cases and pruning away false positives the most important aspect of this kind of work. > > https://gcc.gnu.org/PR38470 is just one example that would be very > much improved by some trivial VRP. > >> I'll tentatively approve for the trunk, but I think we still want warnings >> after optimization/analysis. Which will likely lead to a scheme like I >> proposed many years for uninitialized variables where we have multiple >> modes. One warns in the front-end like your implemention does, the other >> defers the warning until after analysis & optimization. > > Isn't this what we already have with -Wuninitialized and -Wmaybe-uninitialized? I'm pretty sure this is different than what I've proposed (and even coded up) in the past. Essentially we run the uninitialized warning analysis twice, saving/comparing the results. One is run just after the into-ssa phase (from which you can get the release-to-release stable warnings), the other as we're done with the optimization pipeline (which allows more precision). You can even do things like "XYZ was used uninitialized, but all those uses were removed by the optimizers". > > It would be OK to get warnings for returning NULL implicitly (either > by propagation, inlining, VRP). I plan to leave 16351 open for that > (or open a new PR). However, in this case, I also think it makes sense > to follow what -Wnonnull already does and always warn for any explicit > "return NULL", even if the code as currently written is never > executed. The two things are not incompatible. > >> So please keep 16351 open after committing. > > Of course, there is also > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01860.html pending Yes, it's on my list. jeff
On 24 July 2015 at 21:30, Jeff Law <law@redhat.com> wrote: > On 07/24/2015 07:45 AM, Manuel López-Ibáñez wrote: >> >> On 23 July 2015 at 19:43, Jeff Law <law@redhat.com> wrote: >>> >>> Warning in the front-ends like this can generate false positives (such as >>> a >>> NULL return in an unreachable path and miss cases where the NULL has to >>> be >>> propagated into the return by later optimizations. >> >> >> False positives (for the warning as proposed right now) would be >> strange, since it would mean that a returns_nonnull function returns >> an explicit NULL in some code-path that is not meant to be executed. >> That sounds like a bug waiting to happen. > > Depends on how you choose to look at things. It's quite common via macros & > such to have unexecutable/unreachable paths. Whether or not to warn about > something found on such a path is a matter of personal preference. I think it is also a matter of the particular warning and on the balance of true positives vs. false positives in typical code-bases. In this case, returning NULL in any code-path from a returns_nonnull function, even if the path is currently unreachable via some macro configuration, seems a bad idea. Of course, I'm open to be proven wrong :-) >> Moreover, this warning works in exactly the same cases as >> __attribute__((nonnull)) does for function arguments, and so far those >> haven't been a source of false positives. > > I'm sure I could write one ;-) And given that a FE based version of this > will only catch explicit NULLs in argument lists, I consider it so weak as > to be virtually useless. Well, it is catching exactly all the cases that you were testing for in your original -Wnull-attribute patch ;-) >> I'm very much in favour of this, but only for things that cannot >> reliably be warned from the FE. Clang has shown that it is possible to >> improve much more the accuracy of warnings in the FE and still compile >> faster than GCC by performing some kind of fast CCP (and VRP?) in the >> FE (or make the CCP and VRP passes occur earlier and even without >> optimization): > > And my assertion is that for things like we're discussing, you need the > analysis & optimizations both to expose the non-trivial cases and prune away > those which are false positives. I consider exposing the non-trivial cases > and pruning away false positives the most important aspect of this kind of > work. Based on my experience, I'm not convinced that moving warnings to the middle-end is a good idea. The middle-end does a very poor job keeping sensible locations when doing transformations and it will not only introduce false positives, it will also remove true positives. The diagnostics often refer to the wrong variable or code that is not what the user originally wrote, which makes very hard to understand the problem. One only has to read all the reports we have about -Warray-bounds, -Wuninitialized, -Wstrict-overflow and other middle-end warnings. For example, Clang is currently able to warn about the following case without any optimization, while GCC cannot at any optimization level: int f(bool b) { int n; if (b) n = 1; return n; } sometimes-uninit.cpp:3:7: warning: variable 'n' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] if (b) ^ sometimes-uninit.cpp:5:10: note: uninitialized use occurs here return n; ^ sometimes-uninit.cpp:3:3: note: remove the 'if' if its condition is always true if (b) ^~~~~~ sometimes-uninit.cpp:2:8: note: initialize the variable 'n' to silence this warning int n; ^ = 0 Another example is -Warray-bounds, for which the warnings from Clang are not only more precise: deadcode.cpp:7:5: warning: array index 44 is past the end of the array (which contains 42 elements) [-Warray-bounds] f(arr[N]); ^ ~ than the same warning from GCC : deadcode.cpp:7:4: warning: array subscript is above array bounds [-Warray-bounds] f(arr[N]); ^ but they work even without -O2: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35587 Cheers, Manuel.
On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > On 24 July 2015 at 21:30, Jeff Law <law@redhat.com> wrote: >> On 07/24/2015 07:45 AM, Manuel López-Ibáñez wrote: >>> >>> On 23 July 2015 at 19:43, Jeff Law <law@redhat.com> wrote: >>>> >>>> Warning in the front-ends like this can generate false positives (such as >>>> a >>>> NULL return in an unreachable path and miss cases where the NULL has to >>>> be >>>> propagated into the return by later optimizations. >>> >>> >>> False positives (for the warning as proposed right now) would be >>> strange, since it would mean that a returns_nonnull function returns >>> an explicit NULL in some code-path that is not meant to be executed. >>> That sounds like a bug waiting to happen. >> >> Depends on how you choose to look at things. It's quite common via macros & >> such to have unexecutable/unreachable paths. Whether or not to warn about >> something found on such a path is a matter of personal preference. > > I think it is also a matter of the particular warning and on the > balance of true positives vs. false positives in typical code-bases. > In this case, returning NULL in any code-path from a returns_nonnull > function, even if the path is currently unreachable via some macro > configuration, seems a bad idea. Of course, I'm open to be proven > wrong :-) > >>> Moreover, this warning works in exactly the same cases as >>> __attribute__((nonnull)) does for function arguments, and so far those >>> haven't been a source of false positives. >> >> I'm sure I could write one ;-) And given that a FE based version of this >> will only catch explicit NULLs in argument lists, I consider it so weak as >> to be virtually useless. > > Well, it is catching exactly all the cases that you were testing for > in your original -Wnull-attribute patch ;-) > >>> I'm very much in favour of this, but only for things that cannot >>> reliably be warned from the FE. Clang has shown that it is possible to >>> improve much more the accuracy of warnings in the FE and still compile >>> faster than GCC by performing some kind of fast CCP (and VRP?) in the >>> FE (or make the CCP and VRP passes occur earlier and even without >>> optimization): >> >> And my assertion is that for things like we're discussing, you need the >> analysis & optimizations both to expose the non-trivial cases and prune away >> those which are false positives. I consider exposing the non-trivial cases >> and pruning away false positives the most important aspect of this kind of >> work. > > Based on my experience, I'm not convinced that moving warnings to the > middle-end is a good idea. The middle-end does a very poor job > keeping sensible locations when doing transformations and it will not > only introduce false positives, it will also remove true positives. > The diagnostics often refer to the wrong variable or code that is not > what the user originally wrote, which makes very hard to understand > the problem. One only has to read all the reports we have about > -Warray-bounds, -Wuninitialized, -Wstrict-overflow and other > middle-end warnings. > > For example, Clang is currently able to warn about the following case > without any optimization, while GCC cannot at any optimization level: > > int f(bool b) { > int n; > if (b) > n = 1; > return n; > } Is there a PR for this particular test case? I am interested in improving the uninit analysis for gcc 6 so this potentially seems up my alley.
On 25 July 2015 at 00:17, Patrick Palka <patrick@parcs.ath.cx> wrote: > On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez > <lopezibanez@gmail.com> wrote: > Is there a PR for this particular test case? I am interested in > improving the uninit analysis for gcc 6 so this potentially seems up > my alley. We do not warn because of the infamous PR18501 (probably the -Wuninitialized bug with the highest number of duplicates), where CPP removes the default SSA definition of n and simply returns 1 unconditionally. But fixing PR18501 may not be necessary to detect this case (Clang does it before doing any optimization). There are other cases that would be better warned from the FE: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19808 Cheers, Manuel. .
On Fri, Jul 24, 2015 at 11:55:23PM +0200, Manuel López-Ibáñez wrote: > On 24 July 2015 at 21:30, Jeff Law <law@redhat.com> wrote: > > On 07/24/2015 07:45 AM, Manuel López-Ibáñez wrote: > >> > >> On 23 July 2015 at 19:43, Jeff Law <law@redhat.com> wrote: > >>> > >>> Warning in the front-ends like this can generate false positives (such as > >>> a > >>> NULL return in an unreachable path and miss cases where the NULL has to > >>> be > >>> propagated into the return by later optimizations. > >> > >> > >> False positives (for the warning as proposed right now) would be > >> strange, since it would mean that a returns_nonnull function returns > >> an explicit NULL in some code-path that is not meant to be executed. > >> That sounds like a bug waiting to happen. > > > > Depends on how you choose to look at things. It's quite common via macros & > > such to have unexecutable/unreachable paths. Whether or not to warn about > > something found on such a path is a matter of personal preference. > > I think it is also a matter of the particular warning and on the > balance of true positives vs. false positives in typical code-bases. > In this case, returning NULL in any code-path from a returns_nonnull > function, even if the path is currently unreachable via some macro > configuration, seems a bad idea. Of course, I'm open to be proven > wrong :-) I'd actually expect reasonable cases to be fairly common. For example T * foobar (int x) { if (blah) { UNREACHABLE(); return NULL; } ... } You may want to return a value to make some random compiler happy or whatever, but obviously the code should never run, and you may not have a real value. Another case is Foo * bar() { #if SHOULD_USE_BAR ... #else return NULL; #endif } And somehow your program is setup so bar is only called when SHOULD_USE_BAR is defined. In that sort of case it may be convenient for the function bar to always be defined, but not for the type Foo to be defined in which case there is no real value for the function to return. > >> Moreover, this warning works in exactly the same cases as > >> __attribute__((nonnull)) does for function arguments, and so far those > >> haven't been a source of false positives. > > > > I'm sure I could write one ;-) And given that a FE based version of this > > will only catch explicit NULLs in argument lists, I consider it so weak as > > to be virtually useless. > > Well, it is catching exactly all the cases that you were testing for > in your original -Wnull-attribute patch ;-) maybe, but I'd bet that just means the tests aren't very extensive ;-) > >> I'm very much in favour of this, but only for things that cannot > >> reliably be warned from the FE. Clang has shown that it is possible to > >> improve much more the accuracy of warnings in the FE and still compile > >> faster than GCC by performing some kind of fast CCP (and VRP?) in the > >> FE (or make the CCP and VRP passes occur earlier and even without > >> optimization): > > > > And my assertion is that for things like we're discussing, you need the > > analysis & optimizations both to expose the non-trivial cases and prune away > > those which are false positives. I consider exposing the non-trivial cases > > and pruning away false positives the most important aspect of this kind of > > work. > > Based on my experience, I'm not convinced that moving warnings to the > middle-end is a good idea. The middle-end does a very poor job > keeping sensible locations when doing transformations and it will not > only introduce false positives, it will also remove true positives. > The diagnostics often refer to the wrong variable or code that is not > what the user originally wrote, which makes very hard to understand > the problem. One only has to read all the reports we have about > -Warray-bounds, -Wuninitialized, -Wstrict-overflow and other > middle-end warnings. On the other hand those warnings absolutely require non trivial analysis, so if you can share code between analyzing for warnings and for optimization that seems valuable. Similarly if you can share the code between languages that seems useful. Of course that is even more important if you want to catch things that only become visible with inlining. > For example, Clang is currently able to warn about the following case > without any optimization, while GCC cannot at any optimization level: So at some point we're just talking semantics of words, but I think you could make a credible argument that clang doesn't have a true -O0 mode. I'm no expert on clang internals, but I think the place they draw a line between what is a front end and what is a back is farther back than where gcc draws it, but that's basically just a guess. Trev > > int f(bool b) { > int n; > if (b) > n = 1; > return n; > } > > sometimes-uninit.cpp:3:7: warning: variable 'n' is used uninitialized > whenever 'if' condition is false [-Wsometimes-uninitialized] > if (b) > ^ > sometimes-uninit.cpp:5:10: note: uninitialized use occurs here > return n; > ^ > sometimes-uninit.cpp:3:3: note: remove the 'if' if its condition is always true > if (b) > ^~~~~~ > sometimes-uninit.cpp:2:8: note: initialize the variable 'n' to silence > this warning > int n; > ^ > = 0 > > Another example is -Warray-bounds, for which the warnings from Clang > are not only more precise: > > deadcode.cpp:7:5: warning: array index 44 is past the end of the array > (which contains 42 elements) [-Warray-bounds] > f(arr[N]); > ^ ~ > > than the same warning from GCC : > > deadcode.cpp:7:4: warning: array subscript is above array bounds > [-Warray-bounds] > f(arr[N]); > ^ > > but they work even without -O2: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35587 > > Cheers, > > Manuel.
On 25 July 2015 at 01:15, Trevor Saunders <tbsaunde@tbsaunde.org> wrote: > I'd actually expect reasonable cases to be fairly common. For example I added it to -Wall, I guess we'll see... not sure how many users of returns_nonnull are out there. In all your cases there is no reason to prefer NULL rather than, say, (T*)0xdeadbeef. Moreover, moving the warning to the ME does not guarantee that no false positives will be emitted either. It also does not guarantee that no false positives will be introduced by optimization that did not exist in the user code. And optimizations may remove trivial true positives, which seems even worse. > So at some point we're just talking semantics of words, but I think you > could make a credible argument that clang doesn't have a true -O0 mode. > I'm no expert on clang internals, but I think the place they draw a line > between what is a front end and what is a back is farther back than > where gcc draws it, but that's basically just a guess. I think they make a distinction between dataflow analysis and code transformation. Clang does dataflow analysis on the AST without changing the code (I think it can even do interprocedural dataflow), but it leaves the succession of analysis and transformation passes to LLVM, which does not do warnings. I do not know the precise details of how all this works, but it seems to work better and faster than what GCC does right now for Wuninitialized. Cheers, Manuel.
On 07/24/2015 03:55 PM, Manuel López-Ibáñez wrote: > > I think it is also a matter of the particular warning and on the > balance of true positives vs. false positives in typical code-bases. > In this case, returning NULL in any code-path from a returns_nonnull > function, even if the path is currently unreachable via some macro > configuration, seems a bad idea. Of course, I'm open to be proven > wrong :-) Again, this is a matter of personal preference and I think we come from two totally different mindsets when it comes to this class of warnings. > >>> Moreover, this warning works in exactly the same cases as >>> __attribute__((nonnull)) does for function arguments, and so far those >>> haven't been a source of false positives. >> >> I'm sure I could write one ;-) And given that a FE based version of this >> will only catch explicit NULLs in argument lists, I consider it so weak as >> to be virtually useless. > > Well, it is catching exactly all the cases that you were testing for > in your original -Wnull-attribute patch ;-) Which is an argument that we should have more complex testcases -- there's certainly real world code where this kind of stuff is an issue. > >>> I'm very much in favour of this, but only for things that cannot >>> reliably be warned from the FE. Clang has shown that it is possible to >>> improve much more the accuracy of warnings in the FE and still compile >>> faster than GCC by performing some kind of fast CCP (and VRP?) in the >>> FE (or make the CCP and VRP passes occur earlier and even without >>> optimization): >> >> And my assertion is that for things like we're discussing, you need the >> analysis & optimizations both to expose the non-trivial cases and prune away >> those which are false positives. I consider exposing the non-trivial cases >> and pruning away false positives the most important aspect of this kind of >> work. > > Based on my experience, I'm not convinced that moving warnings to the > middle-end is a good idea. The middle-end does a very poor job > keeping sensible locations when doing transformations and it will not > only introduce false positives, it will also remove true positives. > The diagnostics often refer to the wrong variable or code that is not > what the user originally wrote, which makes very hard to understand > the problem. One only has to read all the reports we have about > -Warray-bounds, -Wuninitialized, -Wstrict-overflow and other > middle-end warnings. Again, I think we come at this from fundamentally different viewpoints. I'm willing to compromise on locations and I think we have a different viewpoint of what a true positive is. If it's on an unreachable path, then I consider warning about it a false positive. Those are based on decades of writing the analysis, then using the result then being horrified when I see things that are missed because a particular warning is so weak that it's nearly useless (-Warray-bounds comes to mind). > > For example, Clang is currently able to warn about the following case > without any optimization, while GCC cannot at any optimization level: > > int f(bool b) { > int n; > if (b) > n = 1; > return n; > } ANd this is a known failing in GCC. Interestingly enough I believe my proposed revamping of how we handle uninitialized warnings would catch this IIRC. >> Another example is -Warray-bounds, for which the warnings from Clang > are not only more precise: Our implementation of -Warray-bounds sucks because it only warns when we have a range that collapses down to a singularity that feeds into an array reference. It does absolutely no may-be-out-of-bounds analysis. For that reason I consider -Warray-bounds as it stands today fundamentally broken. > > but they work even without -O2: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35587 I don't really care about that. Others do, I don't. But this is something folks can fix if the consensus is that we want these warnings with the optimizer off. It's a trade-off, just like just about everything in this space. jeff
On 07/24/2015 04:17 PM, Patrick Palka wrote: > On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez > <lopezibanez@gmail.com> wrote: >> On 24 July 2015 at 21:30, Jeff Law <law@redhat.com> wrote: >>> On 07/24/2015 07:45 AM, Manuel López-Ibáñez wrote: >>>> >>>> On 23 July 2015 at 19:43, Jeff Law <law@redhat.com> wrote: >>>>> >>>>> Warning in the front-ends like this can generate false positives (such as >>>>> a >>>>> NULL return in an unreachable path and miss cases where the NULL has to >>>>> be >>>>> propagated into the return by later optimizations. >>>> >>>> >>>> False positives (for the warning as proposed right now) would be >>>> strange, since it would mean that a returns_nonnull function returns >>>> an explicit NULL in some code-path that is not meant to be executed. >>>> That sounds like a bug waiting to happen. >>> >>> Depends on how you choose to look at things. It's quite common via macros & >>> such to have unexecutable/unreachable paths. Whether or not to warn about >>> something found on such a path is a matter of personal preference. >> >> I think it is also a matter of the particular warning and on the >> balance of true positives vs. false positives in typical code-bases. >> In this case, returning NULL in any code-path from a returns_nonnull >> function, even if the path is currently unreachable via some macro >> configuration, seems a bad idea. Of course, I'm open to be proven >> wrong :-) >> >>>> Moreover, this warning works in exactly the same cases as >>>> __attribute__((nonnull)) does for function arguments, and so far those >>>> haven't been a source of false positives. >>> >>> I'm sure I could write one ;-) And given that a FE based version of this >>> will only catch explicit NULLs in argument lists, I consider it so weak as >>> to be virtually useless. >> >> Well, it is catching exactly all the cases that you were testing for >> in your original -Wnull-attribute patch ;-) >> >>>> I'm very much in favour of this, but only for things that cannot >>>> reliably be warned from the FE. Clang has shown that it is possible to >>>> improve much more the accuracy of warnings in the FE and still compile >>>> faster than GCC by performing some kind of fast CCP (and VRP?) in the >>>> FE (or make the CCP and VRP passes occur earlier and even without >>>> optimization): >>> >>> And my assertion is that for things like we're discussing, you need the >>> analysis & optimizations both to expose the non-trivial cases and prune away >>> those which are false positives. I consider exposing the non-trivial cases >>> and pruning away false positives the most important aspect of this kind of >>> work. >> >> Based on my experience, I'm not convinced that moving warnings to the >> middle-end is a good idea. The middle-end does a very poor job >> keeping sensible locations when doing transformations and it will not >> only introduce false positives, it will also remove true positives. >> The diagnostics often refer to the wrong variable or code that is not >> what the user originally wrote, which makes very hard to understand >> the problem. One only has to read all the reports we have about >> -Warray-bounds, -Wuninitialized, -Wstrict-overflow and other >> middle-end warnings. >> >> For example, Clang is currently able to warn about the following case >> without any optimization, while GCC cannot at any optimization level: >> >> int f(bool b) { >> int n; >> if (b) >> n = 1; >> return n; >> } > > Is there a PR for this particular test case? I am interested in > improving the uninit analysis for gcc 6 so this potentially seems up > my alley. To fix this you have to stop the reduction of degenerate PHIs when the RHS has a single real value and one or more undefined values. One of the advantages of the two pass scheme I suggested years ago is it would allow us to detect this before the optimizers collapsed the degenerate PHI. jeff >
On 07/24/2015 04:41 PM, Manuel López-Ibáñez wrote: > On 25 July 2015 at 00:17, Patrick Palka <patrick@parcs.ath.cx> wrote: >> On Fri, Jul 24, 2015 at 5:55 PM, Manuel López-Ibáñez >> <lopezibanez@gmail.com> wrote: >> Is there a PR for this particular test case? I am interested in >> improving the uninit analysis for gcc 6 so this potentially seems up >> my alley. > > We do not warn because of the infamous PR18501 (probably the > -Wuninitialized bug with the highest number of duplicates), where CPP > removes the default SSA definition of n and simply returns 1 > unconditionally. But fixing PR18501 may not be necessary to detect > this case (Clang does it before doing any optimization). You don't have to move the warning to the FE to fix this bug. Go back to my proposal from several years ago which has a warning analysis phase at the start and the end of the optimization pipeline. It can catch this because it gets a chance to analyze the code prior to the generate PHI elimination. > > There are other cases that would be better warned from the FE: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19808 I haven't looked at any of these recently. But fundamentally you don't have to move the warning into the front-end, you just have to move it to the start of the optimization pipeline. jeff
On 07/24/2015 05:15 PM, Trevor Saunders wrote: > >>>> Moreover, this warning works in exactly the same cases as >>>> __attribute__((nonnull)) does for function arguments, and so far those >>>> haven't been a source of false positives. >>> >>> I'm sure I could write one ;-) And given that a FE based version of this >>> will only catch explicit NULLs in argument lists, I consider it so weak as >>> to be virtually useless. >> >> Well, it is catching exactly all the cases that you were testing for >> in your original -Wnull-attribute patch ;-) > > maybe, but I'd bet that just means the tests aren't very extensive ;-) Exactly. Frankly extending them to cases where they're missed by a front-end only solution, but caught after analysis is fairly trivial. Pull the constant out into a temporary. Viola, the front end only solution fails miserably. It's so trivial it didn't seem worth it, but with a FE solution in place, it's probably worth it now. > > On the other hand those warnings absolutely require non trivial > analysis, so if you can share code between analyzing for warnings and > for optimization that seems valuable. Similarly if you can share the > code between languages that seems useful. Of course that is even more > important if you want to catch things that only become visible with > inlining. > >> For example, Clang is currently able to warn about the following case >> without any optimization, while GCC cannot at any optimization level: > > So at some point we're just talking semantics of words, but I think you > could make a credible argument that clang doesn't have a true -O0 mode. > I'm no expert on clang internals, but I think the place they draw a line > between what is a front end and what is a back is farther back than > where gcc draws it, but that's basically just a guess. And that's probably a reasonable way to look at the situation. We can easily put the line at the GENERIC->GIMPLE transition, pay the compile-time penalty and generate warnings at that point regardless of -O0 or -O2. We've chosen not to do that so far. It's a piece of the discussion I don't much care about. If that's the way folks want to go, it shouldn't be hard to dust off the work I did a decade or so ago and make it work in the current tree. But I'm not terribly inclined to do it myself anymore -- primarily because I weary of arguing one way or the other. Jeff
On July 25, 2015 1:15:21 AM GMT+02:00, Trevor Saunders <tbsaunde@tbsaunde.org> wrote: > >Another case is > >Foo * >bar() >{ > #if SHOULD_USE_BAR > ... >#else > return NULL; >#endif >} > >And somehow your program is setup so bar is only called when >SHOULD_USE_BAR is defined. In that sort of case it may be convenient >for the function bar to always be defined, but not for the type Foo to >be defined in which case there is no real value for the function to >return. Which just proves that cxx is just too broken^wcomplicated to grok for any human being or compiler, for that matter |) Cheers,
Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 225868) +++ gcc/doc/invoke.texi (working copy) @@ -3709,11 +3709,13 @@ formats that may yield only a two-digit @item -Wnonnull @opindex Wnonnull @opindex Wno-nonnull Warn about passing a null pointer for arguments marked as -requiring a non-null value by the @code{nonnull} function attribute. +requiring a non-null value by the @code{nonnull} function attribute +or returning a null pointer from a function declared with the attribute +@code{returns_nonnull}. @option{-Wnonnull} is included in @option{-Wall} and @option{-Wformat}. It can be disabled with the @option{-Wno-nonnull} option. @item -Winit-self @r{(C, C++, Objective-C and Objective-C++ only)} Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 225868) +++ gcc/c-family/c-common.c (working copy) @@ -9508,10 +9508,22 @@ check_nonnull_arg (void * ARG_UNUSED (ct if (integer_zerop (param)) warning (OPT_Wnonnull, "null argument where non-null required " "(argument %lu)", (unsigned long) param_num); } +/* Possibly warn if RETVAL is a null pointer and FNDECL is declared + with attribute returns_nonnull. LOC is the location of RETVAL. */ + +void +maybe_warn_returns_nonnull (location_t loc, tree fndecl, tree retval) +{ + if (integer_zerop (retval) + && lookup_attribute ("returns_nonnull", + TYPE_ATTRIBUTES (TREE_TYPE (fndecl)))) + warning_at (loc, OPT_Wnonnull, "null return value where non-null required"); +} + /* Helper for nonnull attribute handling; fetch the operand number from the attribute argument list. */ static bool get_nonnull_operand (tree arg_num_expr, unsigned HOST_WIDE_INT *valp) Index: gcc/c-family/c-common.h =================================================================== --- gcc/c-family/c-common.h (revision 225868) +++ gcc/c-family/c-common.h (working copy) @@ -1049,10 +1049,11 @@ extern void do_warn_double_promotion (tr extern void set_underlying_type (tree); extern void record_types_used_by_current_var_decl (tree); extern void record_locally_defined_typedef (tree); extern void maybe_record_typedef_use (tree); extern void maybe_warn_unused_local_typedefs (void); +extern void maybe_warn_returns_nonnull (location_t, tree, tree); extern void maybe_warn_bool_compare (location_t, enum tree_code, tree, tree); extern vec<tree, va_gc> *make_tree_vector (void); extern void release_tree_vector (vec<tree, va_gc> *); extern vec<tree, va_gc> *make_tree_vector_single (tree); extern vec<tree, va_gc> *make_tree_vector_from_list (tree); Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 225868) +++ gcc/c/c-typeck.c (working copy) @@ -9372,10 +9372,11 @@ c_finish_return (location_t loc, tree re { semantic_type = TREE_TYPE (retval); retval = TREE_OPERAND (retval, 0); } retval = c_fully_fold (retval, false, NULL); + maybe_warn_returns_nonnull (loc, current_function_decl, retval); if (semantic_type) retval = build1 (EXCESS_PRECISION_EXPR, semantic_type, retval); } if (!retval) Index: gcc/testsuite/c-c++-common/wnonnull-1.c =================================================================== --- gcc/testsuite/c-c++-common/wnonnull-1.c (revision 0) +++ gcc/testsuite/c-c++-common/wnonnull-1.c (revision 0) @@ -0,0 +1,42 @@ +/* { dg-do compile } */ +/* { dg-options "-Wnonnull" } */ + + +extern void foo(void *) __attribute__ ((__nonnull__ (1))); + +int z; +int y; + +void +com (int a) +{ + foo (a == 42 ? &z : (void *) 0); /* { dg-warning "null" } */ +} + +void +bar (void) +{ + foo ((void *)0); /* { dg-warning "null" } */ +} + +int * foo_r(int a) __attribute__((returns_nonnull)); +int * bar_r(void) __attribute__((returns_nonnull)); + +int * +foo_r(int a) +{ + switch (a) + { + case 0: + return &z; + default: + return (int *)0; /* { dg-warning "null" } */ + } +} + +int * +bar_r (void) +{ + return 0; /* { dg-warning "null" } */ +} + Index: gcc/cp/typeck.c =================================================================== --- gcc/cp/typeck.c (revision 225868) +++ gcc/cp/typeck.c (working copy) @@ -8702,10 +8702,12 @@ check_return_expr (tree retval, bool *no /* We don't need to do any conversions when there's nothing being returned. */ if (!retval) return NULL_TREE; + maybe_warn_returns_nonnull (input_location, current_function_decl, retval); + /* Do any required conversions. */ if (retval == result || DECL_CONSTRUCTOR_P (current_function_decl)) /* No conversions are required. */ ; else
While looking at PR c/16351, I noticed that all tests proposed for -Wnull-attribute (https://gcc.gnu.org/ml/gcc-patches/2014-01/msg01715.html) could be warned from the FEs by simply extending the existing Wnonnull. Bootstrapped and regression tested on x86_64-linux-gnu. OK? gcc/ChangeLog: 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> PR c/16351 * doc/invoke.texi (Wnonnull): Document behavior for returns_nonnull. gcc/testsuite/ChangeLog: 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> PR c/16351 * c-c++-common/wnonnull-1.c: New test. gcc/cp/ChangeLog: 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> PR c/16351 * typeck.c (check_return_expr): Call maybe_warn_returns_nonnull. gcc/c-family/ChangeLog: 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> PR c/16351 * c-common.c (maybe_warn_returns_nonnull): New. * c-common.h (maybe_warn_returns_nonnull): Declare. gcc/c/ChangeLog: 2015-07-22 Manuel López-Ibáñez <manu@gcc.gnu.org> PR c/16351 * c-typeck.c (c_finish_return): Call maybe_warn_returns_nonnull.