@Pedro Alves I have reworked the patch to follow a different approach, it feels a lot more solid now! This time there is no need for follow-up work, the patch is self-contained. The only thing I would need confirmation from you guys if it's OK to remove the mentioned tests. On Sun, 30 Jun 2024 at 21:36, Carlos Galvez wrote: > This fixes PR 31331: > https://sourceware.org/bugzilla/show_bug.cgi?id=31331 > > Currently, enum-flags.h is suppressing the warning > -Wenum-constexpr-conversion coming from recent versions of Clang. > This warning is intended to be made a compiler error > (non-downgradeable) in future versions of Clang: > > https://github.com/llvm/llvm-project/issues/59036 > > The rationale is that casting a value of an integral type into an > enumeration is Undefined Behavior if the value does not fit in the > range of values of the enum: > https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1766 > > Undefined Behavior is not allowed in constant expressions, leading to > an ill-formed program. > > In this case, in enum-flags.h, we are casting the value -1 to an enum > of a positive range only, which is UB as per the Standard and thus not > allowed in a constexpr context. > > The purpose of doing this instead of using std::underlying_type is > because, for C-style enums, std::underlying_type typically returns > "unsigned int". However, when operating with it arithmetically, the > enum is promoted to *signed* int, which is what we want to avoid. > > This patch solves this issue as follows: > > * Use std::underlying_type and remove the custom enum_underlying_type. > > * Ensure that operator~ is called always on an unsigned integer. We do > this by casting the input enum into std::size_t, which can fit any > unsigned integer. We have the guarantee that the cast is safe, > because we have checked that the underlying type is unsigned. If the > enum had negative values, the underlying type would be signed. > > This solves the issue with C-style enums, but also solves a hidden > issue: enums with underlying type of std::uint8_t or std::uint16_t are > *also* promoted to signed int. Now they are all explicitly casted > to the largest unsigned int type and operator~ is safe to use. > > * There is one more thing that needs fix. Currently, operator~ is > implemented as follows: > > return (enum_type) ~underlying(e); > > After applying ~underlying(e), the result is a very large value, > which we then cast to "enum_type". This cast is Undefined Behavior > if the large value does not fit in the range of the enum. For > C++ enums (scoped and/or with explicit underlying type), the range > of the enum is the entire range of the underlying type, so the cast > is safe. However, for C-style enums, the range is the smallest > bit-field that can hold all the values of the enumeration. So the > range is a lot smaller and casting a large value to the enum would > invoke Undefined Behavior. > > To solve this problem, we create a new trait > EnumHasFixedUnderlyingType, to ensure operator~ may only be called > on C++-style enums. This behavior is roughly the same as what we > had on trunk, but relying on different properties of the enums. > > * Once this is implemented, the following tests fail to compile: > > CHECK_VALID (true, int, true ? EF () : EF2 ()) > > This is because it expects the enums to be promoted to signed int, > instead of unsigned int (which is the true underlying type). > > I propose to remove these tests altogether, because: > > - The comment nearby say they are not very important. > - Comparing 2 enums of different type like that is strange, relies > on integer promotions and thus hurts readability. As per comments > in the related PR, we likely don't want this type of code in gdb > code anyway, so there's no point in testing it. > - Most importantly, this type of comparison will be ill-formed in > C++26 for regular enums, so enum_flags does not need to emulate > that. > > Since this is the only place where the warning was suppressed, remove > also the corresponding macro in include/diagnostics.h. > > The change has been tested by running the entire gdb test suite > (make check) and comparing the results (testsuite/gdb.sum) against > trunk. No noticeable differences have been observed. > --- > gdb/unittests/enum-flags-selftests.c | 27 ------- > gdbsupport/enum-flags.h | 104 ++++++++++++++++++--------- > include/diagnostics.h | 5 -- > 3 files changed, 70 insertions(+), 66 deletions(-) > > diff --git a/gdb/unittests/enum-flags-selftests.c > b/gdb/unittests/enum-flags-selftests.c > index b55d8c31406..02563e568d1 100644 > --- a/gdb/unittests/enum-flags-selftests.c > +++ b/gdb/unittests/enum-flags-selftests.c > @@ -233,33 +233,6 @@ CHECK_VALID (true, UEF, ~UEF ()) > CHECK_VALID (true, EF, true ? EF () : RE ()) > CHECK_VALID (true, EF, true ? RE () : EF ()) > > -/* These are valid, but it's not a big deal since you won't be able to > - assign the resulting integer to an enum or an enum_flags without a > - cast. > - > - The latter two tests are disabled on older GCCs because they > - incorrectly fail with gcc 4.8 and 4.9 at least. Running the test > - outside a SFINAE context shows: > - > - invalid user-defined conversion from ‘EF’ to ‘RE2’ > - > - They've been confirmed to compile/pass with gcc 5.3, gcc 7.1 and > - clang 3.7. */ > - > -CHECK_VALID (true, int, true ? EF () : EF2 ()) > -CHECK_VALID (true, int, true ? EF2 () : EF ()) > -CHECK_VALID (true, int, true ? EF () : RE2 ()) > -CHECK_VALID (true, int, true ? RE2 () : EF ()) > - > -/* Same, but with an unsigned enum. */ > - > -typedef unsigned int uns; > - > -CHECK_VALID (true, uns, true ? EF () : UEF ()) > -CHECK_VALID (true, uns, true ? UEF () : EF ()) > -CHECK_VALID (true, uns, true ? EF () : URE ()) > -CHECK_VALID (true, uns, true ? URE () : EF ()) > - > /* Unfortunately this can't work due to the way C++ computes the > return type of the ternary conditional operator. int isn't > implicitly convertible to the raw enum type, so the type of the > diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h > index 50780043477..acec2030fca 100644 > --- a/gdbsupport/enum-flags.h > +++ b/gdbsupport/enum-flags.h > @@ -75,30 +75,6 @@ > namespace. The compiler finds the corresponding > is_enum_flags_enum_type function via ADL. */ > > -/* Note that std::underlying_type is not what we want here, > - since that returns unsigned int even when the enum decays to signed > - int. */ > -template class integer_for_size { typedef void type; > }; > -template<> struct integer_for_size<1, 0> { typedef uint8_t type; }; > -template<> struct integer_for_size<2, 0> { typedef uint16_t type; }; > -template<> struct integer_for_size<4, 0> { typedef uint32_t type; }; > -template<> struct integer_for_size<8, 0> { typedef uint64_t type; }; > -template<> struct integer_for_size<1, 1> { typedef int8_t type; }; > -template<> struct integer_for_size<2, 1> { typedef int16_t type; }; > -template<> struct integer_for_size<4, 1> { typedef int32_t type; }; > -template<> struct integer_for_size<8, 1> { typedef int64_t type; }; > - > -template > -struct enum_underlying_type > -{ > - DIAGNOSTIC_PUSH > - DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION > - typedef typename > - integer_for_size(T (-1) < T (0))>::type > - type; > - DIAGNOSTIC_POP > -}; > - > namespace enum_flags_detail > { > > @@ -119,10 +95,62 @@ struct zero_type; > /* gdb::Requires trait helpers. */ > template > using EnumIsUnsigned > - = std::is_unsigned::type>; > + = std::is_unsigned::type>; > + > +/* Helper to detect whether an enum has a fixed underlying type. This can > be > + achieved by using a scoped enum (in which case the type is "int") or > + an explicit underlying type. C-style enums that are unscoped or do not > + have an explicit underlying type have an implementation-defined > underlying > + type. > + > + https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#5 > + > + We need this trait in order to ensure that operator~ below does NOT > + operate on old-style enums. This is because we apply operator~ on > + the value and then cast the result to the enum_type. This is however > + Undefined Behavior if the result does not fit in the range of possible > + values for the enum. For enums with fixed underlying type, the entire > + range of the integer is available. However, for old-style enums, the > range > + is only the smallest bit-field that can hold all the values of the > + enumeration, typically much smaller than the underlying integer: > + > + https://timsong-cpp.github.io/cppwp/n4659/expr.static.cast#10 > + https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#8 > + > + To implement this, we leverage the fact that, since C++17, enums with > + fixed underlying type can be list-initialized from an integer: > + https://timsong-cpp.github.io/cppwp/n4659/dcl.init.list#3.7 > + > + Old-style enums cannot be initialized like that, leading to ill-formed > + code. > + > + We then use this together with SFINAE to create the desired trait. > + > +*/ > +// Primary template > +template > +struct EnumHasFixedUnderlyingType : std::false_type > +{ > + static_assert(std::is_enum::value); > +}; > + > +// Specialization that is active only if enum_type can be list-initialized > +// from an integer (0). Only enums with fixed underlying type satisfy this > +// property in C++17. > +template > +struct EnumHasFixedUnderlyingType std::void_t> : std::true_type > +{ > + static_assert(std::is_enum::value); > +}; > + > +template > +using EnumIsSafeForBitwiseComplement = std::conjunction< > + EnumIsUnsigned, > + EnumHasFixedUnderlyingType > +>; > + > template > -using EnumIsSigned > - = std::is_signed::type>; > +using EnumIsUnsafeForBitwiseComplement = > std::negation>; > > } > > @@ -131,7 +159,7 @@ class enum_flags > { > public: > typedef E enum_type; > - typedef typename enum_underlying_type::type underlying_type; > + typedef typename std::underlying_type::type underlying_type; > > /* For to_string. Maps one enumerator of E to a string. */ > struct string_mapping > @@ -394,33 +422,41 @@ ENUM_FLAGS_GEN_COMP (operator!=, !=) > template typename = is_enum_flags_enum_type_t, > typename > - = gdb::Requires>> > + = > gdb::Requires>> > constexpr enum_type > operator~ (enum_type e) > { > using underlying = typename enum_flags::underlying_type; > - return (enum_type) ~underlying (e); > + // Cast to std::size_t first, to prevent integer promotions from > + // enums with fixed underlying type std::uint8_t or std::uint16_t > + // to signed int. > + // This ensures we apply the bitwise complement on an unsigned type. > + return (enum_type)(underlying) ~std::size_t (e); > } > > template typename = is_enum_flags_enum_type_t, > - typename = > gdb::Requires>> > + typename = > gdb::Requires>> > constexpr void operator~ (enum_type e) = delete; > > template typename = is_enum_flags_enum_type_t, > typename > - = gdb::Requires>> > + = > gdb::Requires>> > constexpr enum_flags > operator~ (enum_flags e) > { > using underlying = typename enum_flags::underlying_type; > - return (enum_type) ~underlying (e); > + // Cast to std::size_t first, to prevent integer promotions from > + // enums with fixed underlying type std::uint8_t or std::uint16_t > + // to signed int. > + // This ensures we apply the bitwise complement on an unsigned type. > + return (enum_type)(underlying) ~std::size_t (e); > } > > template typename = is_enum_flags_enum_type_t, > - typename = > gdb::Requires>> > + typename = > gdb::Requires>> > constexpr void operator~ (enum_flags e) = delete; > > /* Delete operator<< and operator>>. */ > diff --git a/include/diagnostics.h b/include/diagnostics.h > index 97e30ab807f..14575e20e27 100644 > --- a/include/diagnostics.h > +++ b/include/diagnostics.h > @@ -76,11 +76,6 @@ > # 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 \ > -- > 2.34.1 > >