[PATCH 1/2] gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h

[PATCH 1/2] gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h

[Simon Marchi]

When building with clang 16, we get:

      CXX    gdb.o
    In file included from /home/smarchi/src/binutils-gdb/gdb/gdb.c:19:
    In file included from /home/smarchi/src/binutils-gdb/gdb/defs.h:65:
    /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:95:52: error: integer value -1 is outside the valid range of values [0, 15] for this enumeration type [-Wenum-constexpr-conversion]
        integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type
                                                       ^

The error message does not make it clear in the context of which enum
flag this fails (i.e. what is T in this context), but it doesn't really
matter, we have similar warning/errors for many of them, if we let the
build go through.

clang is right that the value -1 is invalid for the enum type we cast -1
to.  However, we do need this expression in order to select an integer
type with the appropriate signedness.  That is, with the same signedness
as the underlying type of the enum.

I first wondered if that was really needed, if we couldn't use
std::underlying_type for that.  It turns out that the comment just above
says:

    /* Note that std::underlying_type<enum_type> is not what we want here,
       since that returns unsigned int even when the enum decays to signed
       int.  */

I was surprised, because std::is_signed<std::underlying_type<enum_type>>
returns the right thing.  So I tried replacing all this with
std::underlying_type, see if that would work.  Doing so causes some
build failures in unittests/enum-flags-selftests.c:

      CXX    unittests/enum-flags-selftests.o
    /home/smarchi/src/binutils-gdb/gdb/unittests/enum-flags-selftests.c:254:1: error: static assertion failed due to requirement 'gdb::is_same<selftests::enum_flags_tests::check_valid_expr254::archetype<enum_flags<s
    elftests::enum_flags_tests::RE>, selftests::enum_flags_tests::RE, enum_flags<selftests::enum_flags_tests::RE2>, selftests::enum_flags_tests::RE2, enum_flags<selftests::enum_flags_tests::URE>, selftests::enum_fla
    gs_tests::URE, int>, selftests::enum_flags_tests::check_valid_expr254::archetype<enum_flags<selftests::enum_flags_tests::RE>, selftests::enum_flags_tests::RE, enum_flags<selftests::enum_flags_tests::RE2>, selfte
    sts::enum_flags_tests::RE2, enum_flags<selftests::enum_flags_tests::URE>, selftests::enum_flags_tests::URE, unsigned int>>::value == true':
    CHECK_VALID (true,  int,  true ? EF () : EF2 ())
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    /home/smarchi/src/binutils-gdb/gdb/unittests/enum-flags-selftests.c:91:3: note: expanded from macro 'CHECK_VALID'
      CHECK_VALID_EXPR_6 (EF, RE, EF2, RE2, UEF, URE, VALID, EXPR_TYPE, EXPR)
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/valid-expr.h:105:3: note: expanded from macro 'CHECK_VALID_EXPR_6'
      CHECK_VALID_EXPR_INT (ESC_PARENS (typename T1, typename T2,           \
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/valid-expr.h:66:3: note: expanded from macro 'CHECK_VALID_EXPR_INT'
      static_assert (gdb::is_detected_exact<archetype<TYPES, EXPR_TYPE>,    \
      ^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is a bit hard to decode, but basically enumerations have the
following funny property that they decay into a signed int, even if
their implicit underlying type is unsigned.  This code:

    enum A {};
    enum B {};

    int main() {
      std::cout << std::is_signed<std::underlying_type<A>::type>::value
                << std::endl;
      std::cout << std::is_signed<std::underlying_type<B>::type>::value
                << std::endl;
      auto result = true ? A() : B();
      std::cout << std::is_signed<decltype(result)>::value << std::endl;
    }

produces:

    0
    0
    1

So, the "CHECK_VALID" above checks that this property works for enum flags the
same way as it would if you were using their underlying enum types.  And
somehow, changing integer_for_size to use std::underlying_type breaks that.

Since the current code does what we want, and I don't see any way of doing it
differently, ignore -Wenum-constexpr-conversion around it.

Change-Id: Ibc82ae7bbdb812102ae3f1dd099fc859dc6f3cc2
---
 gdbsupport/enum-flags.h | 3 +++
 include/diagnostics.h   | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index 700037f61260..41ac7838f060 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -91,9 +91,12 @@ template<> struct integer_for_size<8, 1> { typedef int64_t type; };
 template<typename T>
 struct enum_underlying_type
 {
+  DIAGNOSTIC_PUSH
+  DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION
   typedef typename
     integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type
     type;
+  DIAGNOSTIC_POP
 };
 
 namespace enum_flags_detail
diff --git a/include/diagnostics.h b/include/diagnostics.h
index d3ff27bc0080..41e6db65391f 100644
--- a/include/diagnostics.h
+++ b/include/diagnostics.h
@@ -76,6 +76,11 @@
 # define DIAGNOSTIC_ERROR_SWITCH \
   DIAGNOSTIC_ERROR ("-Wswitch")
 
+# if __has_warning ("-Wenum-constexpr-conversion")
+#  define DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION \
+   DIAGNOSTIC_IGNORE ("-Wenum-constexpr-conversion")
+# endif
+
 #elif defined (__GNUC__) /* GCC */
 
 # define DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS \
@@ -155,4 +160,8 @@
 # define DIAGNOSTIC_ERROR_SWITCH
 #endif
 
+#ifndef DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION
+# define DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION
+#endif
+
 #endif /* DIAGNOSTICS_H */
-- 
2.39.2

[PATCH 2/2] gdb: fix -Wsingle-bit-bitfield-constant-conversion warning in z80-tdep.c

[Simon Marchi]

When building with clang 16, I see:

    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:338:32: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
            info->prologue_type.load_args = 1;
                                          ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:345:36: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.critical = 1;
                                       ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:351:37: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.interrupt = 1;
                                        ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:367:36: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
                  info->prologue_type.fp_sdcc = 1;
                                              ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:375:35: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.fp_sdcc = 1;
                                      ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:380:35: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.fp_sdcc = 1;
                                      ^ ~

Fix that by using "unsigned int" as the bitfield's underlying type.

Change-Id: I3550a0112f993865dc70b18f02ab11bb5012693d
---
 gdb/z80-tdep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/z80-tdep.c b/gdb/z80-tdep.c
index 9dc24df5ed1c..5c66331efcdf 100644
--- a/gdb/z80-tdep.c
+++ b/gdb/z80-tdep.c
@@ -98,11 +98,11 @@ struct z80_unwind_cache
 
   struct
   {
-    int called:1;	/* there is return address on stack */
-    int load_args:1;	/* prologues loads args using POPs */
-    int fp_sdcc:1;	/* prologue saves and adjusts frame pointer IX */
-    int interrupt:1;	/* __interrupt handler */
-    int critical:1;	/* __critical function */
+    unsigned int called : 1;    /* there is return address on stack */
+    unsigned int load_args : 1; /* prologues loads args using POPs */
+    unsigned int fp_sdcc : 1;   /* prologue saves and adjusts frame pointer IX */
+    unsigned int interrupt : 1; /* __interrupt handler */
+    unsigned int critical : 1;  /* __critical function */
   } prologue_type;
 
   /* Table indicating the location of each and every register.  */
-- 
2.39.2

Re: [PATCH 1/2] gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h

[Simon Marchi]

On 2/23/23 12:35, Simon Marchi via Gdb-patches wrote:
> When building with clang 16, we get:

I pushed these 2 patches.

Simon

[PATCH 0/2] Cherry-pick patches to build with Clang 16 to gdb-13-branch

[Simon Marchi]

I would suggest cherry-picking to gdb-13-branch these two patches that
fix some build issues when building with Clang 16.  Some people at AMD
needed them, so it's possible that people elsewhere will face the same
problems.

Simon Marchi (2):
  gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h
  gdb: fix -Wsingle-bit-bitfield-constant-conversion warning in
    z80-tdep.c

 gdb/z80-tdep.c          | 10 +++++-----
 gdbsupport/enum-flags.h |  3 +++
 include/diagnostics.h   |  9 +++++++++
 3 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.40.1

[PATCH 2/2] gdb: fix -Wsingle-bit-bitfield-constant-conversion warning in z80-tdep.c

[Simon Marchi]

When building with clang 16, I see:

    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:338:32: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
            info->prologue_type.load_args = 1;
                                          ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:345:36: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.critical = 1;
                                       ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:351:37: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.interrupt = 1;
                                        ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:367:36: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
                  info->prologue_type.fp_sdcc = 1;
                                              ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:375:35: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.fp_sdcc = 1;
                                      ^ ~
    /home/smarchi/src/binutils-gdb/gdb/z80-tdep.c:380:35: error: implicit truncation from 'int' to a one-bit wide bit-field changes value from 1 to -1 [-Werror,-Wsingle-bit-bitfield-constant-conversion]
          info->prologue_type.fp_sdcc = 1;
                                      ^ ~

Fix that by using "unsigned int" as the bitfield's underlying type.

(cherry picked from commit 07f285934886016ddd82cac99a3873e68b499d5c)

Change-Id: I3550a0112f993865dc70b18f02ab11bb5012693d
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30423
---
 gdb/z80-tdep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/z80-tdep.c b/gdb/z80-tdep.c
index aa296c7169f6..2af6198ff3fc 100644
--- a/gdb/z80-tdep.c
+++ b/gdb/z80-tdep.c
@@ -98,11 +98,11 @@ struct z80_unwind_cache
 
   struct
   {
-    int called:1;	/* there is return address on stack */
-    int load_args:1;	/* prologues loads args using POPs */
-    int fp_sdcc:1;	/* prologue saves and adjusts frame pointer IX */
-    int interrupt:1;	/* __interrupt handler */
-    int critical:1;	/* __critical function */
+    unsigned int called : 1;    /* there is return address on stack */
+    unsigned int load_args : 1; /* prologues loads args using POPs */
+    unsigned int fp_sdcc : 1;   /* prologue saves and adjusts frame pointer IX */
+    unsigned int interrupt : 1; /* __interrupt handler */
+    unsigned int critical : 1;  /* __critical function */
   } prologue_type;
 
   /* Table indicating the location of each and every register.  */
-- 
2.40.1