[PATCH] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h

[PATCH] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h

[Carlos Galvez]

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 to detect
cases where a C-style enum (unscoped, without underlying type) would decay
(be promoted) to a signed integer.

The same can be achieved by explicitly forcing an integer promotion on the
enum, and evaluating its sign.

This approach is more generic and it actually uncovers existing issues, where
enums with unsigned small integer underlying types (e.g. unsigned char) are
also promoted to signed integers, for example:

  flags = flags & ~IS_PARENT_DEFERRED;

Where IS_PARENT_DEFERRED belongs to the following enum:

  enum enum cooked_index_flag_enum : unsigned char
  {
     ...
     IS_PARENT_DEFERRED = 16,
  };

Here, the enum is first promoted to signed int before applying operator~ on it,
which is what we want to avoid. These should be fixed in a follow-up patch, to
keep this one small and focused.

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.
---
 gdbsupport/enum-flags.h | 126 ++++++++++++++++++++++++++++++++++++++--
 include/diagnostics.h   |   5 --
 2 files changed, 120 insertions(+), 11 deletions(-)

diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index 50780043477..c6e0401b9b8 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -77,7 +77,56 @@
 
 /* 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.  */
+   int.
+
+   More specifically, we have the following use cases:
+
+   # 1
+   ---
+   We want enum_flags to behave in the same way as a raw enum
+   in the following context, which is pervasive in the codebase:
+
+   auto x = true ? Enum1() : Enum2();
+
+   Where Enum1 and Enum2 are C-style enums, i.e. unscoped and without
+   fixed underlying type. The output "x" will be of type "int" in this
+   case, even if std::underlying_type would yield "unsigned int" for
+   either enum.
+
+   The rationale for it is that Enum1 and Enum2 are different enum
+   types, so they must be converted to a common type. In order to achieve
+   this, "usual arithmethic conversions" are performed:
+
+   https://timsong-cpp.github.io/cppwp/n4140/expr.cond#6.2
+
+   Following that, "integral promotions" are performed on the operands:
+
+   https://timsong-cpp.github.io/cppwp/n4140/expr#10.5
+
+   Finally, unscoped enums without underlying type are converted to "int"
+   as first choice, if all the values fit in it:
+
+   https://timsong-cpp.github.io/cppwp/n4140/conv.prom#3
+
+   # 2
+   ---
+   We want to avoid applying unary operator~ on signed integer types,
+   since it can lead to implementation-defined or undefined behavior.
+
+   One example is:
+
+   flags = flags & ~ENUM_VALUE;
+
+   If ENUM_VALUE belongs to a C-style enum, integral promotions are
+   applied _before_ computing operator~:
+
+   https://timsong-cpp.github.io/cppwp/n4140/expr.unary.op#10
+
+   As mentioned previously, integral promotions will lead to a signed int
+   as first choice. operator~ would then be applied on a signed int,
+   which we want to avoid.
+
+   */
 template<int size, bool sign> 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; };
@@ -88,15 +137,80 @@ 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; };
 
+/* Helper struct to determine whether an enum will be promoted to a
+   signed integer type when performing "usual arithmetic conversions".
+
+   "Usual arithmetic conversions" can happen in different contexts,
+   for example when using additive operators, so let's use that here.
+
+   https://timsong-cpp.github.io/cppwp/n4140/expr#add-1
+
+*/
+template<typename T>
+struct enum_will_be_promoted_to_signed_integer
+{
+  static_assert(std::is_enum<T>::value, "T must be an enum");
+  static constexpr bool value = std::is_signed<decltype(T()+T())>::value;
+};
+
+/* Helper struct to determine whether an enum has a small integer
+   as underlying type.
+*/
+template <typename T>
+struct enum_has_underlying_small_integer
+{
+  static_assert(std::is_enum<T>::value, "T must be an enum");
+  static constexpr bool value = sizeof(T) < sizeof(int);
+};
+
+/* Helper struct to determine which sign we want our custom
+   "enum_underlying_type" to have. We need to consider two aspects:
+
+   - The sign of the underlying type.
+   - The sign of the integer which the enum would be promoted into,
+     for example when used in arithmetic contexts.
+
+   Note: currently, we need to add an extra condition:
+   "!enum_has_underlying_small_integer".
+
+   This is because we have the following code on trunk:
+
+   flags = flags & ~IS_PARENT_DEFERRED;
+
+   Where IS_PARENT_DEFERRED belongs to a enum like:
+
+   enum enum cooked_index_flag_enum : unsigned char
+   {
+      ...
+      IS_PARENT_DEFERRED = 16,
+   };
+
+   Here, IS_PARENT_DEFERRED will _also_ be promoted to signed integer,
+   due to integral promotions on small integers:
+
+   https://timsong-cpp.github.io/cppwp/n4140/conv.prom#1
+
+   This is undesirable, because operator~ will be applied on a signed
+   integer, which is what we are trying to avoid.
+   Future work should remove this condition and fix the relevant code.
+*/
+template <typename T>
+struct enum_sign
+{
+  static_assert(std::is_enum<T>::value, "T must be an enum");
+  static constexpr bool value =
+    std::is_signed<typename std::underlying_type<T>::type>::value ||
+    (enum_will_be_promoted_to_signed_integer<T>::value &&
+    /* TODO: remove the condition below and fix affected code */
+    !enum_has_underlying_small_integer<T>::value);
+};
+
 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
