[PATCH v2] gdb, include: replace pragmas with DIAGNOSTIC macros, fix build with g++ 4.8

[PATCH v2] gdb, include: replace pragmas with DIAGNOSTIC macros, fix build with g++ 4.8

[Simon Marchi]

New in v2:

 - Patches 1 and 2 merged together
 - Add the default case for when DIAGNOSTIC_ERROR_SWITCH is not defined
 - Control whether DIAGNOSTIC_ERROR_SWITCH is defined based on gcc
   version in diagnostic.h

When introducing this code, I forgot that we had some macros for this.
Replace some "manual" pragma diagnostic with some DIAGNOSTIC_* macros,
provided by include/diagnostics.h.

In diagnostics.h:

 - Add DIAGNOSTIC_ERROR, to enable a diagnostic at error level.
 - Add DIAGNOSTIC_ERROR_SWITCH, to enable -Wswitch at error level, for
   both gcc and clang.

Additionally, using DIAGNOSTIC_PUSH, DIAGNOSTIC_ERROR_SWITCH and
DIAGNOSTIC_POP seems to misbehave with g++ 4.8, where we see these
errors:

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

Because of the POP, the diagnostic should go back to being disabled,
since it was disabled in the beginning, but that's now what we see
here.  Versions of GCC >= 5 compile correctly.

Work around this by making DIAGNOSTIC_ERROR_SWITCH a no-op for GCC < 5.

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 and cause a build error if building with future
compilers.  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: Id33ebec3de39bd449409ea0bab59831289ffe82d
---
 gdb/target/waitstatus.c |  6 +++---
 gdb/target/waitstatus.h |  7 ++++---
 include/diagnostics.h   | 16 ++++++++++++++++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
index a7209e3f2b7..0fbcec5b7c8 100644
--- a/gdb/target/waitstatus.c
+++ b/gdb/target/waitstatus.c
@@ -30,8 +30,8 @@ target_waitstatus::to_string () const
 
 /* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
    but not handled here.  */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic error "-Wswitch"
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_ERROR_SWITCH
   switch (this->kind ())
     {
     case TARGET_WAITKIND_EXITED:
@@ -63,7 +63,7 @@ target_waitstatus::to_string () const
     case TARGET_WAITKIND_THREAD_CREATED:
       return str;
     }
-#pragma GCC diagnostic pop
+DIAGNOSTIC_POP
 
   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 48405d222f4..5b537354184 100644
--- a/gdb/target/waitstatus.h
+++ b/gdb/target/waitstatus.h
@@ -20,6 +20,7 @@
 #ifndef TARGET_WAITSTATUS_H
 #define TARGET_WAITSTATUS_H
 
+#include "diagnostics.h"
 #include "gdbsupport/gdb_signals.h"
 
 /* Stuff for target_wait.  */
@@ -108,8 +109,8 @@ target_waitkind_str (target_waitkind kind)
 {
 /* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
    but not handled here.  */
-#pragma GCC diagnostic push
-#pragma GCC diagnostic error "-Wswitch"
+DIAGNOSTIC_PUSH
+DIAGNOSTIC_ERROR_SWITCH
   switch (kind)
   {
     case TARGET_WAITKIND_EXITED:
@@ -145,7 +146,7 @@ target_waitkind_str (target_waitkind kind)
     case TARGET_WAITKIND_THREAD_EXITED:
       return "THREAD_EXITED";
   };
-#pragma GCC diagnostic pop
+DIAGNOSTIC_POP
 
   gdb_assert_not_reached ("invalid target_waitkind value: %d\n", (int) kind);
 }
diff --git a/include/diagnostics.h b/include/diagnostics.h
index f6fd30e26df..7b3f6db9ff3 100644
--- a/include/diagnostics.h
+++ b/include/diagnostics.h
@@ -40,6 +40,8 @@
 
 # define DIAGNOSTIC_IGNORE(option) \
   _Pragma (DIAGNOSTIC_STRINGIFY (GCC diagnostic ignored option))
+# define DIAGNOSTIC_ERROR(option) \
+  _Pragma (DIAGNOSTIC_STRINGIFY (GCC diagnostic error option))
 #else
 # define DIAGNOSTIC_PUSH
 # define DIAGNOSTIC_POP
