From: Richard Biener <richard.guenther@gmail.com>
To: Lewis Hyatt <lhyatt@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] diagnostics: Fix virtual location for -Wuninitialized [PR69543]
Date: Fri, 30 Sep 2022 08:10:08 +0200 [thread overview]
Message-ID: <CAFiYyc2Lq14bi3FkFf64p0QW274s91NSX=oJDxTjYdh6ybmNfQ@mail.gmail.com> (raw)
In-Reply-To: <81f46d99de6ed37b7a65914d743d996a3a39ea9f.1664489390.git.lhyatt@gmail.com>
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,
prev parent reply other threads:[~2022-09-30 6:10 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-29 22:10 Lewis Hyatt
2022-09-30 6:10 ` Richard Biener [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFiYyc2Lq14bi3FkFf64p0QW274s91NSX=oJDxTjYdh6ybmNfQ@mail.gmail.com' \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=lhyatt@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).