[v2] c, c++: -Wswitch warning on [[maybe_unused]] enumerator [PR105497]

Corrected version that avoids an uninitialized warning:

This PR complains that we emit the "enumeration value not handled in
switch" warning even though the enumerator was marked with the
[[maybe_unused]] attribute.

The first snag was that I couldn't just check TREE_USED, because
the enumerator could have been used earlier in the function, which
doesn't play well with the c_do_switch_warnings warning.  Instead,
I had to check the attributes on the CONST_DECL directly, which led
to the second, and worse, snag: in C we don't have direct access to
the CONST_DECL for the enumerator.  I had to handle that by adding
a new function that extracts the decl from an identifier's binding.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

	PR c++/105497

gcc/c-family/ChangeLog:

	* c-common.h (get_decl_for_identifier): Declare.
	* c-warn.cc (c_do_switch_warnings): Don't warn about unhandled
	enumerator when it was marked as unused.

gcc/c/ChangeLog:

	* c-decl.cc (get_decl_for_identifier): New.

gcc/cp/ChangeLog:

	* tree.cc (get_decl_for_identifier): New.

gcc/testsuite/ChangeLog:

	* c-c++-common/Wswitch-1.c: New test.
	* g++.dg/warn/Wswitch-4.C: New test.
---
 gcc/c-family/c-common.h                |  1 +
 gcc/c-family/c-warn.cc                 | 24 ++++++++++--
 gcc/c/c-decl.cc                        | 10 +++++
 gcc/cp/tree.cc                         |  8 ++++
 gcc/testsuite/c-c++-common/Wswitch-1.c | 29 ++++++++++++++
 gcc/testsuite/g++.dg/warn/Wswitch-4.C  | 52 ++++++++++++++++++++++++++
 6 files changed, 121 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wswitch-1.c
 create mode 100644 gcc/testsuite/g++.dg/warn/Wswitch-4.C


base-commit: 0c723bb4be2a67657828b692997855afcdc5d286

On 5/7/22 18:26, Marek Polacek wrote:
> Corrected version that avoids an uninitialized warning:
> 
> This PR complains that we emit the "enumeration value not handled in
> switch" warning even though the enumerator was marked with the
> [[maybe_unused]] attribute.
> 
> The first snag was that I couldn't just check TREE_USED, because
> the enumerator could have been used earlier in the function, which
> doesn't play well with the c_do_switch_warnings warning.  Instead,
> I had to check the attributes on the CONST_DECL directly, which led
> to the second, and worse, snag: in C we don't have direct access to
> the CONST_DECL for the enumerator.

I wonder if you want to change that instead of working around it?

