[binutils-gdb] gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h

[binutils-gdb] gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h

[Simon Marchi]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ae61525fcf456ab395d55c45492a106d1275873a

commit ae61525fcf456ab395d55c45492a106d1275873a
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Thu Feb 23 12:35:40 2023 -0500

    gdbsupport: ignore -Wenum-constexpr-conversion in enum-flags.h
    
    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

Diff:
---
 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 700037f6126..41ac7838f06 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 d3ff27bc008..41e6db65391 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 */