Re: [PATCH 2/2] include, gdb: fix -Wswitch build errors with gcc 4.8

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 11/23/21 10:14 PM, Simon Marchi via Gdb-patches wrote:
> I started seeing these strange errors when building with gcc 4.8:
> 
>       CXX    ada-tasks.o
>     /home/smarchi/src/binutils-gdb/gdb/ada-tasks.c: In function ‘void read_known_tasks()’:
>     /home/smarchi/src/binutils-gdb/gdb/ada-tasks.c:998:10: error: enumeration value ‘ADA_TASKS_UNKNOWN’ not handled in switch [-Werror=switch]
>        switch (data->known_tasks_kind)
>               ^
> 
> It is caused by commit 06de25b7af21 ("gdb: introduce
> target_waitkind_str, use it in target_waitstatus::to_string"), which
> introduced some pragmas to enable -Wswitch locally.  That shouldn't
> affect the rest of the code, since we are using push and pop around
> that.  It looks like gcc 4.8 (and 4.9)'s diagnostic push/pop is not
> reliable, as it doesn't cause a problem with later versions.
> 
> Work around this by not enabling -Wswitch for GCC < 5.  This makes the
> code a bit ugly, it would be nice to find a good way to factor this out,
> especially if we want to re-use it elsewhere.  But for now, I just want
> to un-break the build.
> 
> Note that this code (already as it exists in master today) enables
> -Wswitch at the error level even if --disable-werror is passed.  It
> shouldn't be a problem, as it's not like a new enumerator will appear
> out of nowhere.  So it shouldn't cause a spurious build error in the
> future.  Still, for correctness, we would ideally want to ask the
> compiler to enable -Wswitch at its default level (as if the user had
> passed -Wswitch on the command-line).  There doesn't seem to be a way to
> do this.
> 
> Change-Id: I50d66b348bf83de099c3c0879e2eb352d60540ba
> ---
>  gdb/target/waitstatus.c | 9 ++++++++-
>  gdb/target/waitstatus.h | 9 ++++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
> index 0fbcec5b7c8..e2388aa80b8 100644
> --- a/gdb/target/waitstatus.c
> +++ b/gdb/target/waitstatus.c
> @@ -29,9 +29,14 @@ target_waitstatus::to_string () const
>      ("status->kind = %s", target_waitkind_str (this->kind ()));
>  
>  /* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
> -   but not handled here.  */
> +   but not handled here.
> +
> +   GCC 4.8's "diagnostic push/pop" seems broken, it leaves -Werror=switch
> +   enabled after the pop.  Skip it for GCC < 5.  */
>  DIAGNOSTIC_PUSH
> +#if (defined(__GNUC__) && __GNUC__ >= 5) || defined(__clang__)
>  DIAGNOSTIC_ERROR_SWITCH
> +#endif
>    switch (this->kind ())
>      {
>      case TARGET_WAITKIND_EXITED:
> @@ -63,7 +68,9 @@ DIAGNOSTIC_ERROR_SWITCH
>      case TARGET_WAITKIND_THREAD_CREATED:
>        return str;
>      }
> +#if (defined(__GNUC__) && __GNUC__ >= 5) || defined(__clang__)
>  DIAGNOSTIC_POP
> +#endif
>  

It looks a bit funny to do the push unconditionally, and the pop
conditionally.

Is this not better fixed at the def site, once, rather than at two use
sites?  How about this instead:
...
diff --git a/include/diagnostics.h b/include/diagnostics.h
index 5ab43a85e9c..43f0458bd8c 100644
--- a/include/diagnostics.h
+++ b/include/diagnostics.h
@@ -79,8 +79,10 @@
 # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL \
   DIAGNOSTIC_IGNORE ("-Wformat-nonliteral")

+#if __GNUC__ >= 5
 # define DIAGNOSTIC_ERROR_SWITCH \
   DIAGNOSTIC_ERROR ("-Wswitch")
+#endif

 #endif

...
?

Thanks,
- Tom


>    gdb_assert_not_reached ("invalid target_waitkind value: %d",
>  			  (int) this->kind ());
> diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
> index 5b537354184..59014a37362 100644
> --- a/gdb/target/waitstatus.h
> +++ b/gdb/target/waitstatus.h
> @@ -108,9 +108,14 @@ static inline const char *
>  target_waitkind_str (target_waitkind kind)
>  {
>  /* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
> -   but not handled here.  */
> +   but not handled here.
> +
> +   GCC 4.8's "diagnostic push/pop" seems broken, it leaves -Werror=switch
> +   enabled after the pop.  Skip it for GCC < 5.  */
>  DIAGNOSTIC_PUSH
> +#if (defined(__GNUC__) && __GNUC__ >= 5) || defined(__clang__)
>  DIAGNOSTIC_ERROR_SWITCH
> +#endif
>    switch (kind)
>    {
>      case TARGET_WAITKIND_EXITED:
> @@ -146,7 +151,9 @@ DIAGNOSTIC_ERROR_SWITCH
>      case TARGET_WAITKIND_THREAD_EXITED:
>        return "THREAD_EXITED";
>    };
> +#if (defined(__GNUC__) && __GNUC__ >= 5) || defined(__clang__)
>  DIAGNOSTIC_POP
> +#endif
>  
>    gdb_assert_not_reached ("invalid target_waitkind value: %d\n", (int) kind);
>  }
>