[Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[shafik]

Summary

In D130058 we diagnose the undefined behavior of setting the value outside the range of the enumerations values for an enum without a fixed underlying type.

Based on feedback we will provide users to the ability to downgrade this this diagnostic to a waring to allow for a transition period. We expect to turn this diagnostic to an error in the next release.

[shafik]

clang/lib/AST/ExprConstant.cpp

13537

Changing this to a warning meaning it applied in more cases, so I need to explicitly restrict this to C++ contexts.

clang/test/SemaCXX/MicrosoftCompatibility.cpp

242

This is undefined behavior, using 0 avoids the diagnostic and I believe still sticks to the intent of the test. Same below.

clang/test/SemaCXX/compare.cpp

432

This is UB and I don't think this applies anymore.

clang/test/SemaCXX/constant-expression-cxx11.cpp

2422

I kept this on the next line since when we turn this back into an error we will have two diagnostics again.

[smeenai]

Thank you!

I'm worried that users might miss this if it's only in the release notes, and then we'd be in a similar situation again when we try converting it to an error. Maybe you could also include the bit about the diagnostic turning into error-only in the diagnostic message itself, to make it more likely for people to notice it?

[thakis]

It's already an error, but it's a warning default-mapped to an error. You can -Wno-error=name to downgrade it into a warning, but that requires an explicit action. So people are unlikely to miss it.

This is how we usually handle these breaking changes.

Maybe there could be a test for the -Wno-error= case? But this looks roughly right to me overall. I haven't looked in detail.

[smeenai]

In D131307#3704709, @thakis wrote:

It's already an error, but it's a warning default-mapped to an error. You can -Wno-error=name to downgrade it into a warning, but that requires an explicit action. So people are unlikely to miss it.

This is how we usually handle these breaking changes.

Maybe there could be a test for the -Wno-error= case? But this looks roughly right to me overall. I haven't looked in detail.

Right, but we also want people to understand that the downgrade possibility will be going away in the next release (i.e. it'll always be an error and there's nothing you can do about that), so they really do want to deal with this as a priority.

[erichkeane]

In D131307#3704709, @thakis wrote:

It's already an error, but it's a warning default-mapped to an error. You can -Wno-error=name to downgrade it into a warning, but that requires an explicit action. So people are unlikely to miss it.

This is how we usually handle these breaking changes.

Maybe there could be a test for the -Wno-error= case? But this looks roughly right to me overall. I haven't looked in detail.

We don't typically test versions of -Wno-error=, we tend to trust that the diagnostics system 'works' in this case. Unless there is a situation that you're concerned about?

In D131307#3705362, @smeenai wrote:

In D131307#3704709, @thakis wrote:

It's already an error, but it's a warning default-mapped to an error. You can -Wno-error=name to downgrade it into a warning, but that requires an explicit action. So people are unlikely to miss it.

This is how we usually handle these breaking changes.

Maybe there could be a test for the -Wno-error= case? But this looks roughly right to me overall. I haven't looked in detail.

Right, but we also want people to understand that the downgrade possibility will be going away in the next release (i.e. it'll always be an error and there's nothing you can do about that), so they really do want to deal with this as a priority.

While I'm sympathetic to this, I don't think there is precedent for doing something like that. I think I'd be OK tacking an extra note onto this (and starting said precedent), but I think we'd need to hear from a code-owner to make a change like that.

[thakis]

Default-mapped-to-error and release notes seems like a pretty strong signal to me. I think this is fine as is.

[aaron.ballman]

In D131307#3705362, @smeenai wrote:

In D131307#3704709, @thakis wrote:

It's already an error, but it's a warning default-mapped to an error. You can -Wno-error=name to downgrade it into a warning, but that requires an explicit action. So people are unlikely to miss it.

This is how we usually handle these breaking changes.

Maybe there could be a test for the -Wno-error= case? But this looks roughly right to me overall. I haven't looked in detail.

Right, but we also want people to understand that the downgrade possibility will be going away in the next release (i.e. it'll always be an error and there's nothing you can do about that), so they really do want to deal with this as a priority.

FWIW, we had a similar situation (but not quite the same) during Clang 15 where we had some diagnostics that were strengthened to be an error but that allowance only exists in some language modes:

- The ``-Wimplicit-function-declaration`` warning diagnostic now defaults to
  an error in C99 and later. Prior to C2x, it may be downgraded to a warning
  with ``-Wno-error=implicit-function-declaration``, or disabled entirely with
  ``-Wno-implicit-function-declaration``. As of C2x, support for implicit
  function declarations has been removed, and the warning options will have no
  effect.
- The ``-Wimplicit-int`` warning diagnostic now defaults to an error in C99 and
  later. Prior to C2x, it may be downgraded to a warning with
  ``-Wno-error=implicit-int``, or disabled entirely with ``-Wno-implicit-int``.
  As of C2x, support for implicit int has been removed, and the warning options
  will have no effect. Specifying ``-Wimplicit-int`` in C89 mode will now issue
  warnings instead of being a noop.

I think the release note we have in this patch is reasonable.

This LGTM!

[smeenai]

All right, SGTM

[ronlieb]

hi, latest commit seems to have broken buildbot for amdgpu
https://lab.llvm.org/buildbot/#/builders/193/builds/16651

[shafik]

In D131307#3708413, @ronlieb wrote:

hi, latest commit seems to have broken buildbot for amdgpu
https://lab.llvm.org/buildbot/#/builders/193/builds/16651

This looks like a real bug, the enum which has a typedef hsa_agent_info_t has a max enumerator with a value of 24 and therefore because it does not have a fixed underlying type the value range is indeed [0, 31].

I am not sure what the right fix is here since it looks like the enum is meant to be used with C as well as C++. In C++ one could give a fixed underlying type if it was meant to have any int value let's say.

HSA_AMD_AGENT_INFO_COMPUTE_UNIT_COUNT is way outside the values of hsa_agent_info_t so maybe this was not intended at all.

[ronlieb]

thanks for the analysis , i agree its a coding error in our library.

[sylvestre.ledru]

We have this error in OpenMP:
https://github.com/llvm/llvm-project/issues/57022

[aaron.ballman]

In D131307#3709083, @sylvestre.ledru wrote:

We have this error in OpenMP:
https://github.com/llvm/llvm-project/issues/57022

That bug looks to be a true positive from this change.

[JonChesterfield]

Did some digging here. The function hsa_agent_get_info takes an argument of type hsa_agent_info_t, which has declared values in the range [0 24]. The implementation of that (in amd_gpu_agent fwiw) casts that argument to a size_t and then switches on it, checking those declared values and a bunch of extensions. This is used to provide vendor extensions through a vendor-agnostic interface.

This seems to be a case where C and C++ have diverged. As far as I can tell, C thinks an enum is an int, and anything that fits in an int can be stored in one and retrieved later. C23 lets one specify the underlying type. C++ evidently thinks the value stored must be within [min max] of the declaration, which is at least more flexible than requiring it be one in the declaration.

So I think the fix here is to change hsa_agent_info_t to include HSA_AGENT_INFO_UNUSED_INCREASE_RANGE_OF_TYPE = INT32_MAX so the vendor extensions remain accessible. It's a header that is usable from C and C++ so it needs to do something conforming to both. Does that sound right?

[aaron.ballman]

In D131307#3709494, @JonChesterfield wrote:

Did some digging here. The function hsa_agent_get_info takes an argument of type hsa_agent_info_t, which has declared values in the range [0 24]. The implementation of that (in amd_gpu_agent fwiw) casts that argument to a size_t and then switches on it, checking those declared values and a bunch of extensions. This is used to provide vendor extensions through a vendor-agnostic interface.

This seems to be a case where C and C++ have diverged.

Yes, they've always been divergent in this area, it's only thanks to the magic of constexpr that anyone really notices now.

As far as I can tell, C thinks an enum is an int, and anything that fits in an int can be stored in one and retrieved later.

It's a bit more complicated than that, unfortunately. But this is kind of the gist of it.

C23 lets one specify the underlying type. C++ evidently thinks the value stored must be within [min max] of the declaration, which is at least more flexible than requiring it be one in the declaration.

Correct, C++ uses the minimum-sized bit-field that can hold all of the values of the enumeration.

So I think the fix here is to change hsa_agent_info_t to include HSA_AGENT_INFO_UNUSED_INCREASE_RANGE_OF_TYPE = INT32_MAX so the vendor extensions remain accessible. It's a header that is usable from C and C++ so it needs to do something conforming to both. Does that sound right?

Yes, that's how I'd solve the problem.

[zatrazz]

This commit seems to have broken test-suite on aarch64 [1]. The warning shows:

FAILED: MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o
/home/adhemerval.zanella/projects/llvm/test-suite-build/tools/timeit --summary MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o.time /home/adhemerval.zanella/projects/llvm/llvm-src-build-stage1-buildbot/bin/clang++ -DNDEBUG  -O3 -DNDEBUG   -w -Werror=date-time -DNOASM -DLLVM -MD -MT MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o -MF MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o.d -o MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o -c /home/adhemerval.zanella/projects/llvm/test-suite/MultiSource/Benchmarks/PAQ8p/paq8p.cpp
/home/adhemerval.zanella/projects/llvm/test-suite/MultiSource/Benchmarks/PAQ8p/paq8p.cpp:3606:24: error: integer value -1 is outside the valid range of values [0, 15] for this enumeration type [-Wenum-constexpr-conversion]
    if (c==EOF) return (Filetype)(-1);
                       ^
1 error generated.

I am not sure if this is an existing issue with test-suite.

[1] https://lab.llvm.org/staging/#/builders/158/builds/344

[erichkeane]

In D131307#3709595, @zatrazz wrote:

This commit seems to have broken test-suite on aarch64 [1]. The warning shows:

FAILED: MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o
/home/adhemerval.zanella/projects/llvm/test-suite-build/tools/timeit --summary MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o.time /home/adhemerval.zanella/projects/llvm/llvm-src-build-stage1-buildbot/bin/clang++ -DNDEBUG  -O3 -DNDEBUG   -w -Werror=date-time -DNOASM -DLLVM -MD -MT MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o -MF MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o.d -o MultiSource/Benchmarks/PAQ8p/CMakeFiles/paq8p.dir/paq8p.cpp.o -c /home/adhemerval.zanella/projects/llvm/test-suite/MultiSource/Benchmarks/PAQ8p/paq8p.cpp
/home/adhemerval.zanella/projects/llvm/test-suite/MultiSource/Benchmarks/PAQ8p/paq8p.cpp:3606:24: error: integer value -1 is outside the valid range of values [0, 15] for this enumeration type [-Wenum-constexpr-conversion]
    if (c==EOF) return (Filetype)(-1);
                       ^
1 error generated.

I am not sure if this is an existing issue with test-suite.

[1] https://lab.llvm.org/staging/#/builders/158/builds/344

That IS Undefined Behavior, but I think was unintended by this patch. The intent of this patch (and its parent) was to diagnose this UB during constexpr evaluation. HOWEVER, this patch seems to have extended it to non-constexpr constant evaluation. So while that _IS_ undefined behavior, I don't think it should be covered by this error (likely a normal 'warning' is fine here, but not this diagnostic). Hopefully @shafik can correct this when he starts again today.

[shafik]

In D131307#3709644, @erichkeane wrote:

That IS Undefined Behavior, but I think was unintended by this patch. The intent of this patch (and its parent) was to diagnose this UB during constexpr evaluation. HOWEVER, this patch seems to have extended it to non-constexpr constant evaluation. So while that _IS_ undefined behavior, I don't think it should be covered by this error (likely a normal 'warning' is fine here, but not this diagnostic). Hopefully @shafik can correct this when he starts again today.

That was not totally intended. I am working on some changes to narrow down the scope of the diagnostic and hopefully that will reduce the fallout.

Although Erich is correct, that is undefined behavior and should be eventually be fixed.

[shafik]

@zatrazz please see D131528 which should prevent the diagnostic for triggering for the example you showed us.

[sberg]

With this commit,

$ cat test.cc
#include "boost/numeric/conversion/cast.hpp"
int main() { return boost::numeric_cast<int>(0L); }

$ clang++ test.cc

succeeds without any diagnostic, while with its parent commit https://github.com/llvm/llvm-project/commit/b3645353041818f61e2580635409ddb81ff5a272 " [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values" it had started to fail with

In file included from test.cc:1:
In file included from /usr/include/boost/numeric/conversion/cast.hpp:33:
In file included from /usr/include/boost/numeric/conversion/converter.hpp:13:
In file included from /usr/include/boost/numeric/conversion/conversion_traits.hpp:13:
In file included from /usr/include/boost/numeric/conversion/detail/conversion_traits.hpp:18:
In file included from /usr/include/boost/numeric/conversion/detail/int_float_mixture.hpp:19:
In file included from /usr/include/boost/mpl/integral_c.hpp:32:
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: non-type template argument is not a constant expression
    typedef AUX_WRAPPER_INST( BOOST_MPL_AUX_STATIC_CAST(AUX_WRAPPER_VALUE_TYPE, (value - 1)) ) prior;
            ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
#   define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
                                              ^
/usr/include/boost/mpl/integral_c.hpp:31:54: note: expanded from macro 'AUX_WRAPPER_INST'
#define AUX_WRAPPER_INST(value) AUX_WRAPPER_NAME< T, value >
                                                     ^~~~~
/usr/include/boost/numeric/conversion/detail/meta.hpp:30:46: note: in instantiation of template class 'mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>' requested here
       enum { x = ( BOOST_MPL_AUX_VALUE_WKND(T1)::value == BOOST_MPL_AUX_VALUE_WKND(T2)::value ) };
                                             ^
/usr/include/boost/mpl/if.hpp:63:68: note: in instantiation of template class 'boost::numeric::convdetail::equal_to<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>>' requested here
          BOOST_MPL_AUX_STATIC_CAST(bool, BOOST_MPL_AUX_VALUE_WKND(T1)::value)
                                                                   ^
/usr/include/boost/mpl/eval_if.hpp:37:22: note: in instantiation of template class 'boost::mpl::if_<boost::numeric::convdetail::equal_to<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>>, boost::mpl::identity<boost::numeric::convdetail::get_subranged_BuiltIn2BuiltIn<int, long>>, boost::mpl::eval_if<boost::numeric::convdetail::equal_to<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_udt>>, boost::mpl::identity<boost::mpl::identity<boost::numeric::convdetail::subranged_BuiltIn2Udt<int, long>>>, boost::mpl::if_<boost::numeric::convdetail::equal_to<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::udt_to_builtin>>, boost::mpl::identity<boost::numeric::convdetail::subranged_Udt2BuiltIn<int, long>>, boost::mpl::identity<boost::numeric::convdetail::subranged_Udt2Udt<int, long>>>>>' requested here
    typedef typename if_<C,F1,F2>::type f_;
                     ^
/usr/include/boost/numeric/conversion/detail/meta.hpp:81:12: note: in instantiation of template class 'boost::mpl::eval_if<boost::numeric::convdetail::equal_to<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>>, boost::mpl::identity<boost::numeric::convdetail::get_subranged_BuiltIn2BuiltIn<int, long>>, boost::mpl::eval_if<boost::numeric::convdetail::equal_to<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_udt>>, boost::mpl::identity<boost::mpl::identity<boost::numeric::convdetail::subranged_BuiltIn2Udt<int, long>>>, boost::mpl::if_<boost::numeric::convdetail::equal_to<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::udt_to_builtin>>, boost::mpl::identity<boost::numeric::convdetail::subranged_Udt2BuiltIn<int, long>>, boost::mpl::identity<boost::numeric::convdetail::subranged_Udt2Udt<int, long>>>>>' requested here
      mpl::eval_if<is_case0,Case0TypeQ,choose_1_2_3Q>::type
           ^
/usr/include/boost/numeric/conversion/detail/udt_builtin_mixture.hpp:41:7: note: in instantiation of template class 'boost::numeric::convdetail::ct_switch4<mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_builtin>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::builtin_to_udt>, mpl_::integral_c<boost::numeric::udt_builtin_mixture_enum, boost::numeric::udt_to_builtin>, boost::numeric::convdetail::get_subranged_BuiltIn2BuiltIn<int, long>, boost::mpl::identity<boost::numeric::convdetail::subranged_BuiltIn2Udt<int, long>>, boost::mpl::identity<boost::numeric::convdetail::subranged_Udt2BuiltIn<int, long>>, boost::mpl::identity<boost::numeric::convdetail::subranged_Udt2Udt<int, long>>>' requested here
      ct_switch4<UdtMixture
      ^
/usr/include/boost/numeric/conversion/detail/is_subranged.hpp:205:9: note: (skipping 3 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
        for_udt_builtin_mixture<udt_builtin_mixture, BuiltIn2BuiltInQ, BuiltIn2UdtQ, Udt2BuiltInQ, Udt2UdtQ>::type
        ^
/usr/include/boost/numeric/conversion/conversion_traits.hpp:22:7: note: in instantiation of template class 'boost::numeric::convdetail::non_trivial_traits_impl<int, long>' requested here
    : convdetail::get_conversion_traits<T,S>::type
      ^
/usr/include/boost/numeric/conversion/detail/converter.hpp:584:22: note: in instantiation of template class 'boost::numeric::conversion_traits<int, long>' requested here
    typedef typename Traits::trivial trivial ;
                     ^
/usr/include/boost/numeric/conversion/converter.hpp:29:32: note: in instantiation of template class 'boost::numeric::convdetail::get_converter_impl<boost::numeric::conversion_traits<int, long>, boost::numeric::def_overflow_handler, boost::numeric::Trunc<long>, boost::numeric::raw_converter<boost::numeric::conversion_traits<int, long>>, boost::numeric::UseInternalRangeChecker>' requested here
struct converter : convdetail::get_converter_impl<Traits,
                               ^
/usr/include/boost/numeric/conversion/cast.hpp:53:16: note: in instantiation of template class 'boost::numeric::converter<int, long, boost::numeric::conversion_traits<int, long>, boost::numeric::def_overflow_handler, boost::numeric::Trunc<long>>' requested here
        return converter::convert(arg);
               ^
test.cc:2:28: note: in instantiation of function template specialization 'boost::numeric_cast<int, long>' requested here
int main() { return boost::numeric_cast<int>(0L); }
                           ^
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: note: integer value -1 is outside the valid range of values [0, 3] for this enumeration type
    typedef AUX_WRAPPER_INST( BOOST_MPL_AUX_STATIC_CAST(AUX_WRAPPER_VALUE_TYPE, (value - 1)) ) prior;
                              ^
/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
#   define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
                                              ^

I'm not sure that's an intended change? (And sorry for not stripping down the reproducer to something self-contained.)

[shafik]

In D131307#3713011, @sberg wrote:

With this commit,

$ cat test.cc
#include "boost/numeric/conversion/cast.hpp"
int main() { return boost::numeric_cast<int>(0L); }

$ clang++ test.cc

succeeds without any diagnostic, while with its parent commit https://github.com/llvm/llvm-project/commit/b3645353041818f61e2580635409ddb81ff5a272 " [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values" it had started to fail with

Yes, that is intended. When modifying the change to allow it to be turned into a warning it started applying outside of constant expression contexts and that broke a lot more stuff.

I am planning on adding a default to a warning diagnostic for the non-constant expression cases but that will be done separately. I wanted to help folks unbreak their builds first.

[ayermolo]

This comment was removed by ayermolo.

[sberg]

In D131307#3714629, @shafik wrote:

In D131307#3713011, @sberg wrote:

With this commit,

$ cat test.cc
#include "boost/numeric/conversion/cast.hpp"
int main() { return boost::numeric_cast<int>(0L); }

$ clang++ test.cc

succeeds without any diagnostic, while with its parent commit https://github.com/llvm/llvm-project/commit/b3645353041818f61e2580635409ddb81ff5a272 " [Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values" it had started to fail with

Yes, that is intended. When modifying the change to allow it to be turned into a warning it started applying outside of constant expression contexts and that broke a lot more stuff.

I think we might be talking past each other here. There are three commits:

1. D130058 "[Clang] Diagnose ill-formed constant expression when setting a non fixed enum to a value outside the range of the enumeration values"
2. D131307 "[Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values"
3. D131528 "[Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression"

(1) had started to diagnose my reproducer as an error (and I assume it did rightly so). But then (2) stopped diagnosing it at all, letting the reproducer compile successfully without any diagnostic (and I assume wrongly so). (3) didn't make any further change regarding that behavior.

[smeenai]

Was it intended that the warning generated here isn't silenced by -w, only by an explicit -Wno-enum-constexpr-conversion (or -Wno-everythning), and that -Wno-error doesn't downgrade the error? See https://godbolt.org/z/s9qPveTWG for an example.

[erichkeane]

In D131307#3726631, @smeenai wrote:

Was it intended that the warning generated here isn't silenced by -w, only by an explicit -Wno-enum-constexpr-conversion (or -Wno-everythning), and that -Wno-error doesn't downgrade the error? See https://godbolt.org/z/s9qPveTWG for an example.

Yes, we've discussed that on this thread before: Clang's behavior for warnings-as-default-error require explicit suppression of the warning, and isn't effected by global warning/error settings.

[aaron.ballman]

In D131307#3726631, @smeenai wrote:

Was it intended that the warning generated here isn't silenced by -w, only by an explicit -Wno-enum-constexpr-conversion (or -Wno-everythning), and that -Wno-error doesn't downgrade the error? See https://godbolt.org/z/s9qPveTWG for an example.

Yes. That is the behavior of warnings which default to an error. The idea is: these aren't really *warnings*, they're errors that we let users downgrade for <reasons>. So -w shouldn't blanket disable them or users will be very surprised when that warning turns into a hard error in a future version of the compiler. So you have to explicitly disable warnings that default to an error. The same is true for -Wno-error behavior.

[smeenai]

In D131307#3726643, @erichkeane wrote:

In D131307#3726631, @smeenai wrote:

Was it intended that the warning generated here isn't silenced by -w, only by an explicit -Wno-enum-constexpr-conversion (or -Wno-everythning), and that -Wno-error doesn't downgrade the error? See https://godbolt.org/z/s9qPveTWG for an example.

Yes, we've discussed that on this thread before: Clang's behavior for warnings-as-default-error require explicit suppression of the warning, and isn't effected by global warning/error settings.

In D131307#3726644, @aaron.ballman wrote:

In D131307#3726631, @smeenai wrote:

Was it intended that the warning generated here isn't silenced by -w, only by an explicit -Wno-enum-constexpr-conversion (or -Wno-everythning), and that -Wno-error doesn't downgrade the error? See https://godbolt.org/z/s9qPveTWG for an example.

Yes. That is the behavior of warnings which default to an error. The idea is: these aren't really *warnings*, they're errors that we let users downgrade for <reasons>. So -w shouldn't blanket disable them or users will be very surprised when that warning turns into a hard error in a future version of the compiler. So you have to explicitly disable warnings that default to an error. The same is true for -Wno-error behavior.

Yup, all of that makes sense; I just missed it earlier. Thank you both :)

[rsmith]

Based on feedback we will provide users to the ability to downgrade this this diagnostic to a waring to allow for a transition period. We expect to turn this diagnostic to an error in the next release.

Can we revert this now?

[erichkeane]

In D131307#4197767, @rsmith wrote:

Based on feedback we will provide users to the ability to downgrade this this diagnostic to a waring to allow for a transition period. We expect to turn this diagnostic to an error in the next release.

Can we revert this now?

Seems appropriate. Aaron, WDYT?

[aaron.ballman]

In D131307#4197789, @erichkeane wrote:

In D131307#4197767, @rsmith wrote:

Based on feedback we will provide users to the ability to downgrade this this diagnostic to a waring to allow for a transition period. We expect to turn this diagnostic to an error in the next release.

Can we revert this now?

Seems appropriate. Aaron, WDYT?

I think it's fine -- if we get significant user feedback after 16 goes out to the public which suggests we need to retain this for longer, it's easy enough to bring back from version control.

clang/docs/ReleaseNotes.rst

- Fix a crash when evaluating a multi-dimensional array's array filler
   expression is element-dependent. This fixes
   `Issue 50601 <https://github.com/llvm/llvm-project/issues/56016>`_.
 
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now correctly diagnose as ill-formed a constant expression where an
   enum without a fixed underlying type is set to a value outside the range of
-  the enumeration's values. Fixes
+  the enumeration's values. Due to the extended period of time this bug was
+  present in major C++ implementations (including Clang), this error has the
+  ability to be downgraded into a warning (via: -Wno-error=enum-constexpr-conversion)
+  to provide a transition period for users. This diagnostic is expected to turn
+  into an error-only diagnostic in the next Clang release. Fixes
   `Issue 50055: <https://github.com/llvm/llvm-project/issues/50055>`_.
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.
 
 Non-comprehensive list of changes in this release

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_constexpr_delete_base_nonvirt_dtor : Note<
   "base class type %0 with non-virtual destructor">;
 def note_constexpr_memory_leak : Note<
   "allocation performed here was not deallocated"
   "%plural{0:|: (along with %0 other memory leak%s0)}0">;
 def note_constexpr_unsupported_layout : Note<
   "type %0 has unexpected layout">;
 def note_constexpr_unsupported_flexible_array : Note<
   "flexible array initialization is not yet supported">;
-def note_constexpr_unscoped_enum_out_of_range : Note<
-  "integer value %0 is outside the valid range of values [%1, %2] for this "
-  "enumeration type">;
 def err_experimental_clang_interp_failed : Error<
   "the experimental clang interpreter failed to evaluate an expression">;
 
 def warn_integer_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,
   InGroup<DiagGroup<"integer-overflow">>;
 def warn_fixedpoint_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,
   InGroup<DiagGroup<"fixed-point-overflow">>;
+def warn_constexpr_unscoped_enum_out_of_range : Warning<
+  "integer value %0 is outside the valid range of values [%1, %2] for this "
+  "enumeration type">, DefaultError, InGroup<DiagGroup<"enum-constexpr-conversion">>;
 
 // This is a temporary diagnostic, and shall be removed once our
 // implementation is complete, and like the preceding constexpr notes belongs
 // in Sema.
 def note_unimplemented_constexpr_lambda_feature_ast : Note<
     "unimplemented constexpr lambda feature: %0 (coming soon!)">;
 
 def warn_is_constant_evaluated_always_true_constexpr : Warning<

clang/lib/AST/ExprConstant.cpp

if (!Result.isInt()) {
       // to detect here.  We let it through on the assumption the user knows
       // what they are doing.)
       if (Result.isAddrLabelDiff())
         return Info.Ctx.getTypeSize(DestType) <= Info.Ctx.getTypeSize(SrcType);
       // Only allow casts of lvalues if they are lossless.
       return Info.Ctx.getTypeSize(DestType) == Info.Ctx.getTypeSize(SrcType);
     }
 
-    if (DestType->isEnumeralType()) {
+    if (Info.Ctx.getLangOpts().CPlusPlus && DestType->isEnumeralType()) {
       const EnumType *ET = dyn_cast<EnumType>(DestType.getCanonicalType());
       const EnumDecl *ED = ET->getDecl();
       // Check that the value is within the range of the enumeration values.
       //
       // This corressponds to [expr.static.cast]p10 which says:
       // A value of integral or enumeration type can be explicitly converted
       // to a complete enumeration type ... If the enumeration type does not
       // have a fixed underlying type, the value is unchanged if the original
       // value is within the range of the enumeration values ([dcl.enum]), and
       // otherwise, the behavior is undefined.
       //
       // This was resolved as part of DR2338 which has CD5 status.
       if (!ED->isFixed()) {
         llvm::APInt Min;
         llvm::APInt Max;
 
         ED->getValueRange(Max, Min);
         --Max;
 
         if (ED->getNumNegativeBits() &&
             (Max.slt(Result.getInt().getSExtValue()) ||
              Min.sgt(Result.getInt().getSExtValue())))
-          CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
-              << Result.getInt() << Min.getSExtValue() << Max.getSExtValue();
+          Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+                                       diag::warn_constexpr_unscoped_enum_out_of_range)
+         << llvm::toString(Result.getInt(),10) << Min.getSExtValue() << Max.getSExtValue();
         else if (!ED->getNumNegativeBits() &&
                  Max.ult(Result.getInt().getZExtValue()))
-          CCEDiag(E, diag::note_constexpr_unscoped_enum_out_of_range)
-              << Result.getInt() << Min.getZExtValue() << Max.getZExtValue();
+          Info.Ctx.getDiagnostics().Report(E->getExprLoc(),
+                                       diag::warn_constexpr_unscoped_enum_out_of_range)
+      << llvm::toString(Result.getInt(),10) << Min.getZExtValue() << Max.getZExtValue();
       }
     }
 
     return Success(HandleIntToIntCast(Info, E, DestType, SrcType,
                                       Result.getInt()), E);
   }
 
   case CK_PointerToIntegral: {

clang/test/SemaCXX/MicrosoftCompatibility.cpp

 
 template void function_missing_typename<D>(const D::Type param);
 
 }
 
 //MSVC allows forward enum declaration
 enum ENUM; // expected-warning {{forward references to 'enum' types are a Microsoft extension}}
 ENUM *var = 0;     