+  integer_for_size<sizeof (T), enum_sign<T>::value>::type
+  type;
 };
 
 namespace enum_flags_detail
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

[PING][PATCH] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h

[Carlos Galvez]

[-- Attachment #1: Type: text/plain, Size: 8338 bytes --]

Friendly ping.

On Fri, 31 May 2024, 20:16 Carlos Galvez, <carlosgalvezp@gmail.com> 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 to
> detect
> cases where a C-style enum (unscoped, without underlying type) would decay
> (be promoted) to a signed integer.
>
> The same can be achieved by explicitly forcing an integer promotion on the
> enum, and evaluating its sign.
>
> This approach is more generic and it actually uncovers existing issues,
> where
> enums with unsigned small integer underlying types (e.g. unsigned char) are
> also promoted to signed integers, for example:
>
>   flags = flags & ~IS_PARENT_DEFERRED;
>
> Where IS_PARENT_DEFERRED belongs to the following enum:
>
>   enum enum cooked_index_flag_enum : unsigned char
>   {
>      ...
>      IS_PARENT_DEFERRED = 16,
>   };
>
> Here, the enum is first promoted to signed int before applying operator~
> on it,
> which is what we want to avoid. These should be fixed in a follow-up
> patch, to
> keep this one small and focused.
>
> 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.
> ---
>  gdbsupport/enum-flags.h | 126 ++++++++++++++++++++++++++++++++++++++--
>  include/diagnostics.h   |   5 --
>  2 files changed, 120 insertions(+), 11 deletions(-)
>
> diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
> index 50780043477..c6e0401b9b8 100644
> --- a/gdbsupport/enum-flags.h
> +++ b/gdbsupport/enum-flags.h
> @@ -77,7 +77,56 @@
>
>  /* 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.  */
> +   int.
> +
> +   More specifically, we have the following use cases:
> +
> +   # 1
> +   ---
> +   We want enum_flags to behave in the same way as a raw enum
> +   in the following context, which is pervasive in the codebase:
> +
> +   auto x = true ? Enum1() : Enum2();
> +
> +   Where Enum1 and Enum2 are C-style enums, i.e. unscoped and without
> +   fixed underlying type. The output "x" will be of type "int" in this
> +   case, even if std::underlying_type would yield "unsigned int" for
> +   either enum.
> +
> +   The rationale for it is that Enum1 and Enum2 are different enum
> +   types, so they must be converted to a common type. In order to achieve
> +   this, "usual arithmethic conversions" are performed:
> +
> +   https://timsong-cpp.github.io/cppwp/n4140/expr.cond#6.2
> +
> +   Following that, "integral promotions" are performed on the operands:
> +
> +   https://timsong-cpp.github.io/cppwp/n4140/expr#10.5
> +
> +   Finally, unscoped enums without underlying type are converted to "int"
> +   as first choice, if all the values fit in it:
> +
> +   https://timsong-cpp.github.io/cppwp/n4140/conv.prom#3
> +
> +   # 2
> +   ---
> +   We want to avoid applying unary operator~ on signed integer types,
> +   since it can lead to implementation-defined or undefined behavior.
> +
> +   One example is:
> +
> +   flags = flags & ~ENUM_VALUE;
> +
> +   If ENUM_VALUE belongs to a C-style enum, integral promotions are
> +   applied _before_ computing operator~:
> +
> +   https://timsong-cpp.github.io/cppwp/n4140/expr.unary.op#10
> +
> +   As mentioned previously, integral promotions will lead to a signed int
> +   as first choice. operator~ would then be applied on a signed int,
> +   which we want to avoid.
> +
> +   */
>  template<int size, bool sign> 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; };
> @@ -88,15 +137,80 @@ 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; };
>
> +/* Helper struct to determine whether an enum will be promoted to a
> +   signed integer type when performing "usual arithmetic conversions".
> +
> +   "Usual arithmetic conversions" can happen in different contexts,
> +   for example when using additive operators, so let's use that here.
> +
> +   https://timsong-cpp.github.io/cppwp/n4140/expr#add-1
> +
> +*/
> +template<typename T>
> +struct enum_will_be_promoted_to_signed_integer
> +{
> +  static_assert(std::is_enum<T>::value, "T must be an enum");
> +  static constexpr bool value = std::is_signed<decltype(T()+T())>::value;
> +};
> +
> +/* Helper struct to determine whether an enum has a small integer
> +   as underlying type.
> +*/
> +template <typename T>
> +struct enum_has_underlying_small_integer
> +{
> +  static_assert(std::is_enum<T>::value, "T must be an enum");
> +  static constexpr bool value = sizeof(T) < sizeof(int);
> +};
> +
> +/* Helper struct to determine which sign we want our custom
> +   "enum_underlying_type" to have. We need to consider two aspects:
> +
> +   - The sign of the underlying type.
> +   - The sign of the integer which the enum would be promoted into,
> +     for example when used in arithmetic contexts.
> +
> +   Note: currently, we need to add an extra condition:
> +   "!enum_has_underlying_small_integer".
> +
> +   This is because we have the following code on trunk:
> +
> +   flags = flags & ~IS_PARENT_DEFERRED;
> +
> +   Where IS_PARENT_DEFERRED belongs to a enum like:
> +
> +   enum enum cooked_index_flag_enum : unsigned char
> +   {
> +      ...
> +      IS_PARENT_DEFERRED = 16,
> +   };
> +
> +   Here, IS_PARENT_DEFERRED will _also_ be promoted to signed integer,
> +   due to integral promotions on small integers:
> +
> +   https://timsong-cpp.github.io/cppwp/n4140/conv.prom#1
> +
> +   This is undesirable, because operator~ will be applied on a signed
> +   integer, which is what we are trying to avoid.
> +   Future work should remove this condition and fix the relevant code.
> +*/
> +template <typename T>
> +struct enum_sign
> +{
> +  static_assert(std::is_enum<T>::value, "T must be an enum");
> +  static constexpr bool value =
> +    std::is_signed<typename std::underlying_type<T>::type>::value ||
> +    (enum_will_be_promoted_to_signed_integer<T>::value &&
> +    /* TODO: remove the condition below and fix affected code */
> +    !enum_has_underlying_small_integer<T>::value);
> +};
> +
>  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
> +  integer_for_size<sizeof (T), enum_sign<T>::value>::type
> +  type;
>  };
>
>  namespace enum_flags_detail
> 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
>
>

