diagnostics: Fix virtual location for -Wuninitialized [PR69543]

  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.

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
  

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,