> I had to handle that by adding
> a new function that extracts the decl from an identifier's binding.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> 	PR c++/105497
> 
> gcc/c-family/ChangeLog:
> 
> 	* c-common.h (get_decl_for_identifier): Declare.
> 	* c-warn.cc (c_do_switch_warnings): Don't warn about unhandled
> 	enumerator when it was marked as unused.
> 
> gcc/c/ChangeLog:
> 
> 	* c-decl.cc (get_decl_for_identifier): New.
> 
> gcc/cp/ChangeLog:
> 
> 	* tree.cc (get_decl_for_identifier): New.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* c-c++-common/Wswitch-1.c: New test.
> 	* g++.dg/warn/Wswitch-4.C: New test.
> ---
>   gcc/c-family/c-common.h                |  1 +
>   gcc/c-family/c-warn.cc                 | 24 ++++++++++--
>   gcc/c/c-decl.cc                        | 10 +++++
>   gcc/cp/tree.cc                         |  8 ++++
>   gcc/testsuite/c-c++-common/Wswitch-1.c | 29 ++++++++++++++
>   gcc/testsuite/g++.dg/warn/Wswitch-4.C  | 52 ++++++++++++++++++++++++++
>   6 files changed, 121 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/c-c++-common/Wswitch-1.c
>   create mode 100644 gcc/testsuite/g++.dg/warn/Wswitch-4.C
> 
> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> index f10be9bd67e..c4221089a18 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -831,6 +831,7 @@ extern tree (*make_fname_decl) (location_t, tree, int);
>   
>   /* In c-decl.cc and cp/tree.cc.  FIXME.  */
>   extern void c_register_addr_space (const char *str, addr_space_t as);
> +extern tree get_decl_for_identifier (tree);
>   
>   /* In c-common.cc.  */
>   extern bool in_late_binary_op;
> diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc
> index cae89294aea..03cbc0541b2 100644
> --- a/gcc/c-family/c-warn.cc
> +++ b/gcc/c-family/c-warn.cc
> @@ -1738,8 +1738,23 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
>     for (chain = TYPE_VALUES (type); chain; chain = TREE_CHAIN (chain))
>       {
>         tree value = TREE_VALUE (chain);
> +      tree id = TREE_PURPOSE (chain);
> +      tree attrs = NULL_TREE;
> +      /* In C++, the TREE_VALUE is a CONST_DECL.  */
>         if (TREE_CODE (value) == CONST_DECL)
> -	value = DECL_INITIAL (value);
> +	{
> +	  attrs = DECL_ATTRIBUTES (value);
> +	  value = DECL_INITIAL (value);
> +	}
> +      /* In C, the TREE_VALUE is an integer constant.  The TREE_PURPOSE is
> +	 an identifier from which we must tease out the CONST_DECL.  */
> +      else if (tree decl = get_decl_for_identifier (id))
> +	attrs = DECL_ATTRIBUTES (decl);
> +      /* Track if the enumerator was marked as unused.  We can't use
> +	 TREE_USED here: it could have been set on the enumerator if
> +	 the enumerator was used earlier.  */
> +      const bool unused_p = (lookup_attribute ("unused", attrs)
> +			     || lookup_attribute ("maybe_unused", attrs));

Why is this calculation...

>         node = splay_tree_lookup (cases, (splay_tree_key) value);
>         if (node)
>   	{
> @@ -1769,6 +1784,10 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
>         /* We've now determined that this enumerated literal isn't
>   	 handled by the case labels of the switch statement.  */
>   
> +      /* Don't warn if the enumerator was marked as unused.  */
> +      if (unused_p)
> +	continue;

...separate from this test?

>         /* If the switch expression is a constant, we only really care
>   	 about whether that constant is handled by the switch.  */
>         if (cond && tree_int_cst_compare (cond, value))
> @@ -1791,8 +1810,7 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
>   		  (default_node || !warn_switch
>   		   ? OPT_Wswitch_enum
>   		   : OPT_Wswitch),
> -		  "enumeration value %qE not handled in switch",
> -		  TREE_PURPOSE (chain));
> +		  "enumeration value %qE not handled in switch", id);
>       }
>   
>     /* Warn if there are case expressions that don't correspond to
> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> index c701f07befe..c2c813d922a 100644
> --- a/gcc/c/c-decl.cc
> +++ b/gcc/c/c-decl.cc
> @@ -12466,4 +12466,14 @@ c_check_in_current_scope (tree decl)
>     return b != NULL && B_IN_CURRENT_SCOPE (b);
>   }
>   
> +/* Returns the symbol associated with an identifier ID.  Currently, this
> +   is used only in c_do_switch_warnings so that we can obtain the CONST_DECL
> +   associated with an enumerator identifier.  */
> +
> +tree
> +get_decl_for_identifier (tree id)
> +{
> +  return I_SYMBOL_DECL (id);
> +}
> +
>   #include "gt-c-c-decl.h"
> diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> index 979728027ed..13852dc6446 100644
> --- a/gcc/cp/tree.cc
> +++ b/gcc/cp/tree.cc
> @@ -6138,6 +6138,14 @@ maybe_adjust_arg_pos_for_attribute (const_tree fndecl)
>     return n > 0 ? n - 1 : 0;
>   }
>   
> +/* Stub for c-common.  Currently this has no use in the C++ front end.  */
> +
> +tree
> +get_decl_for_identifier (tree)
> +{
> +  gcc_unreachable ();
> +}
> +
>   
>   /* Release memory we no longer need after parsing.  */
>   void
> diff --git a/gcc/testsuite/c-c++-common/Wswitch-1.c b/gcc/testsuite/c-c++-common/Wswitch-1.c
> new file mode 100644
> index 00000000000..de9ee03b0a3
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/Wswitch-1.c
> @@ -0,0 +1,29 @@
> +/* PR c++/105497 */
> +/* { dg-options "-Wswitch" } */
> +
> +enum E {
> +  A,
> +  B,
> +  C __attribute((unused)),
> +  D
> +};
> +
> +void
> +g (enum E e)
> +{
> +  switch (e)
> +    {
> +    case A:
> +    case B:
> +    case D:
> +      break;
> +    }
> +
> +  switch (e) // { dg-warning "not handled in switch" }
> +    {
> +    case A:
> +    case B:
> +    case C:
> +      break;
> +    }
> +}
> diff --git a/gcc/testsuite/g++.dg/warn/Wswitch-4.C b/gcc/testsuite/g++.dg/warn/Wswitch-4.C
> new file mode 100644
> index 00000000000..553a57d777b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wswitch-4.C
> @@ -0,0 +1,52 @@
> +// PR c++/105497
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wswitch" }
> +
> +enum class Button
> +{
> +    Left,
> +    Right,
> +    Middle,
> +    NumberOfButtons [[maybe_unused]]
> +};
> +
> +enum class Sound
> +{
> +  Bark,
> +  Meow,
> +  Hiss,
> +  Moo __attribute((unused))
> +};
> +
> +enum class Chordata
> +{
> +  Urochordata,
> +  Cephalochordata,
> +  Vertebrata
> +};
> +
> +int main()
> +{
> +  Button b = Button::Left;
> +  switch (b) { // { dg-bogus "not handled" }
> +        case Button::Left:
> +        case Button::Right:
> +        case Button::Middle:
> +            break;
> +    }
> +
> +  Sound s = Sound::Bark;
> +  switch (s) { // { dg-bogus "not handled" }
> +    case Sound::Bark:
> +    case Sound::Meow:
> +    case Sound::Hiss:
> +      break;
> +  }
> +
> +  Chordata c = Chordata::Vertebrata;
> +  switch (c) { // { dg-warning "not handled" }
> +    case Chordata::Cephalochordata:
> +    case Chordata::Vertebrata:
> +      break;
> +  }
> +}
> 
> base-commit: 0c723bb4be2a67657828b692997855afcdc5d286

