Message ID | 7390933.EvYhyI6sBW@fomalhaut |
---|---|
State | New |
Headers | show |
Series | Do not erase warning data in gimple_set_location | expand |
On 6/10/2022 4:57 AM, Eric Botcazou via Gcc-patches wrote: > Hi, > > gimple_set_location is mostly invoked on newly built GIMPLE statements, so > their location is UNKNOWN_LOCATION and setting it will clobber the warning > data of the passed location, if any. > > Tested on x86-64/Linux, OK for mainline and 12 branch? > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > * gimple.h (gimple_set_location): Do not copy warning data from > the previous location when it is UNKNOWN_LOCATION. > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > testsuite/ > * c-c++-common/nonnull-1.c: Remove XFAIL for C++. OK for trunk and gcc-12. jeff
On Fri, Jun 10, 2022 at 12:58 PM Eric Botcazou via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi, > > gimple_set_location is mostly invoked on newly built GIMPLE statements, so > their location is UNKNOWN_LOCATION and setting it will clobber the warning > data of the passed location, if any. Hmm, I think instead of special-casing UNKNOWN_LOCATION what gimple_set_location should probably do is either not copy warnings at all or union them. Btw, gimple_set_location also removes a previously set BLOCK (but gimple_set_block preserves the location locus and diagnostic override). So I'd be tempted to axe the copy_warning () completely here. Martin, there were probably cases that warranted it - do you remember anything specific here? Thanks, Richard. > Tested on x86-64/Linux, OK for mainline and 12 branch? > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > * gimple.h (gimple_set_location): Do not copy warning data from > the previous location when it is UNKNOWN_LOCATION. > > > 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> > > testsuite/ > * c-c++-common/nonnull-1.c: Remove XFAIL for C++. > > -- > Eric Botcazou
On 6/13/22 05:15, Richard Biener wrote: > On Fri, Jun 10, 2022 at 12:58 PM Eric Botcazou via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> Hi, >> >> gimple_set_location is mostly invoked on newly built GIMPLE statements, so >> their location is UNKNOWN_LOCATION and setting it will clobber the warning >> data of the passed location, if any. > > Hmm, I think instead of special-casing UNKNOWN_LOCATION > what gimple_set_location should probably do is either not copy > warnings at all or union them. Btw, gimple_set_location also > removes a previously set BLOCK (but gimple_set_block preserves > the location locus and diagnostic override). > > So I'd be tempted to axe the copy_warning () completely here. Martin, > there were > probably cases that warranted it - do you remember anything specific here? Nothing specific, just that the assumption behind the warning group design was that a location must exist in order to suppress a warning (a location is one of the first things that's set early on by the FE and it makes little sense to issue a warning without one). There was and in all likelihood still is code sets TREE_NO_WARNING or gimple_no_warning on new trees/statements before setting their location. That interferes with the design when the new tree or statement is meant to be a replacement of another. I fixed a few cases like that to set the location first but didn't have a way of finding all such instances. My expectation was to over time change GCC to make sure a location would always be set before the no-warning bit, and asserting that on every call to these routines. Adding tests like in the patch below goes in the opposite direction and effectively papers over the problem. I can't think of a way to make the suppression work completely reliably without ensuring that a location is always set before suppressing a warning. Martin > > Thanks, > Richard. > >> Tested on x86-64/Linux, OK for mainline and 12 branch? >> >> >> 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> >> >> * gimple.h (gimple_set_location): Do not copy warning data from >> the previous location when it is UNKNOWN_LOCATION. >> >> >> 2022-06-10 Eric Botcazou <ebotcazou@adacore.com> >> >> testsuite/ >> * c-c++-common/nonnull-1.c: Remove XFAIL for C++. >> >> -- >> Eric Botcazou >
> Hmm, I think instead of special-casing UNKNOWN_LOCATION > what gimple_set_location should probably do is either not copy > warnings at all or union them. Btw, gimple_set_location also > removes a previously set BLOCK (but gimple_set_block preserves > the location locus and diagnostic override). > > So I'd be tempted to axe the copy_warning () completely here. The first thing I tried, but it regressed the original testcase IIRC. Even my minimal patch manages to break bootstrap on ARM: buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/libcpp/lex.cc: 1523:9: error: pointer used after ‘void operator delete(void*, std::size_t)’ [-Werror=use-after-free] # 00:31:04 make[3]: *** [Makefile:227: lex.o] Error 1 # 00:31:04 make[2]: *** [Makefile:9527: all-stage3-libcpp] Error 2 # 00:31:35 make[1]: *** [Makefile:25887: stage3-bubble] Error 2 # 00:31:35 make: *** [Makefile:1072: all] Error 2 /* Don't warn for cases like when a cdtor returns 'this' on ARM. */ else if (warning_suppressed_p (var, OPT_Wuse_after_free)) return; because warning-control.cc:copy_warning also clobbers the warning data of the destination. We have in cp/decl.cc:maybe_return_this the lines: /* Return the address of the object. */ tree val = DECL_ARGUMENTS (current_function_decl); suppress_warning (val, OPT_Wuse_after_free); -Wuse-after-free is suppressed for the location of VAL and the TREE_NO_WARNING bit set on it. But other expressions may have the same location as VAL and the TREE_NO_WARNING bit _not_ set, so when you call copy_warning (expr, expr) (we do that a lot after failed folding) for them, copy_warning erases the warning data of the location. I have installed the obvious fixlet after testing on x86-64/Linux, but the decoupling between TREE_NO_WARNING bit and location looks a bit problematic. * warning-control.cc (copy_warning) [generic version]: Do not erase the warning data of the destination location when the no-warning bit is not set on the source. (copy_warning) [tree version]: Return early if TO is equal to FROM. (copy_warning) [gimple version]: Likewise. testsuite/ * g++.dg/warn/Wuse-after-free5.C: New test.
On Tue, Jun 14, 2022 at 12:49 PM Eric Botcazou <botcazou@adacore.com> wrote: > > > Hmm, I think instead of special-casing UNKNOWN_LOCATION > > what gimple_set_location should probably do is either not copy > > warnings at all or union them. Btw, gimple_set_location also > > removes a previously set BLOCK (but gimple_set_block preserves > > the location locus and diagnostic override). > > > > So I'd be tempted to axe the copy_warning () completely here. > > The first thing I tried, but it regressed the original testcase IIRC. > > Even my minimal patch manages to break bootstrap on ARM: > > buildslave/workspace/tcwg_gnu_1/abe/snapshots/gcc.git~master/libcpp/lex.cc: > 1523:9: error: pointer used after ‘void operator delete(void*, std::size_t)’ > [-Werror=use-after-free] > # 00:31:04 make[3]: *** [Makefile:227: lex.o] Error 1 > # 00:31:04 make[2]: *** [Makefile:9527: all-stage3-libcpp] Error 2 > # 00:31:35 make[1]: *** [Makefile:25887: stage3-bubble] Error 2 > # 00:31:35 make: *** [Makefile:1072: all] Error 2 > > /* Don't warn for cases like when a cdtor returns 'this' on ARM. */ > else if (warning_suppressed_p (var, OPT_Wuse_after_free)) > return; > > because warning-control.cc:copy_warning also clobbers the warning data of the > destination. We have in cp/decl.cc:maybe_return_this the lines: > > /* Return the address of the object. */ > tree val = DECL_ARGUMENTS (current_function_decl); > suppress_warning (val, OPT_Wuse_after_free); > > -Wuse-after-free is suppressed for the location of VAL and the TREE_NO_WARNING > bit set on it. But other expressions may have the same location as VAL and > the TREE_NO_WARNING bit _not_ set, so when you call copy_warning (expr, expr) > (we do that a lot after failed folding) for them, copy_warning erases the > warning data of the location. > > I have installed the obvious fixlet after testing on x86-64/Linux, but the > decoupling between TREE_NO_WARNING bit and location looks a bit problematic. Thanks - that makes more sense. > > * warning-control.cc (copy_warning) [generic version]: Do not erase > the warning data of the destination location when the no-warning > bit is not set on the source. > (copy_warning) [tree version]: Return early if TO is equal to FROM. > (copy_warning) [gimple version]: Likewise. > testsuite/ > * g++.dg/warn/Wuse-after-free5.C: New test. > > -- > Eric Botcazou
diff --git a/gcc/gimple.h b/gcc/gimple.h index 6b1e89ad74e..870629cd562 100644 --- a/gcc/gimple.h +++ b/gcc/gimple.h @@ -1913,7 +1913,8 @@ static inline void gimple_set_location (gimple *g, location_t location) { /* Copy the no-warning data to the statement location. */ - copy_warning (location, g->location); + if (g->location != UNKNOWN_LOCATION) + copy_warning (location, g->location); g->location = location; } diff --git a/gcc/testsuite/c-c++-common/nonnull-1.c b/gcc/testsuite/c-c++-common/nonnull-1.c index ea987365302..7be4e3479dd 100644 --- a/gcc/testsuite/c-c++-common/nonnull-1.c +++ b/gcc/testsuite/c-c++-common/nonnull-1.c @@ -30,5 +30,5 @@ func (char *cp1, char *cp2, char *cp3, char *cp4) __attribute__((nonnull (1))) int func2 (char *cp) { - return (cp != NULL) ? 1 : 0; /* { dg-warning "'nonnull' argument" "cp compared to NULL" { xfail c++ } } */ + return (cp != NULL) ? 1 : 0; /* { dg-warning "'nonnull' argument" "cp compared to NULL" } */ }