On Fri, Sep 30, 2022 at 12:11 AM Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Warnings issued for -Wuninitialized have been using the spelling location of > the problematic usage, discarding any information on the location of the macro > expansion point if such usage was in a macro. This makes the warnings > impossible to control reliably with #pragma GCC diagnostic, and also discards > useful context in the diagnostic output. There seems to be no need to discard > the virtual location information, so this patch fixes that. > > PR69543 was mostly about _Pragma issues which have been fixed for many years > now. The PR remains open because two of the testcases added in response to it > still have xfails, but those xfails have nothing to do with _Pragma and rather > just with the issue fixed by this patch, so the PR can be closed now as well. > > The other testcase modified here, pragma-diagnostic-2.c, was explicitly > testing for the undesirable behavior that was xfailed in pr69543-3.c. I have > adjusted that and also added a new testcase verifying all 3 types of warning > that come from tree-ssa-uninit.cc get the proper location information now.
OK. Thanks for the extensive rationale and digging up history. Thanks, Richard. > gcc/ChangeLog: > > PR preprocessor/69543 > * tree-ssa-uninit.cc (warn_uninit): Stop stripping macro tracking > information away from the diagnostic location. > (maybe_warn_read_write_only): Likewise. > (maybe_warn_operand): Likewise. > > gcc/testsuite/ChangeLog: > > PR preprocessor/69543 > * c-c++-common/pr69543-3.c: Remove xfail. > * c-c++-common/pr69543-4.c: Likewise. > * gcc.dg/cpp/pragma-diagnostic-2.c: Adjust test for new behavior. > * c-c++-common/pragma-diag-16.c: New test. > --- > > Notes: > Hello- > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69543#c9 > > This patch resolves two xfail'ed testcases discussed on the PR. David > seems to > have fully analyzed the situation back in 2017, but stopped short of > pushing > any changes. I am working my way through resolving the remaining _Pragma > related PRs and it would be nice to close this one too. As David > mentioned, > the issue here is that -Wuninitialized warnings are using the wrong > location, > well they discard the macro tracking information and use only the spelling > point of the uninitialized usage. But '#pragma GCC diagnostic' can never > work > reliably if this is done; it needs to know the macro expansion point in > order to look up the diagnostic enablement state as the user would > naturally > interpret it. As a quick example: > > ============ > int g; > #define SET(a, b) ((a) = (b)) > void f () > { > int x; > #pragma GCC diagnostic ignored "-Wuninitialized" > SET (g, x); > } > ============ > > The current status without this patch is that because the macro tracking > information is removed from the location when the diagnostic is issued, > the > location for the diagnostic is effectively line 2, prior to the #pragma, > and > so the diagnostic does not get suppressed. But I think it seems clear that > users expect it should be suppressed in this case. SET could be buried in > some > utility header and in any case has nothing to do with the function or the > actual issue, so its location should not impact whether or not the > diagnostic > gets issued. > > As David also mentioned on the PR, the behavior was changed intentionally > by > r186971 in 2012. Dodji's rationale here: > > https://gcc.gnu.org/ml/gcc-patches/2012-04/msg00574.html > > indicates that this was necessary to avoid some undesirable locations on > the > informative notes for the diagnostic, but does not provide any specific > examples of that, and I am not able to find any cases myself where it is > worse > with the virtual location restored. Dodji stated it related to cases > where the > variable definition (as opposed to the usage) occurs in a macro, but such > cases are unaffected by my patch, since the same virtual location is used > for the note about the declaration either way. I think a lot has changed > since > that time, and the original rationale likely no longer applies. Given that > it does definitely cause a real problem, and users seem to be rather > interested in being able to suppress diagnostics with pragmas, I feel it > makes > sense to change it back and stop discarding the macro tracking information > when generating the diagnostic. > > Please let me know what you think? bootstrap/regtest all languages looks > good > on x86-64 Linux: > > FAIL 105 105 > PASS 547685 547801 > UNSUPPORTED 15435 15435 > UNTESTED 136 136 > XFAIL 4149 4129 > XPASS 17 17 > > Thanks! > > -Lewis > > gcc/testsuite/c-c++-common/pr69543-3.c | 8 +-- > gcc/testsuite/c-c++-common/pr69543-4.c | 8 +-- > gcc/testsuite/c-c++-common/pragma-diag-16.c | 63 +++++++++++++++++++ > .../gcc.dg/cpp/pragma-diagnostic-2.c | 7 ++- > gcc/tree-ssa-uninit.cc | 12 +--- > 5 files changed, 73 insertions(+), 25 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/pragma-diag-16.c > > diff --git a/gcc/testsuite/c-c++-common/pr69543-3.c > b/gcc/testsuite/c-c++-common/pr69543-3.c > index fcf750cc05d..6d4224f4af7 100644 > --- a/gcc/testsuite/c-c++-common/pr69543-3.c > +++ b/gcc/testsuite/c-c++-common/pr69543-3.c > @@ -3,15 +3,11 @@ > /* Verify disabling a warning, where the _Pragma is in regular code, > but the affected code is within a macro. */ > > -/* TODO: XFAIL: both C and C++ erroneously fail to suppress the warning > - The warning is reported at the macro definition location, rather than > - the macro expansion location. */ > - > -#define WARNABLE_CODE *++yyvsp = yylval; /* { dg-bogus "used uninitialized" > "" { xfail *-*-* } } */ > +#define WARNABLE_CODE *++yyvsp = yylval; /* { dg-bogus "used uninitialized" > } */ > > void test (char yylval) > { > - char *yyvsp; /* { dg-bogus "declared here" "" { xfail *-*-* } } */ > + char *yyvsp; /* { dg-bogus "declared here" } */ > _Pragma ("GCC diagnostic push") > _Pragma ("GCC diagnostic ignored \"-Wuninitialized\"") > _Pragma ("GCC diagnostic ignored \"-Wmaybe-uninitialized\"") > diff --git a/gcc/testsuite/c-c++-common/pr69543-4.c > b/gcc/testsuite/c-c++-common/pr69543-4.c > index cd71e7e1188..3db2eeb16eb 100644 > --- a/gcc/testsuite/c-c++-common/pr69543-4.c > +++ b/gcc/testsuite/c-c++-common/pr69543-4.c > @@ -3,10 +3,6 @@ > /* Verify disabling a warning, where both the _Pragma and the > affected code are within (different) macros. */ > > -/* TODO: XFAIL: both C and C++ erroneously fail to suppress the warning > - The warning is reported at the macro definition location, rather than > - the macro expansion location. */ > - > # define YY_IGNORE_MAYBE_UNINITIALIZED_BEGIN \ > _Pragma ("GCC diagnostic push") \ > _Pragma ("GCC diagnostic ignored \"-Wuninitialized\"")\ > @@ -14,11 +10,11 @@ > # define YY_IGNORE_MAYBE_UNINITIALIZED_END \ > _Pragma ("GCC diagnostic pop") > > -#define WARNABLE_CODE *++yyvsp = yylval; /* { dg-bogus "used uninitialized" > "" { xfail *-*-* } } */ > +#define WARNABLE_CODE *++yyvsp = yylval; /* { dg-bogus "used uninitialized" > } */ > > void test (char yylval) > { > - char *yyvsp; /* { dg-bogus "declared here" "" { xfail *-*-* } } */ > + char *yyvsp; /* { dg-bogus "declared here" } */ > YY_IGNORE_MAYBE_UNINITIALIZED_BEGIN > WARNABLE_CODE > YY_IGNORE_MAYBE_UNINITIALIZED_END > diff --git a/gcc/testsuite/c-c++-common/pragma-diag-16.c > b/gcc/testsuite/c-c++-common/pragma-diag-16.c > new file mode 100644 > index 00000000000..8cacd872bef > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/pragma-diag-16.c > @@ -0,0 +1,63 @@ > +/* Make sure that the 3 types of warnings generated from tree-ssa-uninit.cc > have > + proper virtual locations and so can be controlled by pragmas when they > appear > + in macros. */ > + > +/* { dg-do compile } */ > +/* { dg-options "-Wuninitialized -Wmaybe-uninitialized" } */ > + > +/* 1. Check maybe_warn_read_write_only(). */ > +#define DEREF1(p) (*p) /* { dg-warning {may be used uninitialized} } */ > +__attribute__ ((access (write_only, 1))) > +int f1 (int* x) /* { dg-note {accessing argument 1} } */ > +{ > + return DEREF1 (x); /* { dg-note {in expansion of macro 'DEREF1'} } */ > +} > + > +#define DEREF2(p) (*p) /* { dg-bogus {may be used uninitialized} } */ > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" > +__attribute__ ((access (write_only, 1))) > +int f2 (int* x) /* { dg-bogus {accessing argument 1} } */ > +{ > + return DEREF2 (x); /* { dg-bogus {in expansion of macro 'DEREF1'} } */ > +} > +#pragma GCC diagnostic pop > + > +/* 2. Check warn_uninit(). */ > +int g; > +#define SET3(a, b) ((a) = (b)) /* { dg-warning {'x' is used uninitialized} } > */ > +void f3 () > +{ > + int x; /* { dg-note {'x' was declared here} } */ > + SET3 (g, x); /* { dg-note {in expansion of macro 'SET3'} } */ > +} > + > +#define SET4(a, b) ((a) = (b)) /* { dg-bogus {'x' is used uninitialized} } */ > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wuninitialized" > +void f4 () > +{ > + int x; /* { dg-bogus {'x' was declared here} } */ > + SET4 (g, x); /* { dg-bogus {in expansion of macro 'SET3'} } */ > +} > +#pragma GCC diagnostic pop > + > +/* 3. Check maybe_warn_operand(). */ > +#define CALL5(func, arg) ((func) (arg)) /* { dg-warning {'c' may be used > uninitialized} } */ > +void f5a (const char *); /* { dg-note {by argument 1} } */ > +void f5b () > +{ > + char c; /* { dg-note {'c' declared here} } */ > + CALL5 (f5a, &c); /* { dg-note {in expansion of macro 'CALL5'} } */ > +} > + > +#define CALL6(func, arg) ((func) (arg)) /* { dg-bogus {'c' may be used > uninitialized} } */ > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" > +void f6a (const char *); /* { dg-bogus {by argument 1} } */ > +void f6b () > +{ > + char c; /* { dg-bogus {'c' declared here} } */ > + CALL6 (f6a, &c); /* { dg-bogus {in expansion of macro 'CALL6'} } */ > +} > +#pragma GCC diagnostic pop > diff --git a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c > b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c > index 38fc77c47ba..89d2975ee7b 100644 > --- a/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c > +++ b/gcc/testsuite/gcc.dg/cpp/pragma-diagnostic-2.c > @@ -7,18 +7,17 @@ void f (unsigned); > > #define CODE_WITH_WARNING \ > int a; /* { dg-message "was declared here" } */ \ > - f (a) /* { dg-warning "used uninitialized" } */ > + f (a) /* { dg-error "used uninitialized" } */ > > #pragma GCC diagnostic ignored "-Wuninitialized" > > void > g (void) > { > + /* No warning expected here since the #pragma is in effect. */ > CODE_WITH_WARNING; > } > > -#pragma GCC diagnostic push > - > #pragma GCC diagnostic error "-Wuninitialized" > > void > @@ -26,3 +25,5 @@ h (void) > { > CODE_WITH_WARNING; /* { dg-message "in expansion of macro > 'CODE_WITH_WARNING'" } */ > } > + > +/* { dg-regexp {.*some warnings being treated as errors} } */ > diff --git a/gcc/tree-ssa-uninit.cc b/gcc/tree-ssa-uninit.cc > index bde23997a0a..bf2e50511af 100644 > --- a/gcc/tree-ssa-uninit.cc > +++ b/gcc/tree-ssa-uninit.cc > @@ -274,9 +274,6 @@ warn_uninit (opt_code opt, tree t, tree var, gimple > *context, > else if (var_name_str) > location = gimple_location (var_def_stmt); > > - location = linemap_resolve_location (line_table, location, > - LRK_SPELLING_LOCATION, NULL); > - > auto_diagnostic_group d; > gcc_assert (opt == OPT_Wuninitialized || opt == OPT_Wmaybe_uninitialized); > if (var) > @@ -424,10 +421,7 @@ maybe_warn_read_write_only (tree fndecl, gimple *stmt, > tree arg, tree ptr) > && access->mode != access_write_only) > continue; > > - location_t stmtloc > - = linemap_resolve_location (line_table, gimple_location (stmt), > - LRK_SPELLING_LOCATION, NULL); > - > + location_t stmtloc = gimple_location (stmt); > if (!warning_at (stmtloc, OPT_Wmaybe_uninitialized, > "%qE may be used uninitialized", ptr)) > break; > @@ -722,9 +716,7 @@ maybe_warn_operand (ao_ref &ref, gimple *stmt, tree lhs, > tree rhs, > bool warned = false; > /* We didn't find any may-defs so on all paths either > reached function entry or a killing clobber. */ > - location_t location > - = linemap_resolve_location (line_table, gimple_location (stmt), > - LRK_SPELLING_LOCATION, NULL); > + location_t location = gimple_location (stmt); > if (wlims.always_executed) > { > if (warning_at (location, OPT_Wuninitialized,