On Tue, May 10, 2022 at 08:58:46AM -0400, Jason Merrill wrote:
> On 5/7/22 18:26, Marek Polacek wrote:
> > Corrected version that avoids an uninitialized warning:
> > 
> > This PR complains that we emit the "enumeration value not handled in
> > switch" warning even though the enumerator was marked with the
> > [[maybe_unused]] attribute.
> > 
> > The first snag was that I couldn't just check TREE_USED, because
> > the enumerator could have been used earlier in the function, which
> > doesn't play well with the c_do_switch_warnings warning.  Instead,
> > I had to check the attributes on the CONST_DECL directly, which led
> > to the second, and worse, snag: in C we don't have direct access to
> > the CONST_DECL for the enumerator.
> 
> I wonder if you want to change that instead of working around it?

I wouldn't mind looking into that; I've hit this discrepancy numerous
times throughout the years and it'd be good to unify it so that the
c-common code doesn't need to hack around it.
 
Let's see how far I'll get...

> > +      const bool unused_p = (lookup_attribute ("unused", attrs)
> > +			     || lookup_attribute ("maybe_unused", attrs));
> 
> Why is this calculation...
> 
> >         node = splay_tree_lookup (cases, (splay_tree_key) value);
> >         if (node)
> >   	{
> > @@ -1769,6 +1784,10 @@ c_do_switch_warnings (splay_tree cases, location_t switch_location,
> >         /* We've now determined that this enumerated literal isn't
> >   	 handled by the case labels of the switch statement.  */
> > +      /* Don't warn if the enumerator was marked as unused.  */
> > +      if (unused_p)
> > +	continue;
> 
> ...separate from this test?

Ah, that must be a remnant from a previous version of the patch.  No reason
for the separation anymore.

Thanks,
Marek