[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.