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

[Carlos Galvez <carlosgalvezp@gmail.com>]

[-- 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);
>> > +};
>> > +
>>
>>