[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

Wed Jan 8 08:09:32 PST 2020

eandrews added a comment.

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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72242/new/

https://reviews.llvm.org/D72242