-Wenum-constexpr-conversion should be a hard error, not a downgradable error

[carlosgalvezp]

The -Wenum-constexpr-conversion warning was created to account for the fact that casting integers to enums outside of the valid range of the enum is UB in C++17. Constant expressions invoking UB lead to an ill-formed program.

The current behavior of Clang is that it does warn and it does error, but the message is nevertheless a warning that the user can easily suppress via -Wno-enum-constexpr-conversion.

Since the program is ill-formed, I don't think this should be a warning that can be disabled. Just like one can't disable the warning in this ill-formed code:

constexpr int x[10]{};
constexpr int y = x[10];

Therefore I propose to make the diagnostic a hard error that cannot be disabled. Additionally, this "hard error" behavior should only be active in C++17 (it's currently active in for any standard).

The warning could be repurposed to be a general warning that one can enable, and is active not only in constexpr expressions, but in any expression that involves casting an integer to an enum.

[thesamesam]

https://clang.llvm.org/docs/ReleaseNotes.html#potentially-breaking-changes (for Clang 16) says it's an error by default but is currently downgradable (until Clang 17). So I guess this bug is to track remembering to do so in Clang 17.

See also #50055.

[carlosgalvezp]

Nice, I wasn't aware of that, thanks! Yes that makes a lot of sense, thanks for pointing out. Let's keep this bug as a reminder. I will open a new ticket for having a warning that works also on non-constexpr contexts.

[tbaederr]

Did this happen?

[carlosgalvezp]

https://reviews.llvm.org/D150226

[carlosgalvezp]

@AaronBallman @dwblaikie We are now on Clang 19, do you think it's the right time to pull the trigger and turn this into a non-downgradable error? It has been a warning also in system headers since Clang 18.

[dwblaikie]

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

As is openmp in the LLVM project itself (

llvm-project/openmp/cmake/HandleOpenMPOptions.cmake

Line 44 in 58c494f

append_if(OPENMP_HAVE_WENUM_CONSTEXPR_CONVERSION_FLAG "-Wno-enum-constexpr-conversion" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)

)

And I also found this magic_enum project disabling it too: https://github.com/Neargye/magic_enum/blob/b4ada55c8cea21f3c73c5a58cebd368a227465a1/include/magic_enum/magic_enum.hpp#L71

At least for the openmp usage and probably the binutils usage, it'd be nice to see if some progress can be made removing those warning suppressions.

[AaronBallman]

I expect that so long as we leave it a downgradable diagnostic, people will continue to not fix their code because that's easier or because they're unaware of the issue. The OpenMP find is a wonderful example; it was disabled two years ago "to buy ourselves more time" and no attempts have been made to actually fix the issue since then: 9ff0cc7

That said, Clang 19 might be a bit too soon given that we enabled it in system headers in Clang 18, which isn't yet released. I think Clang 20/21 might be somewhat better because that gives folks another year to discover the problems with their uses, but at some point, I think we really do need to force the issue otherwise the can will be kicked down the road forever.

[carlosgalvezp]

FYI I'm bringing this issue to the binutils project.
https://sourceware.org/bugzilla/show_bug.cgi?id=31331

I agree with Aaron, it's way easier to just suppress the warning and call it a compiler bug than actually do some research and understand the subtle issues when doing these tricks with enums. I think that the more we delay it, the more projects will suppress the warning as they bump their clang installation, and so the harder it will be for us to introduce it.

That said, I'm not opposed to waiting another release or two.

[RJVB]

The -Wenum-constexpr-conversion warning was created to account for the fact that casting integers to enums outside of the valid range of the enum is UB in C++17.

Quick question: it is thus not UB in earlier C++ dialects (or C)? I've just come across this "werror" being triggered building Phonon code as gnu++11with clang-16 .

[carlosgalvezp]

Technically speaking it's unspecified behavior until C++17, after that it's UB.

[RJVB]

Technically speaking it's _unspecified_ behavior until C++17, after that it's UB.

And that's better or worse how? :) Probably not the place, but as a mostly-C programmer I fail to see why it would be either from an implementation point of view. An enum type is some kind of integer behind the scenes (a standard `int` IIRC) and as long as the "incriminated" values you try to assign a variable of the enum type in question are within the range of that type there shouldn't be any ambiguity about the result of an assignment? (And assigning a value that can't be represented by the underlying type could give just the same warning as other assignments of that kind?!) Case in point: the "faulty" Phonon code that led me here could be "fixed" by just adding `: unsigned` to the enum declarations. Seeing that it made sense that this "error" had never caused any runtime problems...

[RJVB]

Technically speaking it's _unspecified_ behavior until C++17, after that it's UB.

Oh, and doesn't this imply that clang shouldn't raise an error about it until C++17, technically speaking of course?

[carlosgalvezp]

And that's better or worse how? :)

Worse, because with UB "anything can happen", there's no restrictions. With Unspecified Behavior, only a defined set of behaviors is allowed (but which one is used is not specified in the Standard).

why it would be either from an implementation point of view.

People writing the Standard should probably answer that :) Some things are kept for historical reasons/backwards compatibility, even though they might make little sense today.

a standard int IIRC

That's true in C, but not in C++, where there's quite a few cases depending on whether the enum is a C-style enum, scoped/unscoped, with explicit underlying type, etc.

Seeing that it made sense that this "error" had never caused any runtime problems...

It's often the case with UB that the program "just works", but it can break anytime in the future when refactoring code, enabling optimizations, updating the compiler, etc.

clang shouldn't raise an error about it until C++17, technically speaking of course?

Good question, I'm not sure actually. The Standard requires that it's an error from C++17, but nothing stops an implementation to make it an error earlier than that. Maybe @AaronBallman @dwblaikie know better?

[RJVB]

On Saturday April 27 2024 12:03:21 Carlos Galvez wrote: > a standard `int` IIRC That's true in C, but not in C++, where there's quite a few cases depending on whether the enum is a C-style enum, scoped/unscoped, with explicit underlying type, etc.

And that hasn't evolved more or less slowly from "true in C" (at some point C++ was just a preprocessor)?

> Seeing that it made sense that this "error" had never caused any runtime problems... It's often the case with UB that the program "just works", but it can break anytime in the future when refactoring code, enabling optimizations, updating the compiler, etc.

True, and it's quite possible the latent bugs I ever found in my own code by simply building it on a different platform (OS) were examples. (But to remain with my own example at hand: 2 compilers on 2 different platforms generate exactly the same code, shared libraries, with and with the "unsigned" fix. I guess "unspecified" doesn't mean that there can't be a sensical thing to do...)

Good question, I'm not sure actually. The Standard requires that it's an error from C++17, but nothing stops an implementation to make it an error earlier than that.

But one that can be overridden, in that case? Standards are probably akin to legislation, where most things are either explicitly forbidden or implicitly allowed?

[carlosgalvezp]

And that hasn't evolved more or less slowly from "true in C" (at some point C++ was just a preprocessor)?

C++ is a different language than C, with different standards. Sure, there is a lot of backwards compatibility with C, but there are slight differences.

I guess "unspecified" doesn't mean that there can't be a sensical thing to do...

Absolutely, but there may be multiple sensical things to do and different implementations don't necessarily need to agree.

But one that can be overridden, in that case? Standards are probably akin to legislation, where most things are either explicitly forbidden or implicitly allowed?

Currently the warning can be suppressed in order to give time to people to update their code, before making it a hard error. In that sense Clang is currently not complying with the Standard (but the goal is to make it be).

Standards describe mostly what is allowed, and often clarify that "otherwise, the behavior is undefined".

[RJVB]

On Sunday April 28 2024 07:05:25 Carlos Galvez wrote: C++ is a different language than C, with different standards. Sure, there is a lot of backwards compatibility with C, but there are slight differences.

Of course it is.

Currently the warning can be suppressed in order to give time to people to update their code, before making it a hard error. In that sense Clang is currently not complying with the Standard (but the goal is to make it be).

No argument against making it a hard error for C++17 and newer; I *was* talking about earlier standards. I'll admit that I can't really wrap my head around whether or not "updating the code" can always be as simple as in the single example I've seen, or less kludgy than just replacing the offending enum by a regular int data type.

[carlosgalvezp]

I suppose that should be technically possible. What's the argument for keeping it since C++11 instead of C++17 @AaronBallman @dwblaikie @shafik ?

[AaronBallman]

I suppose that should be technically possible. What's the argument for keeping it since C++11 instead of C++17 @AaronBallman @dwblaikie @shafik ?

This was a change as a result of a defect report and we typically apply defect reports as though the original specification was always worded with the fix applied.

[carlosgalvezp]

Thanks for the explanation! I guess you meant this one?
https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1766

In cppreference it even says it applies from C++98 onwards.

@RJVB does this answer your question?

[AaronBallman]

Thanks for the explanation! I guess you meant this one? https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1766

It's sort of a combination of both. 1766 made it UB for out-of-range value conversion, 2338 reduced the restriction but still kept some parts of it UB, which impacts constexpr behavior.

[RJVB]

@RJVB does this answer your question?

Not really, as far as I can understand such "philosophical" reasonings I still seem to see retro-active changing of old standards and deliberate breaking of backwards compatibility. Two things I have difficulties wrapping my head around because so contrary to the everything I ever did.

[AaronBallman]

The purpose of the committee adopting a change as a defect report is to fix bugs with features of the language or library. While that can and does change behavior previously promised by the standard, the intent is that the previous standard was wrong and the changes are fixing an issue.

Whether that's a good idea or not is a matter for debate, but it is a practice that WG21 has had for over a decade and is something the committee strongly encourages implementations to do so that the language remains coherent. We are not required to implement a defect report in all language modes in terms of meeting conformance requirements for a standard, but it is a tradeoff. Implementing DRs the way the committee requests often helps migration between standards modes, but at the expense of hurting migration between compiler versions, unfortunately.

In this case, we've signaled our intent to turn this into a hard error in a future release of Clang. If there's compelling rationale for why we should not turn it into a hard error, it's something we'll certainly consider when deciding when or if to follow through on that intent.

Case in point: the "faulty" Phonon code that led me here could be "fixed" by just adding : unsigned to the enum declarations. Seeing that it made sense that this "error" had never caused any runtime problems...

C and C++ differ in their treatment of enumerations. In C before C23, the enumerators were of type int and the underlying type of the enumeration was either char, a signed integer type, or an unsigned integer type (implementation-defined). A lot of implementations had extensions to these rules, and those extensions were largely ratified for C23. However, in all versions of C++, an enumeration's underlying type was the smallest power-of-two integer bit-field that could represent all the values of the enumeration (up to a max of unsigned int). So: enum E { zero, one, two, three }; in C would be able to hold values that can fit in an int while in C++ it would be able to hold values up to unsigned int : 2. It's not possible to say "this makes sense in C so it should be fine in C++ as well" in all cases, and this is one such case. Adding an explicit underlying type to the enum declaration in C++ forces the underlying type to be large enough to hold any value that fits within that type, that's why it fixes the incorrect code. The code may not have caused runtime problems for you, but it was still undefined behavior and not something you could rely on working correctly across compilers or compiler upgrades. It sounds like this diagnostic caught a real issue that would have otherwise remained latent in the code base and could have caused a hard-to-debug issue if an optimizer started taking more advantage of the C++ semantics for enumerations.

[RJVB]

On Tuesday May 07 2024 07:03:53 Aaron Ballman wrote: The code may not have caused runtime problems for you, but it was still undefined behavior and not something you could rely on working correctly across compilers or compiler upgrades.

According to what was said above it was UB only from C++17 onwards, while the code in question probably predates even C++11. Generations of compilers did not miscompile it (or it would have been changed before) and IMHO it would be a conscious choice to start miscompiling it. I would find it very hard to accept a breaking change in what is arguably a compatitibity mode (`-std=c++11`) nowadays, and if I hadn't decided to take the time to understand the very cryptic error messages I might have concluded that I'd best continue to use older (and generally faster) clang versions, or GCC (also generally faster nowadays...) I recall trying to understand the messages and thinking that some really looked like one of those situations where the compiler somehow can't see that you're NOT using a non-initialised variable.

It sounds like this diagnostic caught a real issue that would have otherwise remained latent in the code base and could have caused a hard-to-debug issue if an optimizer started taking more advantage of the C++ semantics for enumerations.

Well, as I said, the message (also when printed as a warning) was certainly no help in debugging the issue, which could well explain why people have the reflex to just make the warning really only a mere warning. Build the same code with g++'s `-Wconversion` and you get something much more useful that actually pinpoints the place where you can fix the problem. That's how I found the location in Phonon that hadn't (possibly still hasn't) been fixed yet.