-ENUM var2 = (ENUM)3;
+ENUM var2 = (ENUM)0;
 enum ENUM1* var3 = 0;// expected-warning {{forward references to 'enum' types are a Microsoft extension}}
 
 enum ENUM1 { kA };
 enum ENUM1;  // This way round is fine.
 
 enum ENUM2 {
-  ENUM2_a = (enum ENUM2) 4,
+  ENUM2_a = (enum ENUM2) 0,
   ENUM2_b = 0x9FFFFFFF, // expected-warning {{enumerator value is not representable in the underlying type 'int'}}
   ENUM2_c = 0x100000000 // expected-warning {{enumerator value is not representable in the underlying type 'int'}}
 };
 
 namespace NsEnumForwardDecl {
   enum E *p; // expected-warning {{forward references to 'enum' types are a Microsoft extension}}
   extern E e;
 }

clang/test/SemaCXX/compare.cpp

void test13() {
     testx<A>();
     testx<B>();
   }
 }
 
 namespace tautological_enum {
   enum E { a, b, c } e;
 
-  // FIXME: We should warn about constructing this out-of-range numeration value.
-  const E invalid = (E)-1;
-  // ... but we should not warn about comparing against it.
-  bool x = e == invalid;
-
   // We should not warn about relational comparisons for enumerators, even if
   // they're tautological.
   bool y = e >= a && e <= b;
   const E first_in_range = a;
   const E last_in_range = b;
   bool z = e >= first_in_range && e <= last_in_range;
 }

