[Clang] Fix how we set the NumPositiveBits on an EnumDecl to cover the case of single enumerator with value zero or an empty enum

[shafik]

Summary

Currently in Sema::ActOnEnumBody(...) when calculating NumPositiveBits we miss the case where there is only a single enumerator with value zero and the case of an empty enum. In both cases we end up with zero positive bits when in fact we need one bit to store the value zero.

This PR updates the calculation to account for these cases.

Relevant C++ standard quotes are [dcl.enum]p7:

.., If the enumerator-list is empty, the underlying type is as if the enumeration had a single enumerator with value 0.

and [dcl.enum]p8:

... Otherwise, the values of the enumeration are the values representable by a hypothetical
integer type with minimal width M such that all enumerators can be represented. The width of the smallest
bit-field large enough to hold all the values of the enumeration type is M. ...

[shafik]

-Modified UBSan test to cover the empty enum case. I also refactored the test which was pretty clever but hard to work with as is.

[erichkeane]

clang/lib/Sema/SemaDecl.cpp

18907

What about:

std::max({NumPositiveBits, ActiveBits, 1}) (which uses the init-list version of std::max).

Also, not completely following this: BUT we don't need a positive bit IF we have a negative bit, right? So this is only necessary if BOTH NumPositiveBits and NumNegativeBits would have been 0?

18908

By coding standard, we need curleys around the 'else' as well if we have it on the 'if'.

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

10

can you do a test on:

enum ENeg { a = -1 }
?

That should ALSO have only a single bit.

[shafik]

clang/lib/Sema/SemaDecl.cpp

18907

Good call on the std::max alternative, I completely forgot about it.

If we look at the description of getNumPositiveBits() in Decl.h it says:

Returns the width in bits required to store all the non-negative enumerators of this enum.

So since 0 is non-negative we should update the positive bits. Make sense?

18908

Good catch.

[erichkeane]

My concern with the 'num positive bits' is really that enum EEmpty{};, enum EZero{ A =0 };, and enum ENeg {A = -1}; should all have 1 total bit of 'values'. So as long as THAT is true, I would expect this to be ok.

I fear that sticking to the 'non-negative' definition of NumPositiveBits might cause some oddball annoyances when trying to add this rule as a side-effect.

clang/lib/Sema/SemaDecl.cpp

18907

Hmm... ok then. As long as my test below ends up with the right values (that is, a value range of -1, 0 in the 1 case, and 0, 1 in the other).

[shafik]

• Addressed style comments
• Changed to use initializer_list version of std::max
• Added test to cover enum with -1 as sole enumerator value

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

10

Added this test and it indeed only has one bit.

[erichkeane]

1 more question, otherwise this looks right to me.

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

27

What does THIS come from? What value is unknown? Shouldn't the -1 be fine?

[aaron.ballman]

Requesting changes until we have an answer for the testing situation.

clang/lib/Sema/SemaDecl.cpp

18904–18905

Add trailing punctuation, removes top-level const.

18913

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

27

+1, I'm surprised by the <unknown> there, but also... neither e1 nor e2 are of type enum EBool!

[aaron.ballman]

Also, this could use a release note. :-)

[shafik]

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

27

So it looks like clang knows that the only valid values for a bool enum is 0 or 1 and it will mask the value accordingly see godbolt for example using argc : https://godbolt.org/z/ceb9hPno9

So that would explain why the original test used a unsigned char* in order to prompt the diagnostic.

Looking into the ubsan diagnostic it looks like it treats bool as an unknown value, separate from integer and float. It is not clear to me why it does this but fixing that feels outside the scope of this change since this was part of the original test.

[aaron.ballman]

LGTM (aside from some small nits that you can fix when landing).

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

27

Thanks for looking into it! I agree that fixing the ubsan diagnostic behavior is out of scope. That said, can you move the CHECK lines so that they come *after* the line of code being checked? I think that's what threw me for a loop regarding the EBool confusion I had.

[shafik]

• Adding release note
• Fixing comments
• Removing top level const on local variable
• Adjusting test so checks go after the line they are checking

[shafik]

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

27

Fixed, I did it like that to keep with the style of the original test but I see how it can be confusing.

clang/docs/ReleaseNotes.rst

 - Clang will now look through type sugar when checking a member function is a
   move assignment operator. Fixes `Issue 56456 <https://github.com/llvm/llvm-project/issues/56456>`_.
 - Fixed a crash when a variable with a bool enum type that has no definition
   used in comparison operators. Fixes `Issue 56560 <https://github.com/llvm/llvm-project/issues/56560>`_.
 - Fix that ``if consteval`` could evaluate to ``true`` at runtime because it was incorrectly
   constant folded. Fixes `Issue 55638 <https://github.com/llvm/llvm-project/issues/55638>`_.
 - Fixed incompatibility of Clang's ``<stdatomic.h>`` with MSVC ``<atomic>``.
   Fixes `MSVC STL Issue 2862 <https://github.com/microsoft/STL/issues/2862>`_.
+- Empty enums and enums with a single enumerator with value zero will be
+  considered to have one positive bit in order to represent the underlying
+  value. This effects whether we consider the store of the value one to be well
+  defined.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/lib/Sema/SemaDecl.cpp

