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!
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.
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.
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?
(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"); }
(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.
(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.
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.
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.
> 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?
Created attachment 15351 [details] Diff between "make check" on trunk and with the proposed change
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
I should perhaps also mention I have commented out lines 241-270 in enum-flags-selftests.c. Maybe that's why :)
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?
(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.
$ 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
(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.
(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.
(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).
@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?
> 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).
(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
(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.
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.
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?
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
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?
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.
Ok, thanks. I think comment#24 sounds fine then.
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).
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.
*** Bug 23099 has been marked as a duplicate of this bug. ***
AFAICT nobody is working on this and I think we should simply remove the target milestone.
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.
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.
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?
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 };
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.
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!
(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.
> 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!
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 :)
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
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!
(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."
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.
(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.
(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.
(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.
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?
(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.
> 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).
(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.
(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.