Fix crash on value dependent bitfields in if conditions in templates

[eandrews]

Summary

This patch is a follow up to D69950, to fix a new crash on CXX condition expressions in templates, when the condition is a bitfield with value-dependent width. Clang currently crashes when integral promotion is attempted on the condition. Name of bitfields with value-dependent width should be set as type-dependent. This patch adds the required value-dependency check and sets type-dependency accordingly.

[erichkeane]

I'm concerned about just making this fallthrough to 'false'. These ARE integral promotions, we just don't know the type size. What happens if you instantiate this template? Will it still give the correct answer/type/diagnosis?

In the case of the crash, I would suspect that the expression 'if (c)' should just be dependent and skipped from the semantic analysis because of it.

[eandrews]

Thanks for taking a look Erich!

I'm concerned about just making this fallthrough to 'false'. These ARE integral promotions, we just don't know the type size.

In this case, when integral promotion is skipped, boolean conversion (C++ 4.12) is done instead when parsing the template declaration.

What happens if you instantiate this template? Will it still give the correct answer/type/diagnosis?

I stepped through the code for an instantiated template and in this case, integral promotion is done since 'b' is no longer value dependent and is whatever we instantiated it with. I'm not sure what the correct behavior here is, so I compared current behavior to Clang prior to D69950 + this patch. The only difference I could see in the AST is an additional implicit cast for if(c) in template declaration (expected since D69950 changed the type dependency of c and this is the only guard). There is no difference in IR generated.

In the case of the crash, I would suspect that the expression 'if (c)' should just be dependent and skipped from the semantic analysis because of it.

It turns out skipping semantic analysis for value dependent condition expressions is not entirely straightforward. CheckBooleanCondition has a check to skip semantic analysis for type-dependent expressions. Adding a value dependency check here however causes crashes (lit tests) because apparently semantic analysis for noexcept calls this as well.

ExprResult Sema::ActOnNoexceptSpec(SourceLocation NoexceptLoc,
                                   Expr *NoexceptExpr,
                                   ExceptionSpecificationType &EST) {
  // FIXME: This is bogus, a noexcept expression is not a condition.
  ExprResult Converted = CheckBooleanCondition(NoexceptLoc, NoexceptExpr);

I don't really know how noexcept is handled in Clang but it looks like it expects the conversion of value dependent expressions. I guess I could add a guard in ActOnCond instead and see what happens.

[eandrews]

Semantic analysis for value dependent conditions is now skipped as per Erich's comment. Patch adds an argument to CheckBooleanCondition to still do the required analysis for noexcept expressions.

[rnk]

I think I liked the first version of this patch better. I would say, if the AST after instantiation remains the same as it was before D69950, then the first one seems like the right fix. The pattern AST will just be missing a node to represent the promotion from the bitfield to a full integer, and then again to a boolean.

[eandrews]

In D72242#1820908, @rnk wrote:

I think I liked the first version of this patch better. I would say, if the AST after instantiation remains the same as it was before D69950, then the first one seems like the right fix.

Thanks for taking a look Reid. The AST after instantiation (ClassTemplateSpecializationDecl) remains the same. I'll upload the old patch for review again after modifying test to add AST checks @erichkeane do you have any concerns?

[rsmith]

This is not an appropriate fix; the code is locally correct. The right way to handle this is exactly what Clang currently does: if the expression is not type-dependent, then contextually convert to bool, and if it's not value-dependent, then make sure it's a constant expression.

The bug is that we should treat the name of a bit-field with a value-dependent width as being type-dependent -- the c expression in the testcase should be a type-dependent expression (even though its type is known to be int) because we don't know how to promote it due to the value-dependent bit-width. (This is ultimately a bug in the C++ standard, which fails to provide such a rule, but I've reported that to the committee and it should be fixed in the standard at some point. But the right fix seems obvious enough that we can just do it.)

[eandrews]

Thanks for taking a look Richard. This patch adds the required value-dependency check and sets type-dependency accordingly.

[smeenai]

This also fixes https://bugs.llvm.org/show_bug.cgi?id=44886

[rnk]

lgtm

The fix looks like what Richard asked for, so I feel comfortable approving this. Thanks for the fix!

[smeenai]

CC @hans for the 10.0 branch.

[eandrews]

Thanks! I'll push it shortly. Just running a final ninja check-all after updating workspace.

[eandrews]

Actually I just noticed it still says 'Needs review' above. I'm not sure what the protocol is. Can I push the patch?

[smeenai]

I'm not sure why Phabricator is still showing Needs Review, but @rnk's approval should make this count as accepted.

[eandrews]

In D72242#1873017, @smeenai wrote:

I'm not sure why Phabricator is still showing Needs Review, but @rnk's approval should make this count as accepted.

Ok. Thank you. I've pushed the patch.

[hans]

In D72242#1872827, @smeenai wrote:

CC @hans for the 10.0 branch.

Cherry-picked to 10.x as 808f8a632f8bc12830157c57461ae8f848c566a3. Please let me know if there are any follow-ups.

clang/lib/AST/Expr.cpp

MemberExpr *MemberExpr::Create(
 
   if (isa<FieldDecl>(MemberDecl)) {
     DeclContext *DC = MemberDecl->getDeclContext();
     // dyn_cast_or_null is used to handle objC variables which do not
     // have a declaration context.
     CXXRecordDecl *RD = dyn_cast_or_null<CXXRecordDecl>(DC);
     if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
       E->setTypeDependent(T->isDependentType());
+
+    // Bitfield with value-dependent width is type-dependent.
+    FieldDecl *FD = dyn_cast<FieldDecl>(MemberDecl);
+    if (FD && FD->isBitField() && FD->getBitWidth()->isValueDependent())
+      E->setTypeDependent(true);
   }
 
   if (HasQualOrFound) {
     // FIXME: Wrong. We should be looking at the member declaration we found.
     if (QualifierLoc && QualifierLoc.getNestedNameSpecifier()->isDependent()) {
       E->setValueDependent(true);
       E->setTypeDependent(true);
       E->setInstantiationDependent(true);

clang/test/SemaTemplate/enum-argument.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
 
 enum Enum { val = 1 };
 template <Enum v> struct C {
   typedef C<v> Self;
 };
 template struct C<val>;
 
 template<typename T>

 namespace rdar8020920 {
   template<typename T>
   struct X {
     enum { e0 = 32 };
 
     unsigned long long bitfield : e0;
 
     void f(int j) {
-      bitfield + j; // expected-warning {{expression result unused}}
+      bitfield + j;
     }
   };
 }

clang/test/SemaTemplate/value-dependent-bitfield-cond.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template <int b>
+class A {
+  int c : b;
+
+public:
+  void f() {
+    if (c)
+      ;
+  }
+};