Page MenuHomePhabricator

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

Authored by shafik on Fri, Aug 5, 6:02 PM.

Details

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.

Diff Detail

Event Timeline

shafik created this revision.Fri, Aug 5, 6:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Aug 5, 6:02 PM
shafik requested review of this revision.Fri, Aug 5, 6:02 PM
shafik added inline comments.Fri, Aug 5, 6:05 PM
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.

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 added a comment.Sat, Aug 6, 5:40 PM

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.

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.

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?

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 added a comment.Mon, Aug 8, 6:33 AM

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

aaron.ballman accepted this revision.Mon, Aug 8, 6:38 AM

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!

This revision is now accepted and ready to land.Mon, Aug 8, 6:38 AM

All right, SGTM

This revision was landed with ongoing or failed builds.Mon, Aug 8, 4:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Aug 8, 4:23 PM
ronlieb added a subscriber: ronlieb.Mon, Aug 8, 4:35 PM

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

shafik added a comment.Mon, Aug 8, 5:26 PM

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.

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

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

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?

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 added a subscriber: zatrazz.Tue, Aug 9, 5:57 AM

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

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.

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.

sberg added a subscriber: sberg.Wed, Aug 10, 8:52 AM

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

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.