[Bug gdb/31331] New: Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[Bug gdb/31331] New: Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

            Bug ID: 31331
           Summary: Wenum-constexpr-conversion should be fixed, soon
                    treated as a hard error
           Product: gdb
           Version: HEAD
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: gdb
          Assignee: unassigned at sourceware dot org
          Reporter: carlosgalvezp at gmail dot com
  Target Milestone: ---

Hi!

In Clang we are looking to turn the -Wenum-constexpr-conversion warning into a
hard error, since it's UB detected in a constexpr context, which is mandated by
the Standard to be flagged. See discussion:

https://github.com/llvm/llvm-project/issues/59036

We see that binutils-gdb is currently suppressing the warning:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=include/diagnostics.h;h=8cc2b493d2c02ba7dbc5879207746107ad7143a0;hb=refs/heads/master#l81

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdbsupport/enum-flags.h;hb=e58beedf2c8a1e0c78e0f57aeab3934de9416bfc#l95

Could the code be fixed instead? We naturally don't want to break important
packages such as this one so we are currently waiting for fixes.

Thanks!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tromey at sourceware dot org

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
This came from here:

https://sourceware.org/git/?p=binutils-gdb.git;a=commit;f=gdbsupport/enum-flags.h;h=ae61525fcf456ab395d55c45492a106d1275873a

The commit message there says there isn't another way to
implement what we want here, so at least offhand it
doesn't sound possible.

I don't really understand why signed enums are even needed here, though.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #2 from Tom Tromey <tromey at sourceware dot org> ---
I changed enum-flags.h to use std::underlying_type, with the idea
that if there were any errors, I'd simply change the base
enum to use an unsigned type.  (I tried auditing the existing
uses by hand but I lost interest partway through.)

Anyway to my surprise, the only compilation issues were in
the enum-flags unit-tests, in a section that's explicitly
commented as being unimportant.

This is in fact the 'CHECK_VALID' call that's pointed out
in the above-linked commit message.  However, that case tests
something that I think is very weird, code of the form:

auto var = cond ? enum_1 : enum_2;

I find it pretty unlikely that this code appears anywhere in
gdb, or that we'd want it to.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org,
                   |                            |simon.marchi at polymtl dot ca

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #3 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Exactly, I really don't understand the purpose of that part of test - what
requirement from the production code is it enforcing/protecting?

I see a couple issues with the existing code:

- The assumption that C-style enums are signed. This is not true - it's
implementation-defined behavior, and one should typically not rely on that. In
practice, we observe that if a C-style enum has all values positive, the
underlying type is unsigned, otherwise signed.

- As you point out, the expression:

auto var = cond ? enum_1 : enum_2;

Does not make much sense. If these were C++ enums, the code would not compile.
One should either use a std::variant, or explicitly cast the enums to int.

I would thus vote for removing that part from the test (as you mentioned, it's
marked as unimportant). But it would be good to understand why it was added in
the first place. I'm also unsure if we can trust that everything still works
just because it compiles fine (maybe some implicit conversions leading to the
wrong numbers).

I was considering to fiddle with it and run "make check" but I had a couple of
questions:

