[PATCH 1/2] gdb: replace pragmas with DIAGNOSTIC macros

[PATCH 1/2] gdb: replace pragmas with DIAGNOSTIC macros

[Simon Marchi]

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.

Change-Id: Id33ebec3de39bd449409ea0bab59831289ffe82d
---
 gdb/target/waitstatus.c | 6 +++---
 gdb/target/waitstatus.h | 7 ++++---
 include/diagnostics.h   | 8 ++++++++
 3 files changed, 15 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..5ab43a85e9c 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,9 @@
 # define DIAGNOSTIC_IGNORE_FORMAT_NONLITERAL \
   DIAGNOSTIC_IGNORE ("-Wformat-nonliteral")
 
+# define DIAGNOSTIC_ERROR_SWITCH \
+  DIAGNOSTIC_ERROR ("-Wswitch")
+
 #endif
 
 #ifndef DIAGNOSTIC_IGNORE_SELF_MOVE
-- 
2.33.1

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

[Simon Marchi]

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
 
   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);
 }
-- 
2.33.1

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

[Tom de Vries]

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);
>  }
> 

Re: [PATCH 1/2] gdb: replace pragmas with DIAGNOSTIC macros

[Simon Marchi]

On 2021-12-01 19:09, Tom de Vries wrote:
> On 11/23/21 10:14 PM, Simon Marchi via Gdb-patches wrote:
>> 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.
>>
> 
> I think you forgot the default definition
> ...
> #ifndef DIAGNOSTIC_ERROR_SWITCH
> # define DIAGNOSTIC_ERROR_SWITCH
> #endif
> ...
> 
> Otherwise, LGTM.
> 
> Thanks,
> - Tom

Right, fixed locally.

I would just like to get an Ack from a binutils maintainer, if possible,
since this touches the include directory.

Simon

Re: [PATCH 1/2] gdb: replace pragmas with DIAGNOSTIC macros

[Simon Marchi]

On 2021-12-01 20:56, Simon Marchi via Gdb-patches wrote:
> On 2021-12-01 19:09, Tom de Vries wrote:
>> On 11/23/21 10:14 PM, Simon Marchi via Gdb-patches wrote:
>>> 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.
>>>
>>
>> I think you forgot the default definition
>> ...
>> #ifndef DIAGNOSTIC_ERROR_SWITCH
>> # define DIAGNOSTIC_ERROR_SWITCH
>> #endif
>> ...
>>
>> Otherwise, LGTM.
>>
>> Thanks,
>> - Tom
> 
> Right, fixed locally.
> 
> I would just like to get an Ack from a binutils maintainer, if possible,
> since this touches the include directory.
> 
> Simon
> 

Actually, I posted a v2 which obsoletes this patch, see:

https://sourceware.org/pipermail/gdb-patches/2021-December/184043.html

Simon

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

[Simon Marchi]

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

Hmm that was a mistake.

> 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

Yeah, that makes sense, all call sites will end up with this.

Done in v2.

Simon