Re: [PATCH] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h

[Pedro Alves]

Hi!

Thank you very much for working on this.

On 2024-05-31 19:15, 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 to detect
> cases where a C-style enum (unscoped, without underlying type) would decay
> (be promoted) to a signed integer.
> 
> The same can be achieved by explicitly forcing an integer promotion on the
> enum, and evaluating its sign.
> 
> This approach is more generic and it actually uncovers existing issues, where
> enums with unsigned small integer underlying types (e.g. unsigned char) are
> also promoted to signed integers, for example:
> 
>   flags = flags & ~IS_PARENT_DEFERRED;
> 
> Where IS_PARENT_DEFERRED belongs to the following enum:
> 
>   enum enum cooked_index_flag_enum : unsigned char
>   {
>      ...
>      IS_PARENT_DEFERRED = 16,
>   };
> 

> Here, the enum is first promoted to signed int before applying operator~ on it,
> which is what we want to avoid. These should be fixed in a follow-up patch, to
> keep this one small and focused.

I think I would want to see or at least understand what you are proposing
to fix that.  I'd say that it would be even better if this was a two-patch
series with a second patch to fix that issue.

Thanks for all the standard references.  I wish I had added something like
this originally, as I have to keep rediscovering why the original code
is the way it is whenever this comes up...