clang/test/SemaCXX/constant-expression-cxx11.cpp

 enum EFixed : int {efixed1=-4, efixed2=4};
 // Enum with fixed underlying type because it is scoped
 enum class EScoped {escoped1=-4, escoped2=4};
 
 enum EMaxInt {emaxint1=-1, emaxint2=__INT_MAX__};
 
 void testValueInRangeOfEnumerationValues() {
   constexpr E1 x1 = static_cast<E1>(-8);
-  constexpr E1 x2 = static_cast<E1>(8); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 8 is outside the valid range of values [-8, 7] for this enumeration type}}
+  constexpr E1 x2 = static_cast<E1>(8);
+  // expected-error@-1 {{integer value 8 is outside the valid range of values [-8, 7] for this enumeration type}}
 
-  constexpr E2 x3 = static_cast<E2>(-8); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value -8 is outside the valid range of values [0, 7] for this enumeration type}}
+  constexpr E2 x3 = static_cast<E2>(-8);
+  // expected-error@-1 {{integer value -8 is outside the valid range of values [0, 7] for this enumeration type}}
   constexpr E2 x4 = static_cast<E2>(0);
-  constexpr E2 x5 = static_cast<E2>(8); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 8 is outside the valid range of values [0, 7] for this enumeration type}}
+  constexpr E2 x5 = static_cast<E2>(8);
+  // expected-error@-1 {{integer value 8 is outside the valid range of values [0, 7] for this enumeration type}}
 
   constexpr E3 x6 = static_cast<E3>(-2048);
   constexpr E3 x7 = static_cast<E3>(-8);
   constexpr E3 x8 = static_cast<E3>(0);
   constexpr E3 x9 = static_cast<E3>(8);