[carlosgalvezp]

Fyi I have now put up a patch for review to the binutils project:
https://sourceware.org/pipermail/gdb-patches/2024-May/209513.html

[carlosgalvezp]

Hi!

We are now at Clang 20. Would it be now a good time to turn the warning into a hard error?

I'm pinging the GDB community but I get no reviews of the above patch. Unfortunately there's not much more I can do. Hopefully if we turn the warning into an error it will put some pressure into fixing the code with the available patch.

What do you think @AaronBallman @dwblaikie @shafik ?

[AaronBallman]

We are now at Clang 20. Would it be now a good time to turn the warning into a hard error?

I think it's worth a shot, yes.

I'm pinging the GDB community but I get no reviews of the above patch. Unfortunately there's not much more I can do. Hopefully if we turn the warning into an error it will put some pressure into fixing the code with the available patch.

Thank you for keeping on top of that!

[carlosgalvezp]

Thanks! I can prepare a patch. For consistency, I think it would be nice to show the error like this:

<source>:2:15: error: constexpr variable 'a' must be initialized by a constant expression
    2 | constexpr int a  = x[15];
      |               ^    ~~~~~
<source>:2:20: note: cannot refer to element 15 of array of 10 elements in a constant expression
    2 | constexpr int a  = x[15];
      |                    ^

I.e. first print the generic error, and follow-up with the note (which is the current diagnostic as a warning). This clarifies that the hard error is only in the context of a constant expression.