> +
> +   Note: currently, we need to add an extra condition:
> +   "!enum_has_underlying_small_integer".
> +
> +   This is because we have the following code on trunk:
> +
> +   flags = flags & ~IS_PARENT_DEFERRED;
> +
> +   Where IS_PARENT_DEFERRED belongs to a enum like:
> +
> +   enum enum cooked_index_flag_enum : unsigned char
> +   {
> +      ...
> +      IS_PARENT_DEFERRED = 16,
> +   };
> +
> +   Here, IS_PARENT_DEFERRED will _also_ be promoted to signed integer,
> +   due to integral promotions on small integers:
> +
> +   https://timsong-cpp.github.io/cppwp/n4140/conv.prom#1
> +
> +   This is undesirable, because operator~ will be applied on a signed
> +   integer, which is what we are trying to avoid.
> +   Future work should remove this condition and fix the relevant code.

This is what I'd like to understand better, and if possible avoid adding
this "future work" commentary.  What are you imagining would be the fixes
to the relevant code like?

Pedro Alves

> +*/
> +template <typename T>
> +struct enum_sign
> +{
> +  static_assert(std::is_enum<T>::value, "T must be an enum");
> +  static constexpr bool value =
> +    std::is_signed<typename std::underlying_type<T>::type>::value ||
> +    (enum_will_be_promoted_to_signed_integer<T>::value &&
> +    /* TODO: remove the condition below and fix affected code */
> +    !enum_has_underlying_small_integer<T>::value);
> +};
> +

Re: [PATCH] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h

[Carlos Galvez]

[-- Attachment #1: Type: text/plain, Size: 6195 bytes --]

Hi!

Thanks for the response. The way forward requires potentially quite some
refactoring and since it's my first ever contribution to the project I
didn't feel confident in attempting it, plus some design discussions are
probably needed. So I wanted to try and leave things as unchanged as
possible, hopefully better documented in case future maintainers come
across it. I also don't feel super confident in the testing since I don't
get deterministic results (even on trunk).

The main reason for putting up the patch is to unblock the Clang team from
turning the warning into a hard error (as required by the Standard).

Regarding "future work", I have some ideas:

*# Idea 1* (quick, more like a workaround)
Do something like:

flags = flags & ~static_cast<uint32_t>(IS_PARENT_DEFERRED);

Then there's no integer promotion. There's only a handful of places where
this would be needed, so it should be doable. But it's extra verbose. It
might also require some changes to operator~, I believe it only accepts
enums (not ints) as input. Perhaps the cast can be put inside operator~
directly, to a sufficiently large type (std::size_t), that should probably
work! I can put up a follow-up patch if wanted.

*# Idea 2* (more work, but more proper solution)

Remove the custom underlying_type altogether, and use std::underlying_type.
For this, we need:

1) Make operator~ safe such that it can handle both signed and unsigned
types. I haven't looked deeply because I haven't fully understood the issue
nor have I found the quote in the Standard about implementation-defined/UB
in this case.

2) Remove these types of requirements:

CHECK_VALID (true,  int,  true ? EF () : EF2 ())

std::underlying_type would return unsigned int, not int.

Do note: using enums of different types with ternary operator like above
will be ill-formed in C++26:
https://eel.is/c++draft/expr.arith.conv#1.
<https://eel.is/c++draft/expr.arith.conv#1.2>3

GCC is already throwing a warning: https://godbolt.org/z/eMv4551aM

So eventually this code pattern will have to be removed from the codebase.
It's hard for me to find out where in the codebase it's used and the impact
of this, but it's good to know.

Best regards,
Carlos

On Fri, 14 Jun 2024 at 20:28, Pedro Alves <pedro@palves.net> wrote:

> Hi!
>
> Thank you very much for working on this.
>
> On 2024-05-31 19:15, 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 to
> detect
> > cases where a C-style enum (unscoped, without underlying type) would
> decay
> > (be promoted) to a signed integer.
> >
> > The same can be achieved by explicitly forcing an integer promotion on
> the
> > enum, and evaluating its sign.
> >
> > This approach is more generic and it actually uncovers existing issues,
> where
> > enums with unsigned small integer underlying types (e.g. unsigned char)
> are
> > also promoted to signed integers, for example:
> >
> >   flags = flags & ~IS_PARENT_DEFERRED;
> >
> > Where IS_PARENT_DEFERRED belongs to the following enum:
> >
> >   enum enum cooked_index_flag_enum : unsigned char
> >   {
> >      ...
> >      IS_PARENT_DEFERRED = 16,
> >   };
> >
>
> > Here, the enum is first promoted to signed int before applying operator~
> on it,
> > which is what we want to avoid. These should be fixed in a follow-up
> patch, to
> > keep this one small and focused.
>
> I think I would want to see or at least understand what you are proposing
> to fix that.  I'd say that it would be even better if this was a two-patch
> series with a second patch to fix that issue.
>
> Thanks for all the standard references.  I wish I had added something like
> this originally, as I have to keep rediscovering why the original code
> is the way it is whenever this comes up...
>
>
> > +
> > +   Note: currently, we need to add an extra condition:
> > +   "!enum_has_underlying_small_integer".
> > +
> > +   This is because we have the following code on trunk:
> > +
> > +   flags = flags & ~IS_PARENT_DEFERRED;
> > +
> > +   Where IS_PARENT_DEFERRED belongs to a enum like:
> > +
> > +   enum enum cooked_index_flag_enum : unsigned char
> > +   {
> > +      ...
> > +      IS_PARENT_DEFERRED = 16,
> > +   };
> > +
> > +   Here, IS_PARENT_DEFERRED will _also_ be promoted to signed integer,
> > +   due to integral promotions on small integers:
> > +
> > +   https://timsong-cpp.github.io/cppwp/n4140/conv.prom#1
> > +
> > +   This is undesirable, because operator~ will be applied on a signed
> > +   integer, which is what we are trying to avoid.
> > +   Future work should remove this condition and fix the relevant code.
>
> This is what I'd like to understand better, and if possible avoid adding
> this "future work" commentary.  What are you imagining would be the fixes
> to the relevant code like?
>
> Pedro Alves
>
> > +*/
> > +template <typename T>
> > +struct enum_sign
> > +{
> > +  static_assert(std::is_enum<T>::value, "T must be an enum");
> > +  static constexpr bool value =
> > +    std::is_signed<typename std::underlying_type<T>::type>::value ||
> > +    (enum_will_be_promoted_to_signed_integer<T>::value &&
> > +    /* TODO: remove the condition below and fix affected code */
> > +    !enum_has_underlying_small_integer<T>::value);
> > +};
> > +
>
>

Re: [PATCH] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h

[Carlos Galvez]

