[Sema] Avoid crash in CheckEnumConstant with contains-error expressions

[sammccall]

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=51554

[hokein]

Thanks!

clang/lib/Sema/SemaDecl.cpp

17817

What's happening during the crash:

• the EltTy is an enum type which points to an incomplete enum decl
• the EnumDecl::getIntegerType() returns a null type
• ASTContext.getIntWidth crashes on a null type

As discussed, an alternative is to add a guard for incomplete-type EltTy (and assert that we only see this case for recovery-expr case), a disadvantage is that this might be narrow...

The current solution also seems fine to me.

clang/test/Sema/enum.cpp

1 ↗

(On Diff #367731)

I'd put the test case in clang/test/SemaCXX/recovery-expr-type.cpp

[guopeilin]

Hi, @sammccall thanks for the patch.
The precommit checks suggest that some test cases failing, could you please fix them. Thanks

[aaron.ballman]

clang/test/Sema/enum.cpp

1 ↗

(On Diff #367731)

+1 -- Sema is for Cish things and SemaCXX is for C++ish things (in general).

8–9 ↗

(On Diff #367731)

Should we be static asserting this? It seems like emergent behavior more than something we intentionally want anyone to rely on.

[sammccall]

address comments

[sammccall]

Thanks, and sorry for sitting on this so long.

Addressed comments. I think the failing windows bots were implicit -fms-extensions or so that sometimes makes diagnosis more lazy. Moving the test to recovery-expr-type.cpp should take care of this as that test case specifies -triple. But waiting for the windows bot to finish before landing.

clang/test/Sema/enum.cpp

8–9 ↗

(On Diff #367731)

Yeah, the thing we're testing is that we can const-evaluate these, and it neither crashes nor propagates dependence further. The static_assert checks the latter.

Replaced with a static assert that isn't sensitive tot he actual value.

clang/lib/Sema/SemaDecl.cpp

EnumConstantDecl *Sema::CheckEnumConstant(EnumDecl *Enum,
 
   if (Val && DiagnoseUnexpandedParameterPack(Val, UPPC_EnumeratorValue))
     Val = nullptr;
 
   if (Val)
     Val = DefaultLvalueConversion(Val).get();
 
   if (Val) {
-    if (Enum->isDependentType() || Val->isTypeDependent())
+    if (Enum->isDependentType() || Val->isTypeDependent() ||
+        Val->containsErrors())
       EltTy = Context.DependentTy;
     else {
       // FIXME: We don't allow folding in C++11 mode for an enum with a fixed
       // underlying type, but do allow it in all other contexts.
       if (getLangOpts().CPlusPlus11 && Enum->isFixed()) {
         // C++11 [dcl.enum]p5: If the underlying type is fixed, [...] the
         // constant-expression in the enumerator-definition shall be a converted
         // constant expression of the underlying type.

clang/test/SemaCXX/recovery-expr-type.cpp

 } // namespace test11
 
 namespace test12 {
 // Verify we do not crash.
 int fun(int *foo = no_such_function()); // expected-error {{undeclared identifier}}
 void crash1() { fun(); }
 void crash2() { constexpr int s = fun(); }
 } // namespace test12
+
+namespace test13 {
+enum Circular {             // expected-note {{not complete until the closing '}'}}
+  Circular_A = Circular(1), // expected-error {{'test13::Circular' is an incomplete type}}
+};
+// Enumerators can be evaluated (they evaluate as zero, but we don't care).
+static_assert(Circular_A == 0 && Circular_A != 0, ""); // expected-error {{static_assert failed}}
+}