Page MenuHomePhabricator

Fix crash on value dependent bitfields in if conditions in templates
ClosedPublic

Authored by eandrews on Jan 5 2020, 7:37 PM.

Details

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.

Diff Detail

Event Timeline

eandrews created this revision.Jan 5 2020, 7:37 PM

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 added a comment.EditedJan 8 2020, 8:06 AM

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 updated this revision to Diff 237132.Jan 9 2020, 11:34 AM
eandrews edited the summary of this revision. (Show Details)

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 added a comment.Jan 14 2020, 4:07 PM

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.

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 requested changes to this revision.Jan 23 2020, 6:58 PM
rsmith added a subscriber: 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.)

This revision now requires changes to proceed.Jan 23 2020, 6:58 PM
eandrews updated this revision to Diff 243049.Thu, Feb 6, 4:50 PM
eandrews edited the summary of this revision. (Show Details)

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

rnk accepted this revision.Wed, Feb 12, 11:10 AM

lgtm

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

smeenai added a subscriber: hans.Wed, Feb 12, 11:20 AM

CC @hans for the 10.0 branch.

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

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

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

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.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Feb 12, 1:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Feb 12, 1:38 PM
hans added a comment.Thu, Feb 13, 12:32 AM

CC @hans for the 10.0 branch.

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