void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
   for (unsigned i = 0, e = Elements.size(); i != e; ++i) {
     EnumConstantDecl *ECD =
       cast_or_null<EnumConstantDecl>(Elements[i]);
     if (!ECD) continue;  // Already issued a diagnostic.
 
     const llvm::APSInt &InitVal = ECD->getInitVal();
 
     // Keep track of the size of positive and negative values.
-    if (InitVal.isUnsigned() || InitVal.isNonNegative())
-      NumPositiveBits = std::max(NumPositiveBits,
-                                 (unsigned)InitVal.getActiveBits());
-    else
+    if (InitVal.isUnsigned() || InitVal.isNonNegative()) {
+      // If the enumerator is zero that should still be counted as a positive
+      // bit since we need a bit to store the value zero.
+      unsigned ActiveBits = InitVal.getActiveBits();
+      NumPositiveBits = std::max({NumPositiveBits, ActiveBits, 1u});
+    } else {
       NumNegativeBits = std::max(NumNegativeBits,
                                  (unsigned)InitVal.getMinSignedBits());
     }
+  }
+
+  // If we have have an empty set of enumerators we still need one bit.
+  // From [dcl.enum]p8
+  // If the enumerator-list is empty, the values of the enumeration are as if
+  // the enumeration had a single enumerator with value 0
+  if (!NumPositiveBits && !NumNegativeBits)
+    NumPositiveBits = 1;
 
   // Figure out the type that should be used for this enum.
   QualType BestType;
   unsigned BestWidth;
 
   // C++0x N3000 [conv.prom]p3:
   //   An rvalue of an unscoped enumeration type whose underlying
   //   type is not fixed can be converted to an rvalue of the first

clang/test/CodeGenCXX/pr12251.cpp

 // NO-STRICT-ENUMS: load i8, i8* %{{[^ ]*}}, align 1, !range
 // NO-STRICT-ENUMS-NOT: !range
 
 enum e1 { };
 e1 g1(e1 *x) {
   return *x;
 }
 // CHECK-LABEL: define{{.*}} i32 @_Z2g1P2e1
-// CHECK: ret i32 0
+// CHECK: ret i32 %0
 
 enum e2 { e2_a = 0 };
 e2 g2(e2 *x) {
   return *x;
 }
 // CHECK-LABEL: define{{.*}} i32 @_Z2g2P2e2
-// CHECK: ret i32 0
+// CHECK: ret i32 %0
 
 enum e3 { e3_a = 16 };
 e3 g3(e3 *x) {
   return *x;
 }
 // CHECK-LABEL: define{{.*}} i32 @_Z2g3P2e3
 // CHECK: load i32, i32* %x, align 4, !range [[RANGE_i32_0_32:![^ ]*]]
 

compiler-rt/test/ubsan/TestCases/Misc/enum.cpp

-// RUN: %clangxx -fsanitize=enum %s -O3 -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-PLAIN
-// RUN: %clangxx -fsanitize=enum -std=c++11 -DE="class E" %s -O3 -o %t && %run %t
-// RUN: %clangxx -fsanitize=enum -std=c++11 -DE="class E : bool" %s -O3 -o %t && not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-BOOL
+// RUN: %clangxx -fsanitize=enum %s -O3 -o %t && %run %t 2>&1 | FileCheck %s --check-prefix=CHECK
 
 // FIXME: UBSan fails to add the correct instrumentation code for some reason on
 // Windows.
 // XFAIL: windows-msvc
 
-enum E { a = 1 } e;
-#undef E
+enum E { a = 1 };
+enum class EClass { a = 1 };
+enum class EBool : bool { a = 1 } e3;
+enum EEmpty {};
+enum EMinus { em = -1 };
 
 int main(int argc, char **argv) {
-  // memset(&e, 0xff, sizeof(e));
-  for (unsigned char *p = (unsigned char*)&e; p != (unsigned char*)(&e + 1); ++p)
+  E e1 = static_cast<E>(0xFFFFFFFF);
+  EClass e2 = static_cast<EClass>(0xFFFFFFFF);
+  EEmpty e4 = static_cast<EEmpty>(1);
+  EEmpty e5 = static_cast<EEmpty>(2);
+  EMinus e6 = static_cast<EMinus>(1);
+  EMinus e7 = static_cast<EMinus>(2);
+
+  for (unsigned char *p = (unsigned char *)&e3; p != (unsigned char *)(&e3 + 1);
+       ++p)
     *p = 0xff;
 
-  // CHECK-PLAIN: error: load of value 4294967295, which is not a valid value for type 'enum E'
-  // FIXME: Support marshalling and display of enum class values.
-  // CHECK-BOOL: error: load of value <unknown>, which is not a valid value for type 'enum E'
-  return (int)e != -1;
+  return ((int)e1 != -1) & ((int)e2 != -1) &
+         // CHECK: error: load of value 4294967295, which is not a valid value for type 'E'
+         ((int)e3 != -1) & ((int)e4 == 1) &
+         // CHECK: error: load of value <unknown>, which is not a valid value for type 'enum EBool'
+         ((int)e5 == 2) & ((int)e6 == 1) &
+         // CHECK: error: load of value 2, which is not a valid value for type 'EEmpty'
+         ((int)e7 == 2);
+  // CHECK: error: load of value 2, which is not a valid value for type 'EMinus'
 }