-  constexpr E3 x10 = static_cast<E3>(2048); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 2048 is outside the valid range of values [-2048, 2047] for this enumeration type}}
+  constexpr E3 x10 = static_cast<E3>(2048);
+  // expected-error@-1 {{integer value 2048 is outside the valid range of values [-2048, 2047] for this enumeration type}}
 
   constexpr E4 x11 = static_cast<E4>(0);
   constexpr E4 x12 = static_cast<E4>(1);
-  constexpr E4 x13 = static_cast<E4>(2); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 2 is outside the valid range of values [0, 1] for this enumeration type}}
+  constexpr E4 x13 = static_cast<E4>(2);
+  // expected-error@-1 {{integer value 2 is outside the valid range of values [0, 1] for this enumeration type}}
 
   constexpr EEmpty x14 = static_cast<EEmpty>(0);
   constexpr EEmpty x15 = static_cast<EEmpty>(1);
-  constexpr EEmpty x16 = static_cast<EEmpty>(2); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 2 is outside the valid range of values [0, 1] for this enumeration type}}
+  constexpr EEmpty x16 = static_cast<EEmpty>(2);
+  // expected-error@-1 {{integer value 2 is outside the valid range of values [0, 1] for this enumeration type}}
 
   constexpr EFixed x17 = static_cast<EFixed>(100);
   constexpr EScoped x18 = static_cast<EScoped>(100);
 
   constexpr EMaxInt x19 = static_cast<EMaxInt>(__INT_MAX__-1);
-  constexpr EMaxInt x20 = static_cast<EMaxInt>((long)__INT_MAX__+1); // expected-error {{must be initialized by a constant expression}}
-  // expected-note@-1 {{integer value 2147483648 is outside the valid range of values [-2147483648, 2147483647] for this enumeration type}}
+  constexpr EMaxInt x20 = static_cast<EMaxInt>((long)__INT_MAX__+1);
+  // expected-error@-1 {{integer value 2147483648 is outside the valid range of values [-2147483648, 2147483647] for this enumeration type}}
 }
 }