public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
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,

      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).