- How long time is it expected to take, on say 16 cores?
- Will the output log be deterministic even when running in parallel? I read in
the wiki that there are failing tests on trunk, so one needs to diff the test
logs. Diffing the test logs is only possible if they have the same content in
the same order?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[simon.marchi at polymtl dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #4 from Simon Marchi <simon.marchi at polymtl dot ca> ---
(In reply to Tom Tromey from comment #2)
> I changed enum-flags.h to use std::underlying_type, with the idea
> that if there were any errors, I'd simply change the base
> enum to use an unsigned type.  (I tried auditing the existing
> uses by hand but I lost interest partway through.)

I think that the code explicitly avoids using underlying_type on the enum type,
to get around this behavior (I'll paste the code to generate this table at the
end).

Type             is_signed_v<T>      T(-1)  T(0)  T(-1) < T(0)
Implicit         false                  -1     0  true  
ExplicitSigned   true                   -1     0  true  
ExplicitUnsigned false          4294967295     0  false 

That is, an enum that doesn't specific a base type (Implicit above) has
unsigned underlying type, according to std::underlying_type, but it decays to
an integer, it appears to behave like a signed type (perhaps some language guru
can explain why).

The expression "T (-1) < T (0)" in enum-flags.h is ultimately used to defined
the traits type EnumIsUnsigned and EnumIsSigned.  And those are used to enable
or disable the use of operator~ on enum / enum flag types that behave like
signed types.  I guess because operator~ is not well-defined for them?

So by doing the std::underlying_type change, here's the difference.  Without
your change, trying to use operator~ on let's say STEP_OVER_BREAKPOINT would
give:

  CXX    infrun.o
/home/simark/src/binutils-gdb/gdb/infrun.c:1470:12: error: overload resolution
selected deleted operator '~'
  auto x = ~STEP_OVER_BREAKPOINT;
           ^~~~~~~~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/../gdbsupport/enum-flags.h:408:16: note:
candidate function [with enum_type = step_over_what_flag, $1 = void, $2 = void]
has been explicitly deleted
constexpr void operator~ (enum_type e) = delete;
               ^

If I change the enum_underlying_type definition to be:

template<typename T>
struct enum_underlying_type
{
  typedef typename integer_for_size<
    sizeof (T), static_cast<bool> (
                  std::is_signed_v<std::underlying_type_t<T>>)>::type type;
};

... then the use of operator~ is allowed.

I don't know if this complexity is needed in the end, I just wanted to explain
why (if I understand correctly) we have what we have today.

---

#include <fmt/core.h>
#include <sstream>

enum Implicit {
  A1 = 1,
  A2 = 2,
  A3 = 4,
};
enum ExplicitSigned : signed {
  B1 = 1,
  B2 = 2,
  B3 = 4,
};
enum ExplicitUnsigned : unsigned {
  C1 = 1,
  C2 = 2,
  C3 = 4,
};

template <typename T> void printInfosAboutT(const char *typeName) {
  fmt::print("{: <16} ", typeName);
  fmt::print("{: <14} ", std::is_signed_v<std::underlying_type_t<T>>);

  {
    std::ostringstream os;
    os << T(-1);
    fmt::print("{: >10}  ", os.str());
  }

  {
    std::ostringstream os;
    os << T(0);
    fmt::print("{: >4}  ", os.str());
  } 

  fmt::print("{: <5} ", T(-1) < T(0));
  fmt::print("\n");
}

int main() {
  fmt::print("Type             is_signed_v<T>      T(-1)  T(0)  T(-1) <
T(0)\n");
  printInfosAboutT<Implicit>("Implicit");
  printInfosAboutT<ExplicitSigned>("ExplicitSigned");
  printInfosAboutT<ExplicitUnsigned>("ExplicitUnsigned");
}

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #5 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Simon Marchi from comment #4)

> The expression "T (-1) < T (0)" in enum-flags.h is ultimately used to
> defined the traits type EnumIsUnsigned and EnumIsSigned.  And those are used
> to enable or disable the use of operator~ on enum / enum flag types that
> behave like signed types.  I guess because operator~ is not well-defined for
> them?

Yeah, this makes sense to me.
But my real proposal is going to be that we simply require an unsigned
enum type and add a static assert to this effect.

To me this seems fine because, AFAIK, this code is only used for the
case where the base enum is a bunch of flags, and never for something
weird involving negative values.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #6 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Carlos Galvez from comment #3)

> I was considering to fiddle with it and run "make check" but I had a couple
> of questions:
> 
> - How long time is it expected to take, on say 16 cores?

On my kind of underpowered 8 core machine:

cd build/gdb/testsuite
make -j8 check

takes about 20 minutes.

> - Will the output log be deterministic even when running in parallel? I read
> in the wiki that there are failing tests on trunk, so one needs to diff the
> test logs. Diffing the test logs is only possible if they have the same
> content in the same order?

There's a post-check step (in the Makefile) to collect the results from the 
run.  This avoids the ordering problem.

There are some racy tests though and also some that
are compiler-dependent.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #7 from Tom Tromey <tromey at sourceware dot org> ---
Ok, maybe I see a problem with my plan.

The underlying type is implementation-defined (as pointed out
in comment #3).  And, C++ does not provide a way to check
whether the underlying type is explicitly specified.
So, we don't have a very good way to require that the
underlying type be specified -- we can only get lucky
with certain compilers.

We'd have to look for ": unsigned" in review I guess.
That doesn't seem horrible, especially since the fallout
from failure is just breaking the build with some
(presumably not-too-typical) compiler.

Looking through the existing uses of this macro I do see
some questionable ones in the gdb/compile code.
Those can maybe be removed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #8 from Tom Tromey <tromey at sourceware dot org> ---
Looked at gcc_cp_symbol_kind for the first time and :-(

This is a spot where we can't just add ": unsigned" because
it's part of the ABI of the library.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #9 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
> If I change the enum_underlying_type definition to be:

That's exactly what I had in mind to change it to. You mention this change
keeps the current behavior and the operator overload works as expected. Is
there anything else that breaks?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #10 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Created attachment 15351
  --> https://sourceware.org/bugzilla/attachment.cgi?id=15351&action=edit
Diff between "make check" on trunk and with the proposed change

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #11 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
So I applied this change:

$ git diff gdbsupport/
diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index 50780043477..212a51396e8 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -94,7 +94,7 @@ 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
+    integer_for_size<sizeof (T), std::is_signed<typename
std::underlying_type<T>::type>::value>::type
     type;
   DIAGNOSTIC_POP
 };


And leads to the above-attached diff in testsuite/gdb.sum. I'm not sure what to
make of it, but it does seem to reduce the number of unexpected failures?

< # of expected passes          104586
< # of unexpected failures      685
---
> # of expected passes		104601
> # of unexpected failures	680
108988c108993
< # of known failures           99
---
> # of known failures		97

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #12 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
I should perhaps also mention I have commented out lines 241-270 in
enum-flags-selftests.c. Maybe that's why :)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #13 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
If I compare:

- trunk - enum flag tests
- trunk - enum flag tests + change in enum-flags.h

Then I get:

< # of expected passes          104591
< # of unexpected failures      684
---
> # of expected passes		104601
> # of unexpected failures	680

So the change does seem to improve the state of things. What do you think? Is
there additional testing that could be done?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[simon.marchi at polymtl dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #14 from Simon Marchi <simon.marchi at polymtl dot ca> ---
(In reply to Carlos Galvez from comment #13)
> If I compare:
> 
> - trunk - enum flag tests
> - trunk - enum flag tests + change in enum-flags.h
> 
> Then I get:
> 
> < # of expected passes		104591
> < # of unexpected failures	684
> ---
> > # of expected passes		104601
> > # of unexpected failures	680
> 
> So the change does seem to improve the state of things. What do you think?
> Is there additional testing that could be done?

You could mention which are the tests whose result change.  It's possible that
it's some of the unfortunately racy tests.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #15 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
$ diff /tmp/trunk_without_enum_tests.sum /tmp/new.sum | grep -A 1 -E "> K?FAIL"
> FAIL: gdb.base/checkpoint.exp: breakpoint 1 6 one (timeout)
7323c7323
--
> FAIL: gdb.base/checkpoint.exp: step in 6 two
41516,41521c41516,41521
--
> KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=1: inferior 1 exited (prompt) (PRMS: gdb/18749)
105968c105971
--
> FAIL: gdb.threads/signal-command-handle-nopass.exp: step-over no: signal SIGUSR1
108987,108988c108990,108991

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[simon.marchi at polymtl dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #16 from Simon Marchi <simon.marchi at polymtl dot ca> ---
(In reply to Carlos Galvez from comment #9)
> > If I change the enum_underlying_type definition to be:
> 
> That's exactly what I had in mind to change it to. You mention this change
> keeps the current behavior and the operator overload works as expected. Is
> there anything else that breaks?

So, with that change, for the enums that don't specify an underlying type,
enum_flags::underlying_type would go from a signed type to an unsigned one.

I didn't spot it at first, but another spot that uses the underlying type is
the definition of the binary bitwise operators, using this template:

https://gitlab.com/gnutools/binutils-gdb/-/blob/68d3bf7d246321407697aeb036036dae1a99a742/gdbsupport/enum-flags.h#L226-333

  /* Raw enum on both LHS/RHS.  Returns raw enum type.  */              \
  template <typename enum_type,                                         \
            typename = is_enum_flags_enum_type_t<enum_type>>            \
  constexpr enum_type                                                   \
  OPERATOR_OP (enum_type e1, enum_type e2)                              \
  {                                                                     \
    using underlying = typename enum_flags<enum_type>::underlying_type; \
    return (enum_type) (underlying (e1) OP underlying (e2));            \
  }                                                                     \

So we will cast the enum values to an unsigned type rather than a signed type
before applying the operator. I don't really see how things can go wrong with
that in practice.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[simon.marchi at polymtl dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #17 from Simon Marchi <simon.marchi at polymtl dot ca> ---
(In reply to Carlos Galvez from comment #11)
> So I applied this change:
> 
> $ git diff gdbsupport/
> diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
> index 50780043477..212a51396e8 100644
> --- a/gdbsupport/enum-flags.h
> +++ b/gdbsupport/enum-flags.h
> @@ -94,7 +94,7 @@ 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
> +    integer_for_size<sizeof (T), std::is_signed<typename
> std::underlying_type<T>::type>::value>::type
>      type;
>    DIAGNOSTIC_POP
>  };
> 
> 
> And leads to the above-attached diff in testsuite/gdb.sum. I'm not sure what
> to make of it, but it does seem to reduce the number of unexpected failures?
> 
> < # of expected passes		104586
> < # of unexpected failures	685
> ---
> > # of expected passes		104601
> > # of unexpected failures	680
> 108988c108993
> < # of known failures		99
> ---
> > # of known failures		97

For the record, if we choose to go with underlying_type, I think we can get rid
of this integer_for_size thing altogether and use std::underlying_type to
define enum_flags::underlying_type directly.

I pinged Pedro Alves, original author of this code, he'll take a look soon.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[simon.marchi at polymtl dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #18 from Simon Marchi <simon.marchi at polymtl dot ca> ---
(In reply to Carlos Galvez from comment #15)
> $ diff /tmp/trunk_without_enum_tests.sum /tmp/new.sum | grep -A 1 -E ">
> K?FAIL"
> > FAIL: gdb.base/checkpoint.exp: breakpoint 1 6 one (timeout)
> 7323c7323
> --
> > FAIL: gdb.base/checkpoint.exp: step in 6 two
> 41516,41521c41516,41521
> --
> > KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=1: inferior 1 exited (prompt) (PRMS: gdb/18749)
> 105968c105971
> --
> > FAIL: gdb.threads/signal-command-handle-nopass.exp: step-over no: signal SIGUSR1
> 108987,108988c108990,108991

Off-hand, they just look like racy tests randomness (I know, it's bad).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #19 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
@Simon Marchi I am not able to reproduce the issue you mention in comment #4.
With the following change, the entire codebase compiles just fine, including
the problematic example of auto x = ~STEP_OVER_BREAKPOINT.

diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
index 50780043477..21fb4d29abc 100644
--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -94,7 +94,7 @@ 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
+    std::underlying_type<T>::type
     type;
   DIAGNOSTIC_POP
 };

What change did you apply that led to that failure?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #20 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
> perhaps some language guru can explain why

This is integer promotion when performing arithmetic:

https://timsong-cpp.github.io/cppwp/n4140/expr.unary.op#10

The first type towards which the enum is promoted to is "int", as long as the
values in the enum fit inside an int, otherwise unsigned int.

https://timsong-cpp.github.io/cppwp/n4140/conv.prom#3

The same behavior happens when e.g. operating two uint8_t together -> the
compiler first converts each uint8_t to "int", then performs the operation as a
signed integer, and finally the result is cast what the user specifies. This
can cause subtle bugs (bitwise arithmetic is not recommended on signed
integers).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #21 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
(Sorry for the spam :))

Running the test suite against the changes in my comment #19 leads to:

< # of expected passes          104591
< # of unexpected failures      684
---
> # of expected passes		104604
> # of unexpected failures	678


With only 2 new failures (can't tell if flaky or not):

$ diff /tmp/trunk_without_enum_tests.sum /tmp/underlying_type.sum | grep -A 1
-E "> K?FAIL"
> KFAIL: gdb.threads/process-dies-while-handling-bp.exp: non_stop=off: cond_bp_target=1: inferior 1 exited (prompt) (PRMS: gdb/18749)
106415a106419
> FAIL: gdb.threads/step-over-thread-exit.exp: step_over_mode=displaced: non-stop=on: target-non-stop=on: schedlock=off: cmd=continue: ns_stop_all=0: iter 6: continue (timeout)
108987,108988c108991,108992

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[simon.marchi at polymtl dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #22 from Simon Marchi <simon.marchi at polymtl dot ca> ---
(In reply to Carlos Galvez from comment #19)
> @Simon Marchi I am not able to reproduce the issue you mention in comment
> #4. With the following change, the entire codebase compiles just fine,
> including the problematic example of auto x = ~STEP_OVER_BREAKPOINT.
> 
> diff --git a/gdbsupport/enum-flags.h b/gdbsupport/enum-flags.h
> index 50780043477..21fb4d29abc 100644
> --- a/gdbsupport/enum-flags.h
> +++ b/gdbsupport/enum-flags.h
> @@ -94,7 +94,7 @@ 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
> +    std::underlying_type<T>::type
>      type;
>    DIAGNOSTIC_POP
>  };
> 
> What change did you apply that led to that failure?

It's the other way around.  If you add a `auto x = ~STEP_OVER_BREAKPOINT` to
master without any other changes, it doesn't compile.  If you then add your
std::underlying_type change, then it compiles.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #23 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Ah, I see. I would argue that behavior on trunk is incorrect, though. A C-style
enum with only positive values has an underlying type which is unsigned, on
this particular compiler. The platform-agnostic way to determine whether an
enum  is signed or not is to use std::is_signed<std::underlying_type<...>>. 

Thus a trait called "IsEnumUnsigned" should correctly return "true" on this
situation, which is why the code compiles in the example with
STEP_OVER_BREAKPOINT. 

An orthogonal problem is integer promotion, which is what I believe to be the
root of the problem. Instead of letting the enum silently be promoted into an
integer, it would be preferable to perform an explicit cast to an integer
suitable for the arithmetic operation at hand.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #24 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
It we want to keep the current behavior, I believe it would be possible to
create  one more trait like "WillEnumBePromotedToSignedInt" that checks exactly
that, regardless of the underlying type of the enum. See example:

https://godbolt.org/z/38Ev5d1ja

So operator~ would be SFINAE-enabled only if both a) enum is unsigned and b)
enum won't be promoted to signed int.

What do you think?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #25 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Hi! How would you like to proceed?

I found also this suggestion from Aaron Ballman that might also work:

https://reviews.llvm.org/D150226#4342516

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #26 from Tom Tromey <tromey at sourceware dot org> ---
If a C-like enum has an implementation-defined underlying type,
and we use the WillEnumBePromotedToSignedInt approach, won't
that mean that operator~ might be defined for some compilers and
not others?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |15.1

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #27 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
There's 2 orthogonal issues:

- C enums have implementation-defined underlying type.
- C enums get promoted to signed ints when operating arithmetically
(well-defined behavior, specified in the Standard).

If we change the logic of the code from detecting "is enum unsigned" to
detecting "will enum be promoted to signed int" then there shouldn't be any
implementation-defined behavior and the code should behave identically under
all compilers.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #28 from Tom Tromey <tromey at sourceware dot org> ---
Ok, thanks.
I think comment#24 sounds fine then.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #29 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Hmm, applying Aaron's suggestion above, I found what I believe to be a bug.

With this change:

+template <typename T>
+struct enum_decays_to_signed
+{
+  static constexpr bool value = std::is_signed_v<decltype(true ?
std::declval<T>() : std::declval<std::underlying_type_t<T>>())>;
+};
+
 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
+    integer_for_size<sizeof (T), enum_decays_to_signed<T>::value>::type
     type;
   D

I get:

./dwarf2/cooked-index.h:212:22: error: use of deleted function ‘constexpr void
operator~(enum_type) [with enum_type = cooked_index_flag_enum;
<template-parameter-1-2> = void; <template-parameter-1-3> = void]’
  212 |     flags = flags & ~IS_PARENT_DEFERRED;
      |                      ^~~~~~~~~~~~~~~~~~


This is because that enum has an underlying type of "unsigned char":

enum cooked_index_flag_enum : unsigned char


Unsigned char does decay/promote to signed integer when performing arithmetic
on it:

https://godbolt.org/z/Tfv7e3f8Y

So current master is allowing operator~ on an enum of type unsigned char, which
will decay to signed int before applying operator~ (which is what we want to
avoid, since bitwise operations shall be performed on unsigned types only).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #30 from Tom Tromey <tromey at sourceware dot org> ---
IIUC the current state is:

- there's no way to detect whether an enum has an explicit
  underlying type
- we can't use the current approach to detecting if the underlying
  type is signed
- we can't force an underlying type to be unsigned because it
  would break the compile plugin ABI
- but we can't rely on the underlying type of C-like enums to
  be unsigned since that is compiler dependent
- we can't use the promotion detection since that is too broad

It seems to me that there's no need to support a signed
enumeration constant in enum-flags.  It just isn't useful.

So, could all the work be done in an unsigned type that is
big enough, and then if the enum is signed, mask off the
high bit (and assert that no constant with the high bit
set is ever passed in)?

For enums with a signed underlying type this would mean
sacrificing one flag value.  But that seems ok.

For unsigned enums it would all stay the same.

operator~ could just be defined unconditionally.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #31 from Tom Tromey <tromey at sourceware dot org> ---
*** Bug 23099 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #32 from Tom Tromey <tromey at sourceware dot org> ---
AFAICT nobody is working on this and I think we should
simply remove the target milestone.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #33 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
I've been meaning to look into it but had to prioritize other things. I might
be able to work on this in two weeks or so. By all means if someone else wants
to do it feel free to :)

But I can't promise anything so it makes sense to remove the milestone.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Joel Brobecker <brobecker at gnat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|15.1                        |---
                 CC|                            |brobecker at gnat dot com

--- Comment #34 from Joel Brobecker <brobecker at gnat dot com> ---
I will remove the target milestone, as discussed on gdb-patches@. Reason is:
Not actually critical for the release, and no one seems to be actively working
on this.

Note that this doesn't prevent this from being fixed in 15.1 or 15.2. If the
fix makes it before branching, or if the fix gets approval to be backported.
But we're not going to wait for this to be fixed before we either branch or
release.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #35 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
I am back at looking at this with fresh eyes ;)  

Playing a bit with the code I believe these are the 2 main issues at hand:

- operator~ on signed types (discussed here, it seems there could be a good way
forward based on the latest suggestion).

- These unit tests, which I find problematic:

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 ())

I propose to remove these unit tests (see below why). With that I believe we
could make this work.

Rationale
---------

If I understand correctly, the goal with these unit tests is to make
"enum_flags" behave like a regular C enum, in the sense that the expression
"true ? enum1() : enum2()" returns an "int".

The story about why it returns an "int" is very complicated, but essentially
comes from here:

https://eel.is/c++draft/expr.cond#7.2

The "usual arithmetic conversions" are applied on the raw enums.

Now, reading up on arithmetic conversions, I find:

https://en.cppreference.com/w/cpp/language/usual_arithmetic_conversions

> If either operand is of enumeration type, and the other operand is of a different enumeration type or a floating-point type, the expression is ill-formed. (since C++26)

So, this seems to indicate that the above expression will be ill-formed in
C++26, and code should no longer compile. GCC is already throwing a warning at
that:
https://godbolt.org/

Therefore, I see no reason to keep functionality that will be ill-formed in the
future. 

Would you be okey with removing those unit tests?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #36 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Applying the following diff I believe I can maintain the current behavior
without changing any tests. It also serves as documentation of what exactly it
does. I've run the test suite a few times and don't notice a change in the
numbers, but of course it'd be great if someone more experienced tests this.

I also confirm that we still get a compiler error as requested in comment #4.

With this in place one can look into refactoring/redesigning to avoid having
this at all, hopefully it should be easier to understand.

Let me know what you think!

--- a/gdbsupport/enum-flags.h
+++ b/gdbsupport/enum-flags.h
@@ -88,15 +88,26 @@ 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<typename T>
+struct enum_will_be_promoted_to_signed_integer
+{
+  static constexpr bool value = std::is_signed<decltype(~T())>::value;
+};
+
+template <typename T>
+struct enum_has_underlying_small_integer
+{
+  static constexpr bool value = sizeof(T) < sizeof(int);
+};
+
 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
+    integer_for_size<sizeof (T),
+                     enum_will_be_promoted_to_signed_integer<T>::value &&
+                     !enum_has_underlying_small_integer<T>::value>::type
     type;
-  DIAGNOSTIC_POP
 };

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #37 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Hi! 

Would you be fine to move forward with the above approach? It doesn't change
the current logic, just makes it more obvious. This would help a future
refactoring and gets rid of the warning.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #38 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Friendly ping. I'm happy to put up a proper patch for review but I'd like to
get an OK about the general concept first. 

Thanks!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[simon.marchi at polymtl dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #39 from Simon Marchi <simon.marchi at polymtl dot ca> ---
(In reply to Carlos Galvez from comment #35)
> - These unit tests, which I find problematic:
> 
> 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 ())
> 
> I propose to remove these unit tests (see below why). With that I believe we
> could make this work.

I agree that these cases probably don't have real-life use cases.  If removing
them helps simplify your implementation, I'd be for it.

About this one that you found:

> ./dwarf2/cooked-index.h:212:22: error: use of deleted function ‘constexpr void operator~(enum_type) [with enum_type = cooked_index_flag_enum; <template-parameter-1-2> = void; <template-parameter-1-3> = void]’
>  212 |     flags = flags & ~IS_PARENT_DEFERRED;
>      |                      ^~~~~~~~~~~~~~~~~~

If I understand correctly, this is a case that our machinery should have
rejected, but it passed through the net?  But at the end of the day, we want to
be able to do this operation here (clear the IS_PARENT_DEFERRED bit).  How else
would we do it if we can't use operator~?

At this point I'm not sure what to think about this, there are just too many
details to wrap my head around it. I suggest you send a proper patch to
gdb-patches, it will make things progress.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #40 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
> How else would we do it if we can't use operator~?

Strictly speaking the correct way would be to cast IS_PARENT_DEFERRED to
unsigned int _before_ applying operator~ on it. That way we ensure that we are
applying the operation on an unsigned integer.

Even if IS_PARENT_DEFERRED is of type "unsigned char", it is _promoted_ to
signed int (as per the Standard) otherwise.

The checks didn't catch it because it's a slightly different issue.

My proposal right now is simply to make this behavior more explicit, without
changing any logic or tests. This should help understanding the code and
facilitating a larger refactoring in the future.

I'll send the patch then, thank you!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #41 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
I've send the patch here:

https://sourceware.org/pipermail/gdb-patches/2024-May/209513.html

It's my first time using git send-email, hopefully I did it correctly! Let me
know otherwise :)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #42 from Sourceware Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ba96d2e697a35517c5ee6b6c20a2fe9a055f7e5f

commit ba96d2e697a35517c5ee6b6c20a2fe9a055f7e5f
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Thu May 30 16:28:21 2024 -0400

    gdb: change names of enumerations in enum flags selftest

    When reading this test (in the context of PR 31331), I had trouble
    understanding the tests, because of the abbreviated names.  I would
    prefer if the names were a bit more explicit, like this.

    Change-Id: I85669b238a9d5dacf673a7bbfc1ca18f80d2b2cf

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #43 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Hi! 

I have a new version of a patch fixing this, but after repeated pings I receive
no feedback:

https://sourceware.org/pipermail/gdb-patches/2024-June/210252.html

We will soon (weeks) turn the Clang warning into a hard error, please consider
reviewing!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-10-22
             Status|UNCONFIRMED                 |NEW

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[jwakely.gcc at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Jonathan Wakely <jwakely.gcc at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jwakely.gcc at gmail dot com

--- Comment #44 from Jonathan Wakely <jwakely.gcc at gmail dot com> ---
(In reply to Simon Marchi from comment #4)
> (In reply to Tom Tromey from comment #2)
> > I changed enum-flags.h to use std::underlying_type, with the idea
> > that if there were any errors, I'd simply change the base
> > enum to use an unsigned type.  (I tried auditing the existing
> > uses by hand but I lost interest partway through.)
> 
> I think that the code explicitly avoids using underlying_type on the enum
> type, to get around this behavior (I'll paste the code to generate this
> table at the end).
> 
> Type             is_signed_v<T>      T(-1)  T(0)  T(-1) < T(0)
> Implicit         false                  -1     0  true  
> ExplicitSigned   true                   -1     0  true  
> ExplicitUnsigned false          4294967295     0  false 
> 
> That is, an enum that doesn't specific a base type (Implicit above) has
> unsigned underlying type,

No, *this* particular enum type has unsigned as the underlying type. Not all
enums without a fixed underlying type though.

The C++ standard doesn't require a particular type, except that:

"For an enumeration whose underlying type is not fixed, the underlying type is
an integral type that can represent all the enumerator values defined in the
enumeration. If no integral type can represent all the enumerator values, the
enumeration is ill-formed. It is implementation-defined which integral type is
used as the underlying type except that the underlying type shall not be larger
than int unless the value of an enumerator cannot fit in an int or unsigned
int."

GCC documents its choice (and Clang is consistent for ABI compat):

https://gcc.gnu.org/onlinedocs/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html

> according to std::underlying_type, but it decays
> to an integer, it appears to behave like a signed type (perhaps some
> language guru can explain why).

Because that's how integer promotion works:

"A prvalue of an unscoped enumeration type whose underlying type is not fixed
can be converted to a prvalue of the first of the following types that can
represent all the values of the enumeration (9.7.1): int, unsigned int, long
int, unsigned long int, long long int, or unsigned long long int."

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[jwakely.gcc at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #45 from Jonathan Wakely <jwakely.gcc at gmail dot com> ---
I'm not sure why this "the underlying type is unsigned but it promotes to int"
behaviour is a problem. If all the valid values of the enumeration fit in int,
it will promote/decay to int, but that's fine because it doesn't alter the
value. All the valid values fit in an int. So promoting to int is
value-preserving.

If the intention is to support:

  auto x = ~STEP_OVER_BREAKPOINT;

That expression is problematic whether or not the enum type is unsigned:

#include <type_traits>
enum E { E1 = 1, E2 = 2 };
static_assert(std::is_unsigned_v<std::underlying_type_t<E>>);
constexpr E e = ~E1;

GCC accepts this, even with -fstrict-enums, but it shouldn't. Clang correctly
rejects it.

The enumeration type has an unsigned underlying type, but ~E1 still produces a
value that is not a valid value of the enumeration type. Converting that back
to E in a constant expression is ill-formed.

I think the deleted operator~ is an incomplete solution trying to prevent ...
something. I'm not sure what.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[jwakely.gcc at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #46 from Jonathan Wakely <jwakely.gcc at gmail dot com> ---
(In reply to Carlos Galvez from comment #43)
> Hi! 
> 
> I have a new version of a patch fixing this, but after repeated pings I
> receive no feedback:
> 
> https://sourceware.org/pipermail/gdb-patches/2024-June/210252.html
> 
> We will soon (weeks) turn the Clang warning into a hard error, please
> consider reviewing!

The patch looks great to me, except this part:

+  // 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);

GCC supports at least one target (msp430 with -mint32 -mn flags) where size_t
is smaller than int, and so promotes to int! Would it make sense to use
unsigned long long or uint64_t instead? Or maybe building GDB on that target
isn't supported anyway.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[jwakely.gcc at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #47 from Jonathan Wakely <jwakely.gcc at gmail dot com> ---
(In reply to Jonathan Wakely from comment #46)
> GCC supports at least one target (msp430 with -mint32 -mn flags) where

Oops, sorry, I got muddled up, it's the H8 target, but I got the right flags.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #48 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Jonathan Wakely from comment #46)

> The patch looks great to me, except this part:

Thank you for reviewing this.
Carlos, did we ask you about copyright assignment yet?

> 
> +  // 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);
> 
> GCC supports at least one target (msp430 with -mint32 -mn flags) where
> size_t is smaller than int, and so promotes to int! Would it make sense to
> use unsigned long long or uint64_t instead? Or maybe building GDB on that
> target isn't supported anyway.

gdb uses ULONGEST for this kind of thing.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #49 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Thanks for reviewing!

> where size_t is smaller than int

Wow, that's mind blowing, would've never thought of that. Sure I can change it
to whatever type is appropriate.

> Carlos, did we ask you about copyright assignment yet?

No, I haven't dealt with this yet. What should I do?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[jwakely.gcc at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #50 from Jonathan Wakely <jwakely.gcc at gmail dot com> ---
(In reply to Carlos Galvez from comment #49)
> > where size_t is smaller than int
> 
> Wow, that's mind blowing, would've never thought of that.

Yeah, it's really horrible :)

N.B. there's an alternative way to handle these "not safe for bitwise
complement" enums, which would be to just use Carlos's new trait in a
static_assert to enforce that all GDB's enum types have a fixed underlying
type. So instead of using the trait to select a deleted operator~ via SFINAE,
just disallow all such enums. With the clever trait, there's no need to rely on
diligent reviewers to notice enum types without a fixed underlying type, they'd
just fail the static_assert. This assumes there's somewhere to put that assert,
e.g. enum_flags gets used with every enum type, or something like that.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #51 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
> enforce that all GDB's enum types have a fixed underlying type

That would require changing existing C-style enums into enums with fixed
underlying type. It was noted in the PR that we probably can't do that since it
could cause ABI breakage. There's a chance the fixed underlying type we choose
is different than the "implementation-specific" type it used to have, even more
so on exotic platforms.

Perhaps it can be looked up in a follow-up commit? My goal was to make the
patch as minimal as possible to solve the immediate problem (Clang build
error).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #52 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Carlos Galvez from comment #49)

> No, I haven't dealt with this yet. What should I do?

Email 'assign@gnu.org' and ask them to get you started.
Feel free to CC me.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #53 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Jonathan Wakely from comment #50)

> N.B. there's an alternative way to handle these "not safe for bitwise
> complement" enums, which would be to just use Carlos's new trait in a
> static_assert to enforce that all GDB's enum types have a fixed underlying
> type.

There's one in shared code (the gcc compile API stuff) that
makes this difficult unfortunately.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #54 from Joel Brobecker <brobecker at gnat dot com> ---
Status update:

Based on
https://sourceware.org/pipermail/gdb-patches/2024-November/213375.html, we're
currently waiting for copyright assignment to go through.

Can you (Carlos) post the links to the patches on gdb-patches which are
approved, and the patches which are pending review still?

Was there only one (in gdbsupport)? And is the latest v2?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Joel Brobecker <brobecker at gnat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |16.1

--- Comment #55 from Joel Brobecker <brobecker at gnat dot com> ---
I'm setting the target milestone to 16.1, to see if we can get that fixed
before then.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #56 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Hi!

The latest patch is the v2 one, here:

https://sourceware.org/pipermail/gdb-patches/2024-June/210252.html

Yes, it's only one patch that would solve this PR. My reading from previous
comments it that it looks good except I should change std::size_t to a more
appropriate type. I'll do that as soon as I get the remaining pieces in place
and push a final v3 version!

I am in the process of going through Copyright assignment, in particular
getting the "employer disclaimer". Unfortunately it seems to go rather slow and
I cannot provide an ETA for when this will be sorted out. Will do my best to
try and expedite this!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #57 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
I have now sorted the legal aspects with my employer and have sent a request
for copyright assignment. Waiting to receive the relevant documents back.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[brobecker at gnat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #58 from Joel Brobecker <brobecker at gnat dot com> ---
Carlos said:

> My reading from previous comments it that it looks good except
> I should change std::size_t to a more appropriate type. I'll do that
> as soon as I get the remaining pieces in place and push a final v3
> version!

Thank you Carlos. When you do, if you could post the link to the gdb-patches
archives in this PR, that would be very helpful for tracking. On my end, I'll
see if I can push a bit for your v3 to be reviewed without too much delay.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #59 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Hello!

The Copyright Assignment is now complete, and I have pushed the v3 version here
changing std::size_t to ULONGEST:

https://sourceware.org/pipermail/gdb-patches/2024-December/214354.html

Let me know if there's anything else I should do!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #60 from Sourceware Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4a0b2cb7210c65c8c9f4a56345749bb7294fbfbf

commit 4a0b2cb7210c65c8c9f4a56345749bb7294fbfbf
Author: Carlos Galvez <carlosgalvezp@gmail.com>
Date:   Sat Dec 21 22:25:40 2024 +0100

    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>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #61 from Tom Tromey <tromey at sourceware dot org> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/31331] Wenum-constexpr-conversion should be fixed, soon treated as a hard error

[carlosgalvezp at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31331

--- Comment #62 from Carlos Galvez <carlosgalvezp at gmail dot com> ---
Thank you for merging and for the minor fixes!

-- 
You are receiving this mail because:
You are on the CC list for the bug.