From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by sourceware.org (Postfix) with ESMTPS id 74A443853829 for ; Fri, 30 Sep 2022 06:10:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 74A443853829 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ej1-x635.google.com with SMTP id r18so6903286eja.11 for ; Thu, 29 Sep 2022 23:10:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=r52OO5haBnXwdhhb+dP/uDyIANIbUPdPw0/QFHFosfQ=; b=DeW+/Ruq7B/j/6xMmVBjbN26KHwT6FmfuFV9rHjfr4qQmpjAfORkirssHL+MgYBlom ntNutXXjETqIU7K46RFPq0CWoLkkIGklei2kxCIAWIljh1o3XiOJh+opHllXvB1k+DjI w6l44Wb38FL2xCKiCS+4rfmmg5WIqYWDqfl1Ldc+kAtGLoNF+dW5ABg+mzxsf43A6Lhf xx8hlSBZXg2gvYkFwvpKJD/NsegvbWJJus7aU+WrrQyH8Ood2YTbPtjRzwW06q/nOGmN oThw3MGScQCGvP6Pt7RO2aw3xIk+J+VLrGNBcdqzX3LF8EjPfFdr+ADyYDWv1GlrXjVt rAUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=r52OO5haBnXwdhhb+dP/uDyIANIbUPdPw0/QFHFosfQ=; b=2GiSRJIF70Q6w/iVS1+U7EWPfbkBDLL15p+pV9Jfrk3hjKzkpzBmxbjIGNUvK9AsaG VstO8ltyDW/aTz/xe4o0f6rqSwshYhJDB1UsiGaiIhpxars1XdGRdzUGhGXYRYqK5o3V e+aNF/OqD2dHGvyXPHNk91Tq1J+s1XPiYVFgJk8SAz3KtDNyWtjvUYZ37uvyy+VUdZgy D7bgYFQZNM+J/kwR51XNaPK0lytKCi6/THKceW8xVr+He9v37ovttXRRVTrjCf4lpHNM YJ2P4PU0TrnfTgJ2Jy8VaabpdP7PzJvkwHWZllpkRp2eYlKyoM/kfexH7wofWbhURrRQ aRdg== X-Gm-Message-State: ACrzQf02OPRX7yGMo6Y269QgRAdF/VgBp7mrlHngzZ76Zo7x0nHeVfxs Ff9Y/Evh+mLvXEU8vp4FzZSdSHcp+0CWUeCQDqU= X-Google-Smtp-Source: AMsMyM646aMpy2TM/wbt1BWxNhSWLjFgtutejJEcoMCX7rq6f92x87Nt+gGoRp4jPUTuYh8+GukyPG02PplqiwfI/xw= X-Received: by 2002:a17:907:7632:b0:76f:f6e7:36cf with SMTP id jy18-20020a170907763200b0076ff6e736cfmr5483836ejc.442.1664518221112; Thu, 29 Sep 2022 23:10:21 -0700 (PDT) MIME-Version: 1.0 References: <81f46d99de6ed37b7a65914d743d996a3a39ea9f.1664489390.git.lhyatt@gmail.com> In-Reply-To: <81f46d99de6ed37b7a65914d743d996a3a39ea9f.1664489390.git.lhyatt@gmail.com> From: Richard Biener Date: Fri, 30 Sep 2022 08:10:08 +0200 Message-ID: Subject: Re: [PATCH] diagnostics: Fix virtual location for -Wuninitialized [PR69543] To: Lewis Hyatt Cc: gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, Sep 30, 2022 at 12:11 AM Lewis Hyatt via Gcc-patches 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,