[clang] Assert the enum FPOpts and LangOpts fit into the storage

[fmayer]

Fix existing failure

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Florian Mayer (fmayer)

Changes

Fix existing failure

---

Full diff: https://github.com/llvm/llvm-project/pull/126166.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/FPOptions.def (+1-1)
• (modified) clang/include/clang/Basic/LangOptions.def (+1-1)
• (modified) clang/include/clang/Basic/LangOptions.h (+7-3)

diff --git a/clang/include/clang/Basic/FPOptions.def b/clang/include/clang/Basic/FPOptions.def
index 79f04c89c9fedc..90428c3c73c8b5 100644
--- a/clang/include/clang/Basic/FPOptions.def
+++ b/clang/include/clang/Basic/FPOptions.def
@@ -28,5 +28,5 @@ OPTION(FPEvalMethod, LangOptions::FPEvalMethodKind, 2, AllowApproxFunc)
 OPTION(Float16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, FPEvalMethod)
 OPTION(BFloat16ExcessPrecision, LangOptions::ExcessPrecisionKind, 2, Float16ExcessPrecision)
 OPTION(MathErrno, bool, 1, BFloat16ExcessPrecision)
-OPTION(ComplexRange, LangOptions::ComplexRangeKind, 2, MathErrno)
+OPTION(ComplexRange, LangOptions::ComplexRangeKind, 3, MathErrno)
 #undef OPTION
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index cb55f09acc076c..bfab0baa089cf7 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -238,7 +238,7 @@ BENIGN_LANGOPT(NoSignedZero      , 1, 0, "Permit Floating Point optimization wit
 BENIGN_LANGOPT(AllowRecip        , 1, 0, "Permit Floating Point reciprocal")
 BENIGN_LANGOPT(ApproxFunc        , 1, 0, "Permit Floating Point approximation")
 
-ENUM_LANGOPT(ComplexRange, ComplexRangeKind, 2, CX_None, "Enable use of range reduction for complex arithmetics.")
+ENUM_LANGOPT(ComplexRange, ComplexRangeKind, 3, CX_None, "Enable use of range reduction for complex arithmetics.")
 
 BENIGN_LANGOPT(ObjCGCBitmapPrint , 1, 0, "printing of GC's bitmap layout for __weak/__strong ivars")
 
diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index f58a719a45a84d..95c1555179781d 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -648,9 +648,12 @@ class LangOptions : public LangOptionsBase {
 
   // Define accessors/mutators for language options of enumeration type.
 #define LANGOPT(Name, Bits, Default, Description)
-#define ENUM_LANGOPT(Name, Type, Bits, Default, Description) \
-  Type get##Name() const { return static_cast<Type>(Name); } \
-  void set##Name(Type Value) { Name = static_cast<unsigned>(Value); }
+#define ENUM_LANGOPT(Name, Type, Bits, Default, Description)                   \
+  Type get##Name() const { return static_cast<Type>(Name); }                   \
+  void set##Name(Type Value) {                                                 \
+    assert(static_cast<unsigned>(Value) < (1u << Bits));                       \
+    Name = static_cast<unsigned>(Value);                                       \
+  }
 #include "clang/Basic/LangOptions.def"
 
   /// Are we compiling a module?
@@ -964,6 +967,7 @@ class FPOptions {
     return static_cast<TYPE>((Value & NAME##Mask) >> NAME##Shift);             \
   }                                                                            \
   void set##NAME(TYPE value) {                                                 \
+    assert(storage_type(value) < (1u << WIDTH));                               \
     Value = (Value & ~NAME##Mask) | (storage_type(value) << NAME##Shift);      \
   }
 #include "clang/Basic/FPOptions.def"

[fmayer]

@zahiraam the default of the ComplexRange, CX_None does overflow the 2 storage bits. That means we were probably still using CX_Full (4 should overflow to 0) even after #78330 (I am not sure why why that changed the default in the first place). Also it's impossible to manually choose CX_None.

[zahiraam]

@zahiraam the default of the ComplexRange, CX_None does overflow the 2 storage bits. That means we were probably still using CX_Full (4 should overflow to 0) even after #78330 (I am not sure why why that changed the default in the first place). Also it's impossible to manually choose CX_None.

The only valid values for the -fcomplex-arithmetic option are: CX_Full, CX_Improved, CX_Promoted and CX_Basic.
Using clang.exe -fcomplex-arithmetic=none t.c would lead to this error:
clang: error: unsupported argument 'none' to option '-fcomplex-arithmetic='
So 2 bits of storage is enough for ComplexRange. ComplexRange values can only be 4 values above.
The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command line. https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L2989 but it's not a valid option for fcomplex-arithmetic.

[fmayer]

@zahiraam the default of the ComplexRange, CX_None does overflow the 2 storage bits. That means we were probably still using CX_Full (4 should overflow to 0) even after #78330 (I am not sure why why that changed the default in the first place). Also it's impossible to manually choose CX_None.

The only valid values for the -fcomplex-arithmetic option are: CX_Full, CX_Improved, CX_Promoted and CX_Basic. Using clang.exe -fcomplex-arithmetic=none t.c would lead to this error: clang: error: unsupported argument 'none' to option '-fcomplex-arithmetic=' So 2 bits of storage is enough for ComplexRange. ComplexRange values can only be 4 values above. The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command line. https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L2989 but it's not a valid option for fcomplex-arithmetic.

Then why does this assertion trigger if I don't pass anything?

[zahiraam]

@zahiraam the default of the ComplexRange, CX_None does overflow the 2 storage bits. That means we were probably still using CX_Full (4 should overflow to 0) even after #78330 (I am not sure why why that changed the default in the first place). Also it's impossible to manually choose CX_None.

The only valid values for the -fcomplex-arithmetic option are: CX_Full, CX_Improved, CX_Promoted and CX_Basic. Using clang.exe -fcomplex-arithmetic=none t.c would lead to this error: clang: error: unsupported argument 'none' to option '-fcomplex-arithmetic=' So 2 bits of storage is enough for ComplexRange. ComplexRange values can only be 4 values above. The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command line. https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L2989 but it's not a valid option for fcomplex-arithmetic.

Then why does this assertion trigger if I don't pass anything?

Not sure I understand your question?
Which assertion?
clang.exe t.c is triggering an assertion?

[fmayer]

Not sure I understand your question? Which assertion? clang.exe t.c is triggering an assertion?

I don't understand why you say

The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command lin

but imply it does not need to be stored in the bitfield. If you patch this CL, and change the size for ComplexRange back, you will see it that it fails the assertion I added if you run the clang test suite.

[zahiraam]

Not sure I understand your question? Which assertion? clang.exe t.c is triggering an assertion?

I don't understand why you say

The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command lin

but imply it does not need to be stored in the bitfield. If you patch this CL, and change the size for ComplexRange back, you will see it that it fails the assertion I added if you run the clang test suite.

OK. I see the issue. Thanks.
I think this should be titled [NFC][Clang] YOUR TITLE.
If you don't want to make it NFC then you need to add a LIT test :-) Not sure you can make it fail with a LIT test.

[fmayer]

Not sure I understand your question? Which assertion? clang.exe t.c is triggering an assertion?

I don't understand why you say

The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command lin

but imply it does not need to be stored in the bitfield. If you patch this CL, and change the size for ComplexRange back, you will see it that it fails the assertion I added if you run the clang test suite.

OK. I see the issue. Thanks. I think this should be titled [NFC][Clang] YOUR TITLE. If you don't want to make it NFC then you need to add a LIT test :-) Not sure you can make it fail with a LIT test.

What do you mean? Of course you can't fail the assertion now that the offending code is fixed.

[zahiraam]

Not sure I understand your question? Which assertion? clang.exe t.c is triggering an assertion?

I don't understand why you say

The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command lin

but imply it does not need to be stored in the bitfield. If you patch this CL, and change the size for ComplexRange back, you will see it that it fails the assertion I added if you run the clang test suite.

OK. I see the issue. Thanks. I think this should be titled [NFC][Clang] YOUR TITLE. If you don't want to make it NFC then you need to add a LIT test :-) Not sure you can make it fail with a LIT test.

What do you mean? Of course you can't fail the assertion now that the offending code is fixed.

I added your patch to my code base (added only the assert, didn't change the size of storage for ComplexRange) and was able to confirm that the assertions go off. So you are right.
I am agreeing to merge this patch.

However the rule is that every patch needs at least one LIT test that illustrates the issue and prove that your patch fixes it. You don't have a LIT test in your patch. So, you can either make this patch NFC by adding it in your title or you need to produce a LIT test.

[fmayer]

Not sure I understand your question? Which assertion? clang.exe t.c is triggering an assertion?

I don't understand why you say

The value CX_None is the defaut value given to the Range when no fcomplex-arithmetic is used on the command lin

but imply it does not need to be stored in the bitfield. If you patch this CL, and change the size for ComplexRange back, you will see it that it fails the assertion I added if you run the clang test suite.

OK. I see the issue. Thanks. I think this should be titled [NFC][Clang] YOUR TITLE. If you don't want to make it NFC then you need to add a LIT test :-) Not sure you can make it fail with a LIT test.

What do you mean? Of course you can't fail the assertion now that the offending code is fixed.

I added your patch to my code base (added only the assert, didn't change the size of storage for ComplexRange) and was able to confirm that the assertions go off. So you are right. I am agreeing to merge this patch.

However the rule is that every patch needs at least one LIT test that illustrates the issue and prove that your patch fixes it. You don't have a LIT test in your patch. So, you can either make this patch NFC by adding it in your title or you need to produce a LIT test.

Just adding the assert makes a whopping 20594 out of 21977 tests fail. Does it really need to be 20595? If you want to test the functionality without the assert, be my guest, but I don't think that's a useful use of time.

This patch is not NFC because it fixes buggy behaviour. If you look closely, your None value actually overflowed into unrelated bits for the FpOpts, causing who knows what behavior to change.

[jansvoboda11]

The idea here makes sense to me and I don't mind if it lands without "NFC" in the title, but I think enforcing this at compile time would be more robust. Could we use static_assert with something like llvm::BitWidth and supporting infrastructure from llvm/include/ADT/BitmaskEnum.h?

[fmayer]

The idea here makes sense to me and I don't mind if it lands without "NFC" in the title, but I think enforcing this at compile time would be more robust. Could we use static_assert with something like llvm::BitWidth and supporting infrastructure from llvm/include/ADT/BitmaskEnum.h?

That would involve annotating all the enums with the largest value, and that's a bigger change, and also prone to error, so even if we did this the assert here is a good idea. We can do this later.

[jansvoboda11]

Ok, makes sense to fix the immediate problem now and follow up with the more robust fix later. Would be good if you could leave a FIXME on one of the assert calls in LangOptions.h outlining the potential improvement via static_assert. LGTM