tree 8980ae7350fcc2ad121d56602f797995326e9bb4
parent 1898ec362a5930c6925d84785b681576acdbcb99
author Carlos Galvez <carlosgalvezp@gmail.com> 1734816340 +0100
committer Tom Tromey <tom@tromey.com> 1734889738 -0700

Fix -Wenum-constexpr-conversion in enum-flags.h

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.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31331
Tested-by: Keith Seitz <keiths@redhat.com>
Approved-By: Tom Tromey <tom@tromey.com>