[Bug c++/104702] New: [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[Bug c++/104702] New: [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[dani at danielbertalan dot dev]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

            Bug ID: 104702
           Summary: [12 Regression] False positive -Wunused-value warning
                    with -fno-exceptions
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dani at danielbertalan dot dev
  Target Milestone: ---

I'm getting a false positive "value computed is not used" warning on trunk iff
I specify -fno-exceptions.

Godbolt link: https://godbolt.org/z/h7ssrza8Y

$ cat unused.cpp
struct FlyString {
  FlyString(char const*);
  ~FlyString();
};

struct Array { FlyString __data[1]; };

void frobnicate(Array&);

int main() {
    Array s_reserved_words { "" };
    frobnicate(s_reserved_words);
}

$ g++ -fno-exceptions -Wunused-value unused.cpp
unused.cpp: In function 'int main()':
unused.cpp:11:33: warning: value computed is not used [-Wunused-value]
   11 |     Array s_reserved_words { "" };
      |                                 ^

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
   Target Milestone|---                         |12.0

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org
   Last reconfirmed|                            |2022-02-26
      Known to fail|                            |12.0
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
      Known to work|                            |11.2.1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Started with r12-6328:

    c++: temporary lifetime with array aggr init [PR94041]

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |msebor at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think the problem in this case are primarily the warning-control.cc changes.
In GCC 11, build_vec_init did at the end:
  /* Now make the result have the correct type.  */
  if (TREE_CODE (atype) == ARRAY_TYPE)
    {
      atype = build_pointer_type (atype);
      stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
      stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
      TREE_NO_WARNING (stmt_expr) = 1;
    }
GCC 12 does instead:
  if (TREE_CODE (atype) == ARRAY_TYPE)
    {
      atype = build_pointer_type (atype);
      stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
      stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
      suppress_warning (stmt_expr /* What warning? */);
    }
Clearly, at least one of the intended warnings that should be suppressed is
OPT_Wunused_value.
But as EXPR_LOCATION (stmt_expr) == UNKNOWN_LOCATION here, all suppress_warning
does is set TREE_NO_WARNING bit on it (i.e. like GCC 11 used).
Later on, add_stmt is called on it and does:
              if (!EXPR_HAS_LOCATION (t))
                SET_EXPR_LOCATION (t, input_location);
There was also even before build_vec_init a call to
suppress_warning with some unrelated expression that had the
284032 {file = 0x3f67310 "pr104702.C", line = 11, column = 31, data = 0x0, sysp
= false}
location (with OPT_Wparentheses) which is the input_location now newly set by
add_stmt on the INDIRECT_REF.
Later on, convert_to_void calls
                        && !warning_suppressed_p (expr, OPT_Wunused_value)
on the INDIRECT_REF.  TREE_NO_WARNING bit is set, but at this point it has a
location, so it looks up the location in hash map, finds that OPT_Wparantheses
is the warning to be suppressed there and returns that OPT_Wunused_value isn't
suppressed there, even when we wanted to suppress all warnings on the tree.

So, I think the options are either to analyze everything where suppress_warning
might be called on trees that don't have EXPR_LOCATION set yet but might get it
changed later on, or perhaps change the warning-control.cc implementation for
the cases where it is called on trees with reserved location (grab another bit
next to TREE_NO_WARNING and set it too if we want to suppress some warnings on
reserved location tree, which would then be sticky and would imply suppress all
warnings on this tree from now on).
Or change warning-control.cc not to base anything on location_t's but on the
trees or gimple * pointers instead.  We'd of course need to copy records in the
hash map if we copy a tree or gimple stmt including the
TREE_NO_WARNING/gimple_no_warning bit (and would need to be deletable so that
when some trees/gimple stmts are garbage collected we remove entries from those
hash tables).  In fact, I thought that was the plan for warning control.
Basing it on location_t has beyond the above mentioned problem also a problem
if ever EXPR_LOCATION changes later on (I admit except for UNKNOWN_LOCATION ->
some other location changes that is quite rare).

Also, unless I'm misreading the warning-control.cc code, it seems it is based
on whatever location_t a tree or especially gimple * has, but we encode in
location_t not just the location, but also the BLOCK starting with
gimple-low.cc, and I don't see any stripping of that BLOCK stuff, i.e. no uses
of
LOCATION_LOCUS anywhere.  So again I don't see how it can work well,
suppress_warning before gimple-low.cc sets gimple_no_warning bit and fills in
details for one location, then we change the location_t because we add BLOCK to
it, then some warning pass tests suppress_warning_p and either will not find
the new location_t in there and thus will imply all warnings are suppressed, or
worse suppress_location before gimple-low.cc was for one warning, there is
another suppress_location after it on the same stmt (that will add a record on
the new location_t) and finally suppress_warning_p test on the first warning
(will return false).

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

--- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
The warning mapping needs to be updated whenever a location of a tree or
gimple* changes (gimple_set_block() calls gimple_set_location() which calls
copy_warning() so that part at least should work).  I saw code in the C++ front
end that doesn't do that when working on the warning control stuff but had no
good way to find it all; my expectation was that it would get discovered as
bugs like this one cropped up.

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> I think the problem in this case are primarily the warning-control.cc
> changes.
> In GCC 11, build_vec_init did at the end:
>   /* Now make the result have the correct type.  */
>   if (TREE_CODE (atype) == ARRAY_TYPE)
>     {
>       atype = build_pointer_type (atype);
>       stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
>       stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
>       TREE_NO_WARNING (stmt_expr) = 1;
>     }
> GCC 12 does instead:
>   if (TREE_CODE (atype) == ARRAY_TYPE)
>     {
>       atype = build_pointer_type (atype);
>       stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
>       stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
>       suppress_warning (stmt_expr /* What warning? */);
>     }
> Clearly, at least one of the intended warnings that should be suppressed is
> OPT_Wunused_value.
> But as EXPR_LOCATION (stmt_expr) == UNKNOWN_LOCATION here, all
> suppress_warning
> does is set TREE_NO_WARNING bit on it (i.e. like GCC 11 used).
> Later on, add_stmt is called on it and does:
> 	      if (!EXPR_HAS_LOCATION (t))
> 		SET_EXPR_LOCATION (t, input_location);
> There was also even before build_vec_init a call to
> suppress_warning with some unrelated expression that had the
> 284032 {file = 0x3f67310 "pr104702.C", line = 11, column = 31, data = 0x0,
> sysp = false}
> location (with OPT_Wparentheses) which is the input_location now newly set
> by add_stmt on the INDIRECT_REF.
> Later on, convert_to_void calls
> 	                && !warning_suppressed_p (expr, OPT_Wunused_value)
> on the INDIRECT_REF.  TREE_NO_WARNING bit is set, but at this point it has a
> location, so it looks up the location in hash map, finds that
> OPT_Wparantheses
> is the warning to be suppressed there and returns that OPT_Wunused_value
> isn't suppressed there, even when we wanted to suppress all warnings on the
> tree.

So shouldn't warning_suppressed_p (tree, ...) also honor TREE_NO_WARNING
even if a location is present?  That is,

diff --git a/gcc/warning-control.cc b/gcc/warning-control.cc
index 0cbb4f079fa..d36f3ff6965 100644
--- a/gcc/warning-control.cc
+++ b/gcc/warning-control.cc
@@ -120,10 +120,12 @@ get_nowarn_spec (const gimple *stmt)
 bool
 warning_suppressed_p (const_tree expr, opt_code opt /* = all_warnings */)
 {
-  const nowarn_spec_t *spec = get_nowarn_spec (expr);
+  if (get_no_warning_bit (expr))
+    return true;

+  const nowarn_spec_t *spec = get_nowarn_spec (expr);
   if (!spec)
-    return get_no_warning_bit (expr);
+    return false;

   const nowarn_spec_t optspec (opt);
   bool dis = *spec & optspec;

or, if that's not desired, at least

  gcc_checking_assert (!get_no_warning_bit (expr));

when there's a spec?

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, we could add verification for that in verify_expr_location_1 (either bit
or spec but not both).

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #4)
> (In reply to Jakub Jelinek from comment #2)
> > I think the problem in this case are primarily the warning-control.cc
> > changes.
> > In GCC 11, build_vec_init did at the end:
> >   /* Now make the result have the correct type.  */
> >   if (TREE_CODE (atype) == ARRAY_TYPE)
> >     {
> >       atype = build_pointer_type (atype);
> >       stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
> >       stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
> >       TREE_NO_WARNING (stmt_expr) = 1;
> >     }
> > GCC 12 does instead:
> >   if (TREE_CODE (atype) == ARRAY_TYPE)
> >     {
> >       atype = build_pointer_type (atype);
> >       stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
> >       stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
> >       suppress_warning (stmt_expr /* What warning? */);
> >     }
> > Clearly, at least one of the intended warnings that should be suppressed is
> > OPT_Wunused_value.
> > But as EXPR_LOCATION (stmt_expr) == UNKNOWN_LOCATION here, all
> > suppress_warning
> > does is set TREE_NO_WARNING bit on it (i.e. like GCC 11 used).
> > Later on, add_stmt is called on it and does:
> > 	      if (!EXPR_HAS_LOCATION (t))
> > 		SET_EXPR_LOCATION (t, input_location);
> > There was also even before build_vec_init a call to
> > suppress_warning with some unrelated expression that had the
> > 284032 {file = 0x3f67310 "pr104702.C", line = 11, column = 31, data = 0x0,
> > sysp = false}
> > location (with OPT_Wparentheses) which is the input_location now newly set
> > by add_stmt on the INDIRECT_REF.
> > Later on, convert_to_void calls
> > 	                && !warning_suppressed_p (expr, OPT_Wunused_value)
> > on the INDIRECT_REF.  TREE_NO_WARNING bit is set, but at this point it has a
> > location, so it looks up the location in hash map, finds that
> > OPT_Wparantheses
> > is the warning to be suppressed there and returns that OPT_Wunused_value
> > isn't suppressed there, even when we wanted to suppress all warnings on the
> > tree.
> 
> So shouldn't warning_suppressed_p (tree, ...) also honor TREE_NO_WARNING
> even if a location is present?  That is,
> 
> diff --git a/gcc/warning-control.cc b/gcc/warning-control.cc
> index 0cbb4f079fa..d36f3ff6965 100644
> --- a/gcc/warning-control.cc
> +++ b/gcc/warning-control.cc
> @@ -120,10 +120,12 @@ get_nowarn_spec (const gimple *stmt)
>  bool
>  warning_suppressed_p (const_tree expr, opt_code opt /* = all_warnings */)
>  {
> -  const nowarn_spec_t *spec = get_nowarn_spec (expr);
> +  if (get_no_warning_bit (expr))
> +    return true;
>  
> +  const nowarn_spec_t *spec = get_nowarn_spec (expr);
>    if (!spec)
> -    return get_no_warning_bit (expr);
> +    return false;
>  
>    const nowarn_spec_t optspec (opt);
>    bool dis = *spec & optspec;
> 
> or, if that's not desired, at least
> 
>   gcc_checking_assert (!get_no_warning_bit (expr));
> 
> when there's a spec?

That would basically throw away all the warning-control.cc stuff, get us back
to
the TREE_NO_WARNING/gimple_no_warning behavior.  Because the TREE_NO_WARNING
/gimple_no_warning bits are used as a flag whether at least one warning is
disabled.

Anyway, if we keep moreless the current warning-control.cc behavior, it is
unclear what should the FE call when it does:
      if (!EXPR_HAS_LOCATION (t))
        SET_EXPR_LOCATION (t, input_location);
Shall it do
      if (!EXPR_HAS_LOCATION (t) && input_location != UNKNOWN_LOCATION)
        {
          if (TREE_NO_WARNING (t))
            copy_warning (input_location, UNKNOWN_LOCATION);
          SET_EXPR_LOCATION (t, input_location);
        }
or so or some function that will do roughly that?

The copy_warning in gimple.h looks just wrong to me:
static inline void
gimple_set_location (gimple *g, location_t location)
{
  /* Copy the no-warning data to the statement location.  */
  copy_warning (location, g->location);
  g->location = location;
}
If I have some random statement which doesn't have any warning suppression
for it (i.e. !gimple_no_warning_p) and I set the location for it to some real
location, all of sudden that location will have suppressions of "all warnings"
if it is ever used somewhere else where warnings are suppressed.
Shouldn't that do:
  if (__builtin_expect (g->no_warning, 0))
    copy_warning (location, g->location);
instead?

Shouldn't we have some early return e.g. in the copy_warning (location_t,
location_t) if the locations are the same?

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jason at gcc dot gnu.org

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:cc76c502a761ddaee215bcbd8fe4720e46d3b9dd

commit r12-8024-gcc76c502a761ddaee215bcbd8fe4720e46d3b9dd
Author: Jason Merrill <jason@redhat.com>
Date:   Wed Apr 6 10:59:40 2022 -0400

    c++: -Wunused-value and array init [PR104702]

    Here, because of problems with the new warning-control code and expressions
    that change location, the suppress_warning on the INDIRECT_REF didn't work.
    Those problems still need to be worked out, but it's simple to avoid
needing
    to use suppress_warning in the first place by using a reference instead.

            PR c++/104702

    gcc/cp/ChangeLog:

            * init.cc (build_vec_init): Use a reference for the result.

    gcc/testsuite/ChangeLog:

            * g++.dg/warn/Wunused-19.C: New test.

[Bug c++/104702] [12 Regression] False positive -Wunused-value warning with -fno-exceptions

[jason at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #8 from Jason Merrill <jason at gcc dot gnu.org> ---
Fixed.