@@ -61,6 +63,9 @@
 # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL \
   DIAGNOSTIC_IGNORE ("-Wformat-nonliteral")
 
+# define DIAGNOSTIC_ERROR_SWITCH \
+  DIAGNOSTIC_ERROR ("-Wswitch")
+
 #elif defined (__GNUC__) /* GCC */
 
 # if __GNUC__ >= 7
@@ -74,6 +79,13 @@
 # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL \
   DIAGNOSTIC_IGNORE ("-Wformat-nonliteral")
 
+/* GCC 4.8's "diagnostic push/pop" seems broken when using this, -Wswitch
+   remains enabled at the error level even after a pop.  Therefore, don't
+   use it for GCC < 5.  */
+# if __GNUC__ >= 5
+#  define DIAGNOSTIC_ERROR_SWITCH DIAGNOSTIC_ERROR ("-Wswitch")
+# endif
+
 #endif
 
 #ifndef DIAGNOSTIC_IGNORE_SELF_MOVE
@@ -100,4 +112,8 @@
 # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
 #endif
 
+#ifndef DIAGNOSTIC_ERROR_SWITCH
+# define DIAGNOSTIC_ERROR_SWITCH
+#endif
+
 #endif /* DIAGNOSTICS_H */
-- 
2.34.1

Re: [PATCH v2] gdb, include: replace pragmas with DIAGNOSTIC macros, fix build with g++ 4.8

[Nick Clifton]

Hi Simon,

>   include/diagnostics.h   | 16 ++++++++++++++++

I have no objections to these changes from a binutils point of view.

Cheers
   Nick

Re: [PATCH v2] gdb, include: replace pragmas with DIAGNOSTIC macros, fix build with g++ 4.8

[Tom de Vries]

On 12/2/21 3:43 AM, Simon Marchi via Gdb-patches wrote:
> New in v2:
> 
>  - Patches 1 and 2 merged together
>  - Add the default case for when DIAGNOSTIC_ERROR_SWITCH is not defined
>  - Control whether DIAGNOSTIC_ERROR_SWITCH is defined based on gcc
>    version in diagnostic.h
> 
> When introducing this code, I forgot that we had some macros for this.
> Replace some "manual" pragma diagnostic with some DIAGNOSTIC_* macros,
> provided by include/diagnostics.h.
> 
> In diagnostics.h:
> 
>  - Add DIAGNOSTIC_ERROR, to enable a diagnostic at error level.
>  - Add DIAGNOSTIC_ERROR_SWITCH, to enable -Wswitch at error level, for
>    both gcc and clang.
> 
> Additionally, using DIAGNOSTIC_PUSH, DIAGNOSTIC_ERROR_SWITCH and
> DIAGNOSTIC_POP seems to misbehave with g++ 4.8, where we see these
> errors:
> 
>       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)
>               ^
> 
> Because of the POP, the diagnostic should go back to being disabled,
> since it was disabled in the beginning, but that's now what we see

now -> not typo

Otherwise, LGTM.

Thanks,
- Tom