[-- Attachment #1: Type: text/plain, Size: 6823 bytes --]

Friendly ping.

@Pedro Alves I was also wondering if you could clarify this comment in
"enum-flags.h":

/* We require underlying type to be unsigned when using operator~ --
   if it were not unsigned, undefined behavior could result.

I'm not able to find any information about this in the Standard, could you
clarify?

Thanks!

On Sun, 16 Jun 2024 at 20:21, Carlos Galvez <carlosgalvezp@gmail.com> wrote:

> Hi!
>
> Thanks for the response. The way forward requires potentially quite some
> refactoring and since it's my first ever contribution to the project I
> didn't feel confident in attempting it, plus some design discussions are
> probably needed. So I wanted to try and leave things as unchanged as
> possible, hopefully better documented in case future maintainers come
> across it. I also don't feel super confident in the testing since I don't
> get deterministic results (even on trunk).
>
> The main reason for putting up the patch is to unblock the Clang team from
> turning the warning into a hard error (as required by the Standard).
>
> Regarding "future work", I have some ideas:
>
> *# Idea 1* (quick, more like a workaround)
> Do something like:
>
> flags = flags & ~static_cast<uint32_t>(IS_PARENT_DEFERRED);
>
> Then there's no integer promotion. There's only a handful of places where
> this would be needed, so it should be doable. But it's extra verbose. It
> might also require some changes to operator~, I believe it only accepts
> enums (not ints) as input. Perhaps the cast can be put inside operator~
> directly, to a sufficiently large type (std::size_t), that should probably
> work! I can put up a follow-up patch if wanted.
>
> *# Idea 2* (more work, but more proper solution)
>
> Remove the custom underlying_type altogether, and use
> std::underlying_type. For this, we need:
>
> 1) Make operator~ safe such that it can handle both signed and unsigned
> types. I haven't looked deeply because I haven't fully understood the issue
> nor have I found the quote in the Standard about implementation-defined/UB
> in this case.
>
> 2) Remove these types of requirements:
>
> CHECK_VALID (true,  int,  true ? EF () : EF2 ())
>
> std::underlying_type would return unsigned int, not int.
>
> Do note: using enums of different types with ternary operator like above
> will be ill-formed in C++26:
> https://eel.is/c++draft/expr.arith.conv#1.
> <https://eel.is/c++draft/expr.arith.conv#1.2>3
>
> GCC is already throwing a warning: https://godbolt.org/z/eMv4551aM
>
> So eventually this code pattern will have to be removed from the codebase.
> It's hard for me to find out where in the codebase it's used and the impact
> of this, but it's good to know.
>
> Best regards,
> Carlos
>
> On Fri, 14 Jun 2024 at 20:28, Pedro Alves <pedro@palves.net> wrote:
>
>> Hi!
>>
>> Thank you very much for working on this.
>>
>> On 2024-05-31 19:15, 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 to
>> detect
>> > cases where a C-style enum (unscoped, without underlying type) would
>> decay
>> > (be promoted) to a signed integer.
>> >
>> > The same can be achieved by explicitly forcing an integer promotion on
>> the
>> > enum, and evaluating its sign.
>> >
>> > This approach is more generic and it actually uncovers existing issues,
>> where
>> > enums with unsigned small integer underlying types (e.g. unsigned char)
>> are
>> > also promoted to signed integers, for example:
>> >
>> >   flags = flags & ~IS_PARENT_DEFERRED;
>> >
>> > Where IS_PARENT_DEFERRED belongs to the following enum:
>> >
>> >   enum enum cooked_index_flag_enum : unsigned char
>> >   {
>> >      ...
>> >      IS_PARENT_DEFERRED = 16,
>> >   };
>> >
>>
>> > Here, the enum is first promoted to signed int before applying
>> operator~ on it,
>> > which is what we want to avoid. These should be fixed in a follow-up
>> patch, to
>> > keep this one small and focused.
>>
>> I think I would want to see or at least understand what you are proposing
>> to fix that.  I'd say that it would be even better if this was a two-patch
>> series with a second patch to fix that issue.
>>
>> Thanks for all the standard references.  I wish I had added something like
>> this originally, as I have to keep rediscovering why the original code
>> is the way it is whenever this comes up...
>>
>>
>> > +
>> > +   Note: currently, we need to add an extra condition:
>> > +   "!enum_has_underlying_small_integer".
>> > +
>> > +   This is because we have the following code on trunk:
>> > +
>> > +   flags = flags & ~IS_PARENT_DEFERRED;
>> > +
>> > +   Where IS_PARENT_DEFERRED belongs to a enum like:
>> > +
>> > +   enum enum cooked_index_flag_enum : unsigned char
>> > +   {
>> > +      ...
>> > +      IS_PARENT_DEFERRED = 16,
>> > +   };
>> > +
>> > +   Here, IS_PARENT_DEFERRED will _also_ be promoted to signed integer,
>> > +   due to integral promotions on small integers:
>> > +
>> > +   https://timsong-cpp.github.io/cppwp/n4140/conv.prom#1
>> > +
>> > +   This is undesirable, because operator~ will be applied on a signed
>> > +   integer, which is what we are trying to avoid.
>> > +   Future work should remove this condition and fix the relevant code.
>>
>> This is what I'd like to understand better, and if possible avoid adding
>> this "future work" commentary.  What are you imagining would be the fixes
>> to the relevant code like?
>>
>> Pedro Alves
>>
>> > +*/
>> > +template <typename T>
>> > +struct enum_sign
>> > +{
>> > +  static_assert(std::is_enum<T>::value, "T must be an enum");
>> > +  static constexpr bool value =
>> > +    std::is_signed<typename std::underlying_type<T>::type>::value ||
>> > +    (enum_will_be_promoted_to_signed_integer<T>::value &&
>> > +    /* TODO: remove the condition below and fix affected code */
>> > +    !enum_has_underlying_small_integer<T>::value);
>> > +};
>> > +
>>
>>