|
| 1 | +From 4a0b2cb7210c65c8c9f4a56345749bb7294fbfbf Mon Sep 17 00:00:00 2001 |
| 2 | +From: Carlos Galvez <carlosgalvezp@gmail.com> |
| 3 | +Date: Sat, 21 Dec 2024 21:25:40 +0000 |
| 4 | +Subject: [PATCH] Fix -Wenum-constexpr-conversion in enum-flags.h |
| 5 | + |
| 6 | +This fixes PR 31331: |
| 7 | +https://sourceware.org/bugzilla/show_bug.cgi?id=31331 |
| 8 | + |
| 9 | +Currently, enum-flags.h implements enum_underlying_type by casting |
| 10 | +integer -1 to the enum type, checking if it is less than 0, and |
| 11 | +choosing signed/unsigned underlying type based on that. |
| 12 | + |
| 13 | +This is undefined behavior in C++ if the integer falls outside the range |
| 14 | +of the enum, which Clang 21 detects and flags as a constexpr conversion error. |
| 15 | + |
| 16 | +Fix it by using std::underlying_type instead, which is standard and does |
| 17 | +not suffer from this issue. |
| 18 | + |
| 19 | +Signed-off-by: Carlos Galvez <carlosgalvezp@gmail.com> |
| 20 | +--- |
| 21 | + gdb/unittests/enum-flags-selftests.c | 27 --------------------------- |
| 22 | + gdbsupport/enum-flags.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------- |
| 23 | + include/diagnostics.h | 9 --------- |
| 24 | + 3 files changed, 63 insertions(+), 56 deletions(-) |
| 25 | + |
| 26 | +--- a/gdb/unittests/enum-flags-selftests.c |
| 27 | ++++ b/gdb/unittests/enum-flags-selftests.c |
| 28 | +@@ -233,33 +233,6 @@ CHECK_VALID (true, EF, true ? RE () : EF ()) |
| 29 | + CHECK_VALID (true, EF, true ? EF () : RE ()) |
| 30 | + CHECK_VALID (true, EF, true ? RE () : EF ()) |
| 31 | + |
| 32 | +-/* These are valid, but it's not a big deal since you won't be able to |
| 33 | +- assign the resulting integer to an enum or an enum_flags without a |
| 34 | +- cast. |
| 35 | +- |
| 36 | +- The latter two tests are disabled on older GCCs because they |
| 37 | +- incorrectly fail with gcc 4.8 and 4.9 at least. Running the test |
| 38 | +- outside a SFINAE context shows: |
| 39 | +- |
| 40 | +- invalid user-defined conversion from ‘EF’ to ‘RE2’ |
| 41 | +- |
| 42 | +- They've been confirmed to compile/pass with gcc 5.3, gcc 7.1 and |
| 43 | +- clang 3.7. */ |
| 44 | +- |
| 45 | +-CHECK_VALID (true, int, true ? EF () : EF2 ()) |
| 46 | +-CHECK_VALID (true, int, true ? EF2 () : EF ()) |
| 47 | +-CHECK_VALID (true, int, true ? EF () : RE2 ()) |
| 48 | +-CHECK_VALID (true, int, true ? RE2 () : EF ()) |
| 49 | +- |
| 50 | +-/* Same, but with an unsigned enum. */ |
| 51 | +- |
| 52 | +-typedef unsigned int uns; |
| 53 | +- |
| 54 | +-CHECK_VALID (true, uns, true ? EF () : UEF ()) |
| 55 | +-CHECK_VALID (true, uns, true ? UEF () : EF ()) |
| 56 | +-CHECK_VALID (true, uns, true ? EF () : URE ()) |
| 57 | +-CHECK_VALID (true, uns, true ? URE () : EF ()) |
| 58 | +- |
| 59 | + /* Unfortunately this can't work due to the way C++ computes the |
| 60 | + return type of the ternary conditional operator. int isn't |
| 61 | + implicitly convertible to the raw enum type, so the type of the |
| 62 | +--- a/gdbsupport/enum-flags.h |
| 63 | ++++ b/gdbsupport/enum-flags.h |
| 64 | +@@ -78,30 +78,6 @@ |
| 65 | + namespace. The compiler finds the corresponding |
| 66 | + is_enum_flags_enum_type function via ADL. */ |
| 67 | + |
| 68 | +-/* Note that std::underlying_type<enum_type> is not what we want here, |
| 69 | +- since that returns unsigned int even when the enum decays to signed |
| 70 | +- int. */ |
| 71 | +-template<int size, bool sign> class integer_for_size { typedef void type; }; |
| 72 | +-template<> struct integer_for_size<1, 0> { typedef uint8_t type; }; |
| 73 | +-template<> struct integer_for_size<2, 0> { typedef uint16_t type; }; |
| 74 | +-template<> struct integer_for_size<4, 0> { typedef uint32_t type; }; |
| 75 | +-template<> struct integer_for_size<8, 0> { typedef uint64_t type; }; |
| 76 | +-template<> struct integer_for_size<1, 1> { typedef int8_t type; }; |
| 77 | +-template<> struct integer_for_size<2, 1> { typedef int16_t type; }; |
| 78 | +-template<> struct integer_for_size<4, 1> { typedef int32_t type; }; |
| 79 | +-template<> struct integer_for_size<8, 1> { typedef int64_t type; }; |
| 80 | +- |
| 81 | +-template<typename T> |
| 82 | +-struct enum_underlying_type |
| 83 | +-{ |
| 84 | +- DIAGNOSTIC_PUSH |
| 85 | +- DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION |
| 86 | +- typedef typename |
| 87 | +- integer_for_size<sizeof (T), static_cast<bool>(T (-1) < T (0))>::type |
| 88 | +- type; |
| 89 | +- DIAGNOSTIC_POP |
| 90 | +-}; |
| 91 | +- |
| 92 | + namespace enum_flags_detail |
| 93 | + { |
| 94 | + |
| 95 | +@@ -119,11 +95,62 @@ |
| 96 | + /* gdb::Requires trait helpers. */ |
| 97 | + template <typename enum_type> |
| 98 | + using EnumIsUnsigned |
| 99 | +- = std::is_unsigned<typename enum_underlying_type<enum_type>::type>; |
| 100 | ++ = std::is_unsigned<typename std::underlying_type<enum_type>::type>; |
| 101 | ++ |
| 102 | ++/* Helper to detect whether an enum has a fixed underlying type. This can be |
| 103 | ++ achieved by using a scoped enum (in which case the type is "int") or |
| 104 | ++ an explicit underlying type. C-style enums that are unscoped or do not |
| 105 | ++ have an explicit underlying type have an implementation-defined underlying |
| 106 | ++ type. |
| 107 | ++ |
| 108 | ++ https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#5 |
| 109 | ++ |
| 110 | ++ We need this trait in order to ensure that operator~ below does NOT |
| 111 | ++ operate on old-style enums. This is because we apply operator~ on |
| 112 | ++ the value and then cast the result to the enum_type. This is however |
| 113 | ++ Undefined Behavior if the result does not fit in the range of possible |
| 114 | ++ values for the enum. For enums with fixed underlying type, the entire |
| 115 | ++ range of the integer is available. However, for old-style enums, the range |
| 116 | ++ is only the smallest bit-field that can hold all the values of the |
| 117 | ++ enumeration, typically much smaller than the underlying integer: |
| 118 | ++ |
| 119 | ++ https://timsong-cpp.github.io/cppwp/n4659/expr.static.cast#10 |
| 120 | ++ https://timsong-cpp.github.io/cppwp/n4659/dcl.enum#8 |
| 121 | ++ |
| 122 | ++ To implement this, we leverage the fact that, since C++17, enums with |
| 123 | ++ fixed underlying type can be list-initialized from an integer: |
| 124 | ++ https://timsong-cpp.github.io/cppwp/n4659/dcl.init.list#3.7 |
| 125 | ++ |
| 126 | ++ Old-style enums cannot be initialized like that, leading to ill-formed |
| 127 | ++ code. |
| 128 | ++ |
| 129 | ++ We then use this together with SFINAE to create the desired trait. |
| 130 | ++ |
| 131 | ++*/ |
| 132 | ++template <typename enum_type, typename = void> |
| 133 | ++struct EnumHasFixedUnderlyingType : std::false_type |
| 134 | ++{ |
| 135 | ++ static_assert(std::is_enum<enum_type>::value); |
| 136 | ++}; |
| 137 | ++ |
| 138 | ++/* Specialization that is active only if enum_type can be |
| 139 | ++ list-initialized from an integer (0). Only enums with fixed |
| 140 | ++ underlying type satisfy this property in C++17. */ |
| 141 | + template <typename enum_type> |
| 142 | +-using EnumIsSigned |
| 143 | +- = std::is_signed<typename enum_underlying_type<enum_type>::type>; |
| 144 | ++struct EnumHasFixedUnderlyingType<enum_type, std::void_t<decltype(enum_type{0})>> : std::true_type |
| 145 | ++{ |
| 146 | ++ static_assert(std::is_enum<enum_type>::value); |
| 147 | ++}; |
| 148 | + |
| 149 | ++template <typename enum_type> |
| 150 | ++using EnumIsSafeForBitwiseComplement = std::conjunction< |
| 151 | ++ EnumIsUnsigned<enum_type>, |
| 152 | ++ EnumHasFixedUnderlyingType<enum_type> |
| 153 | ++>; |
| 154 | ++ |
| 155 | ++template <typename enum_type> |
| 156 | ++using EnumIsUnsafeForBitwiseComplement = std::negation<EnumIsSafeForBitwiseComplement<enum_type>>; |
| 157 | ++ |
| 158 | + } |
| 159 | + |
| 160 | + template <typename E> |
| 161 | +@@ -131,7 +158,7 @@ |
| 162 | + { |
| 163 | + public: |
| 164 | + typedef E enum_type; |
| 165 | +- typedef typename enum_underlying_type<enum_type>::type underlying_type; |
| 166 | ++ typedef typename std::underlying_type<enum_type>::type underlying_type; |
| 167 | + |
| 168 | + /* For to_string. Maps one enumerator of E to a string. */ |
| 169 | + struct string_mapping |
| 170 | +@@ -394,33 +421,41 @@ |
| 171 | + template <typename enum_type, |
| 172 | + typename = is_enum_flags_enum_type_t<enum_type>, |
| 173 | + typename |
| 174 | +- = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>> |
| 175 | ++ = gdb::Requires<enum_flags_detail::EnumIsSafeForBitwiseComplement<enum_type>>> |
| 176 | + constexpr enum_type |
| 177 | + operator~ (enum_type e) |
| 178 | + { |
| 179 | + using underlying = typename enum_flags<enum_type>::underlying_type; |
| 180 | +- return (enum_type) ~underlying (e); |
| 181 | ++ /* Cast to ULONGEST first, to prevent integer promotions from enums |
| 182 | ++ with fixed underlying type std::uint8_t or std::uint16_t to |
| 183 | ++ signed int. This ensures we apply the bitwise complement on an |
| 184 | ++ unsigned type. */ |
| 185 | ++ return (enum_type)(underlying) ~ULONGEST (e); |
| 186 | + } |
| 187 | + |
| 188 | + template <typename enum_type, |
| 189 | + typename = is_enum_flags_enum_type_t<enum_type>, |
| 190 | +- typename = gdb::Requires<enum_flags_detail::EnumIsSigned<enum_type>>> |
| 191 | ++ typename = gdb::Requires<enum_flags_detail::EnumIsUnsafeForBitwiseComplement<enum_type>>> |
| 192 | + constexpr void operator~ (enum_type e) = delete; |
| 193 | + |
| 194 | + template <typename enum_type, |
| 195 | + typename = is_enum_flags_enum_type_t<enum_type>, |
| 196 | + typename |
| 197 | +- = gdb::Requires<enum_flags_detail::EnumIsUnsigned<enum_type>>> |
| 198 | ++ = gdb::Requires<enum_flags_detail::EnumIsSafeForBitwiseComplement<enum_type>>> |
| 199 | + constexpr enum_flags<enum_type> |
| 200 | + operator~ (enum_flags<enum_type> e) |
| 201 | + { |
| 202 | + using underlying = typename enum_flags<enum_type>::underlying_type; |
| 203 | +- return (enum_type) ~underlying (e); |
| 204 | ++ /* Cast to ULONGEST first, to prevent integer promotions from enums |
| 205 | ++ with fixed underlying type std::uint8_t or std::uint16_t to |
| 206 | ++ signed int. This ensures we apply the bitwise complement on an |
| 207 | ++ unsigned type. */ |
| 208 | ++ return (enum_type)(underlying) ~ULONGEST (e); |
| 209 | + } |
| 210 | + |
| 211 | + template <typename enum_type, |
| 212 | + typename = is_enum_flags_enum_type_t<enum_type>, |
| 213 | +- typename = gdb::Requires<enum_flags_detail::EnumIsSigned<enum_type>>> |
| 214 | ++ typename = gdb::Requires<enum_flags_detail::EnumIsUnsafeForBitwiseComplement<enum_type>>> |
| 215 | + constexpr void operator~ (enum_flags<enum_type> e) = delete; |
| 216 | + |
| 217 | + /* Delete operator<< and operator>>. */ |
| 218 | +--- a/include/diagnostics.h |
| 219 | ++++ b/include/diagnostics.h |
| 220 | +@@ -76,11 +76,6 @@ |
| 221 | + # define DIAGNOSTIC_ERROR_SWITCH \ |
| 222 | + DIAGNOSTIC_ERROR ("-Wswitch") |
| 223 | + |
| 224 | +-# if __has_warning ("-Wenum-constexpr-conversion") |
| 225 | +-# define DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION \ |
| 226 | +- DIAGNOSTIC_IGNORE ("-Wenum-constexpr-conversion") |
| 227 | +-# endif |
| 228 | +- |
| 229 | + #elif defined (__GNUC__) /* GCC */ |
| 230 | + |
| 231 | + # define DIAGNOSTIC_IGNORE_DEPRECATED_DECLARATIONS \ |
| 232 | +@@ -157,10 +152,6 @@ |
| 233 | + |
| 234 | + #ifndef DIAGNOSTIC_ERROR_SWITCH |
| 235 | + # define DIAGNOSTIC_ERROR_SWITCH |
| 236 | +-#endif |
| 237 | +- |
| 238 | +-#ifndef DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION |
| 239 | +-# define DIAGNOSTIC_IGNORE_ENUM_CONSTEXPR_CONVERSION |
| 240 | + #endif |
| 241 | + |
| 242 | + #endif /* DIAGNOSTICS_H */ |
0 commit comments