> here.  Versions of GCC >= 5 compile correctly.
> 
> Work around this by making DIAGNOSTIC_ERROR_SWITCH a no-op for GCC < 5.
> 
> 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 and cause a build error if building with future
> compilers.  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: Id33ebec3de39bd449409ea0bab59831289ffe82d
> ---
>  gdb/target/waitstatus.c |  6 +++---
>  gdb/target/waitstatus.h |  7 ++++---
>  include/diagnostics.h   | 16 ++++++++++++++++
>  3 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/target/waitstatus.c b/gdb/target/waitstatus.c
> index a7209e3f2b7..0fbcec5b7c8 100644
> --- a/gdb/target/waitstatus.c
> +++ b/gdb/target/waitstatus.c
> @@ -30,8 +30,8 @@ target_waitstatus::to_string () const
>  
>  /* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
>     but not handled here.  */
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic error "-Wswitch"
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_ERROR_SWITCH
>    switch (this->kind ())
>      {
>      case TARGET_WAITKIND_EXITED:
> @@ -63,7 +63,7 @@ target_waitstatus::to_string () const
>      case TARGET_WAITKIND_THREAD_CREATED:
>        return str;
>      }
> -#pragma GCC diagnostic pop
> +DIAGNOSTIC_POP
>  
>    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 48405d222f4..5b537354184 100644
> --- a/gdb/target/waitstatus.h
> +++ b/gdb/target/waitstatus.h
> @@ -20,6 +20,7 @@
>  #ifndef TARGET_WAITSTATUS_H
>  #define TARGET_WAITSTATUS_H
>  
> +#include "diagnostics.h"
>  #include "gdbsupport/gdb_signals.h"
>  
>  /* Stuff for target_wait.  */
> @@ -108,8 +109,8 @@ target_waitkind_str (target_waitkind kind)
>  {
>  /* Make sure the compiler warns if a new TARGET_WAITKIND enumerator is added
>     but not handled here.  */
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic error "-Wswitch"
> +DIAGNOSTIC_PUSH
> +DIAGNOSTIC_ERROR_SWITCH
>    switch (kind)
>    {
>      case TARGET_WAITKIND_EXITED:
> @@ -145,7 +146,7 @@ target_waitkind_str (target_waitkind kind)
>      case TARGET_WAITKIND_THREAD_EXITED:
>        return "THREAD_EXITED";
>    };
> -#pragma GCC diagnostic pop
> +DIAGNOSTIC_POP
>  
>    gdb_assert_not_reached ("invalid target_waitkind value: %d\n", (int) kind);
>  }
> diff --git a/include/diagnostics.h b/include/diagnostics.h
> index f6fd30e26df..7b3f6db9ff3 100644
> --- a/include/diagnostics.h
> +++ b/include/diagnostics.h
> @@ -40,6 +40,8 @@
>  
>  # define DIAGNOSTIC_IGNORE(option) \
>    _Pragma (DIAGNOSTIC_STRINGIFY (GCC diagnostic ignored option))
> +# define DIAGNOSTIC_ERROR(option) \
> +  _Pragma (DIAGNOSTIC_STRINGIFY (GCC diagnostic error option))
>  #else
>  # define DIAGNOSTIC_PUSH
>  # define DIAGNOSTIC_POP
> @@ -61,6 +63,9 @@
>  # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL \
>    DIAGNOSTIC_IGNORE ("-Wformat-nonliteral")
>  
> +# define DIAGNOSTIC_ERROR_SWITCH \
> +  DIAGNOSTIC_ERROR ("-Wswitch")
> +
>  #elif defined (__GNUC__) /* GCC */
>  
>  # if __GNUC__ >= 7
> @@ -74,6 +79,13 @@
>  # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL \
>    DIAGNOSTIC_IGNORE ("-Wformat-nonliteral")
>  
> +/* GCC 4.8's "diagnostic push/pop" seems broken when using this, -Wswitch
> +   remains enabled at the error level even after a pop.  Therefore, don't
> +   use it for GCC < 5.  */
> +# if __GNUC__ >= 5
> +#  define DIAGNOSTIC_ERROR_SWITCH DIAGNOSTIC_ERROR ("-Wswitch")
> +# endif
> +
>  #endif
>  
>  #ifndef DIAGNOSTIC_IGNORE_SELF_MOVE
> @@ -100,4 +112,8 @@
>  # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL
>  #endif
>  
> +#ifndef DIAGNOSTIC_ERROR_SWITCH
> +# define DIAGNOSTIC_ERROR_SWITCH
> +#endif
> +
>  #endif /* DIAGNOSTICS_H */
> 

Re: [PATCH v2] gdb, include: replace pragmas with DIAGNOSTIC macros, fix build with g++ 4.8

[Simon Marchi]

>> Because of the POP, the diagnostic should go back to being disabled,
>> since it was disabled in the beginning, but that's now what we see
> 
> now -> not typo
> 
> Otherwise, LGTM.

Fixed and pushed, thanks.

Simon

Re: [PATCH v2] gdb, include: replace pragmas with DIAGNOSTIC macros, fix build with g++ 4.8

[Simon Marchi]

On 2021-12-02 08:00, Nick Clifton via Gdb-patches wrote:
> Hi Simon,
> 
>>   include/diagnostics.h   | 16 ++++++++++++++++
> 
> I have no objections to these changes from a binutils point of view.
> 
> Cheers
>   Nick
> 

Thanks, pushed.

Simon