Page MenuHomePhabricator

[Sema] Fix crash on valid where instantiation of lambda cannot access type of 'this'
ClosedPublic

Authored by erik.pilkington on Jun 8 2016, 10:57 AM.

Details

Summary

Clang crashes during the instantiation of a lambda call operator where a VarDecl is initialized with an expression that requires the this type of the enclosing class, because when instantiating the initializing expression we introduce a new context which nulls out the Sema::CXXThisTypeOverride. This patch fixes the crash by retaining the 'this' type when the current lexical context is the body of the lambda.

This is a regression from 3.8 introduced in r267956. Fixes PR27994.

Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

erik.pilkington retitled this revision from to [Sema] Fix crash on valid where instantiation of lambda cannot access type of 'this'.
erik.pilkington updated this object.
erik.pilkington added a subscriber: cfe-commits.
faisalv added inline comments.Jun 11 2016, 6:49 AM
lib/Sema/SemaTemplateInstantiateDecl.cpp
3922 ↗(On Diff #60062)

I don't think this is the right fix. In my opinion, the fix should be to teach getCurrentThisType to recognize that we are in a context where a lambda is being 'transformed' and correctly return the type of 'this', regardless of the containing-context of the lambda. Also see the Richard-inspired FIXME to attempt to comprehensively handle such cases.

getCurrentThisType is broken though in some other ways currently (handling the cv qualification of lambda 'this' captures by value) - and i'm going to commit my patch to fix that later today. Let me know if you'd like me to work on this next, or would prefer to take a crack at it yourself.

Thanks for looking into this - apologies for the delay in feedback.

This new patch changes Sema::getCurrentThisType to recover the this type when a lambda initializer is being transformed. This is done with the same code as when instantiating a generic lambda initializer. I also added a loop that finds the enclosing record, in the case where we need to access the this type through a nested lambda.

Thanks for the feedback!

faisalv edited edge metadata.Jun 12 2016, 4:39 PM

Hmm - after having given this some more thought - I'm not as confident about the best approach. Should we leverage the CXXThisTypeOverride mechanism (as you had done in your initial patch) - and remove the computation entirely from getCurrentThisType - or just remove CXXThisTypeOverride entirely and have getCurrentThisType be smart enough to always figure out the 'this' type? Admittedly, my slight bias is to smarty-pantify getCurrentThisType (and just remove CXXThisTypeOverride if we can do it for all cases) - but I'd be interested in the counterarguments against that. Does it really make sense to leverage and maintain both approaches within the code-base?

Richard you have any thoughts on this (pun ;)?

Apologies Erik, for the waffle here - and thanks again for putting time into it.

lib/Sema/SemaExprCXX.cpp
971 ↗(On Diff #60477)

I wonder if we could just check that 'DC' is a CXXRecordDecl and just use it (so avoid computing it again below)?

erik.pilkington edited edge metadata.

This patch removes the handmade loop from Sema::getCurrentThisType instead just using the result of Sema::getFunctionLevelDeclContext. I believe that this will always give us the nearest CXXRecordDecl that we're looking for (right?).

For what it's worth, I agree with (Saturday) you that Sema::getCurrentThisType should be able to handle this case. That being said it seems at the very least quirky that int x; x = y; and int x = y; take an entirely different path through getCurrentThisType for y in a lambda initializer (the first using the CXXThisTypeOverride path).

@faisalv: Waffles are definitely not a problem, if that's what it takes to get the right fix! Thanks again for helping.

faisalv accepted this revision.Jun 26 2016, 6:46 AM
faisalv edited edge metadata.

With the assert, I would feel comfortable committing the patch. It always helps to get Richard's approval too - so feel free to ping him aggressively to see if he's interested in stopping it from being committed.

Also, I spoke with Richard & Hubert briefly at the Standards meeting in Oulu about dropping CXXThisTypeOverride - they're not sure whether we can entirely drop it (because of noexcept operands and trailing return types) without some other ugliness - but I'll try and investigate that at some point (unless you're interested in looking into it).

Thanks again Erik!

lib/Sema/SemaExprCXX.cpp
971 ↗(On Diff #60622)

I wouldn't mind an assertion here that 'DC' is a CXXRecordDecl if you think this branch can only be triggered if that's the case. Like you, I can't readily think of a case where getCurrentThisType would need to be called while 'transforming' or 'parsing' a lambda and getFunctionLevelDeclContext would not return either a CXXMethorDecl or a CXXRecordDecl (assuming ThisTy is null).

This revision is now accepted and ready to land.Jun 26 2016, 6:46 AM

Okay, thanks! I'll add the assert in.
@rsmith: Any thoughts on this?

This revision was automatically updated to reflect the committed changes.