On Sep 16, 2014, at 7:20 PM, John McCall <[email protected]> wrote:
> On Sep 16, 2014, at 3:56 PM, jahanian <[email protected]> wrote:
>> Hi John,
>>
>> This test case:
>>
>> id test_dict()
>> {
>> return @{
>> @"a": [](){},
>> @"b": [](){}
>> };
>> }
>>
>> Crashes in IRGen because ExprWithCleanups for the dummy BlockDecl we use to
>> facilitate lambda to block conversion
>> is at the wrong place.
>> This happens because a lambda expression is considered a full expression so
>> it calls MaybeCreateExprWithCleanups
>> (see Sema::BuildBlockForLambdaConversion).
>> which ends up adding the clean up code for the BlockDecl of the first lambda
>> to the expression for the 2nd lambda!
>>
>> Ideally, MaybeCreateExprWithCleanups should be called when the entire
>> dictionary literal is seen. But in this case,
>> this does not happen. Attached patch fixes this by making sure that after
>> ‘every’ parse of the ‘value’ expression
>> MaybeCreateExprWithCleanups is called. This, in effect, assumes that the
>> ‘value’ literal is a full expression.
>> I don’t know the ramification of this change in the common case as couple of
>> non-lambda literal tests broke.
>> (it now generates the none-exc version of call instead of exc versions in
>> couple of places).
>>
>> Please review. Let me know if you need additional info.
>
> Hmm. We’ve had a number of similar bugs in the past, and usually the fix is
> to enter a new expression evaluation context in the right place. The right
> place here would be around the emission of the copy-initialization
> expression, not around the array/dictionary elements. Doing that should be
> good enough to fix this in general, I think, but I’d like Doug and Richard to
> weigh in because there’s weirdness afoot.
>
> So, the interesting code here is in BuildBlockForLambdaConversion, at the
> bottom of SemaLambda.cpp. This function is used to implement the conversion
> of a lambda to a block pointer, which occurs (1) within the implementation of
> the non-standard conversion operator to block-pointer type that we add to all
> lambda types and (2) as a special case of applying that conversion when the
> source is a LiteralExpr. Let’s call case (2) the Literal Hack. The Literal
> Hack solves some problems with block memory management which would otherwise
> bite us, but it’s also the code path that’s causing this bug.
>
> ExprResult Sema::BuildBlockForLambdaConversion(SourceLocation
> CurrentLocation,
> SourceLocation ConvLocation,
> CXXConversionDecl *Conv,
> Expr *Src) {
> …
> ExprResult Init = PerformCopyInitialization(
> InitializedEntity::InitializeBlock(ConvLocation,
> Src->getType(),
> /*NRVO=*/false),
> CurrentLocation, Src);
> if (!Init.isInvalid())
> Init = ActOnFinishFullExpr(Init.get());
> ...
>
> This Init expression will become the copy-init expression in the block's
> capture of the lambda (its own capture), which is actually used in two
> different places. The first is that it’s emitted during the IR-generation of
> the block literal expression in order to initialize the corresponding capture
> field; the second is that it's emitted within the block’s copy-helper
> function in order to initialize the destination capture field. In this
> latter situation, IRGen first binds the address of all the captured local
> variables to the corresponding field in the block. It’s because of this
> second use that we generally want to enter a new expression evaluation
> context around emitting the copy-init expression of a capture, because an
> arbitrary class’s copy constructor might require arbitrary stuff in default
> arguments, including the killer cases that absolutely require proper tracking
> of cleanups. (Ordinary C++ temporaries themselves are not problematic
> because we can conservatively say that any expression might have C++ cleanups
> without causing any lasting harm.)
>
> Note that the Src expression parameter to BuildBlockForLambdaConversion
> becomes a part of this Init expression, and it’s already been emitted; in
> the normal case, we could just pass in a std::function to generate it, but in
> the Literal Hack case we can’t, because we don’t know that we’re doing the
> Literal Hack until after we’ve fully parsed that expression and the enclosing
> expression and decided that we need to use the conversion operator. So if
> Src could be an arbitrary expression, we’d be in deep trouble, and I think
> we’d need to split the two use cases and maintain two separate copy-init
> expressions.
>
> Fortunately, it can’t. In the normal case it’ll be a DeclRefExpr, and in the
> Literal Hack case it’ll be a LambdaExpr, which has some arbitrarily complex
> sub-expressions to do capturing but which wraps them all up as
> full-expressions. So I think we can just wrap the construction in an
> EnterExpressionEvaluationContext, flag that there are cleanups in the new
> context if the lambda type has a non-trivial destructor, and then bind that
> up as its own full-expression.
>
> This is slightly wrong from a modeling perspective because it will put the
> LambdaExpr inside an ExprWithCleanups, which in principle limits the lifetime
> of that temporary too much, but the generated code has the correct semantics
> because we’ll elide the lambda temporary directly into the block’s capture
> field, and the block's lifetime is at least as long as the lambda temporary’s.
>
> So I think the right fix is basically:
>
> ExprResult Init;
> {
> EnterExpressionEvaluationContext evalContext(…);
> ExprHasCleanups = !Lambda->hasTrivialDestructor();
> Init = PerformCopyInitialization(…);
> if (!Init.isInvalid())
> Init = ActOnFinishFullExpr(Init.get());
> }
>
> But I’d like to know if I’m missing some way in which arbitrary subexpression
> structure can show up in Src.
Yeh. This change (with minor correction as attached) has the desired effect of
postponing cleanup code for the Literal hack until the outer-most expression is
seen. But, I wait if there are more comments.
Index: lib/Sema/SemaLambda.cpp
===================================================================
--- lib/Sema/SemaLambda.cpp (revision 217960)
+++ lib/Sema/SemaLambda.cpp (working copy)
@@ -1572,14 +1572,20 @@
CallOperator->setReferenced();
CallOperator->markUsed(Context);
- ExprResult Init = PerformCopyInitialization(
- InitializedEntity::InitializeBlock(ConvLocation,
- Src->getType(),
- /*NRVO=*/false),
- CurrentLocation, Src);
- if (!Init.isInvalid())
- Init = ActOnFinishFullExpr(Init.get());
+ ExprResult Init;
+ {
+ EnterExpressionEvaluationContext evalContext(*this,
Sema::PotentiallyEvaluated);
+ ExprNeedsCleanups = !Lambda->hasTrivialDestructor();
+ Init = PerformCopyInitialization(
+ InitializedEntity::InitializeBlock(ConvLocation,
+ Src->getType(),
+ /*NRVO=*/false),
+ CurrentLocation, Src);
+ if (!Init.isInvalid())
+ Init = ActOnFinishFullExpr(Init.get());
+ }
+
if (Init.isInvalid())
return ExprError();
Index: test/CodeGenObjCXX/lambda-in-objc-literals.mm
===================================================================
--- test/CodeGenObjCXX/lambda-in-objc-literals.mm (revision 0)
+++ test/CodeGenObjCXX/lambda-in-objc-literals.mm (working copy)
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10.0.0 -emit-llvm -o - %s
-fexceptions -std=c++11 -fblocks
+// rdar://16879958
+
+@protocol NSCopying @end
+typedef unsigned long NSUInteger;
+typedef long NSInteger;
+
+@interface NSDictionary
++ (id)dictionaryWithObjects:(const id [])objects forKeys:(const id <NSCopying>
[])keys count:(NSUInteger)cnt;
+- (void)setObject:(id)object forKeyedSubscript:(id)key;
+- (id)objectForKeyedSubscript:(id)key;
++ (id) alloc;
+- (id)initWithObjects:(const id [])objects forKeys:(const id [])keys
count:(unsigned long)cnt;
+@end
+
+@interface NSString<NSCopying>
+@end
+
+
+@class NSFastEnumerationState;
+
+@protocol NSFastEnumeration
+
+- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state
objects:(id [])buffer count:(NSUInteger)len;
+
+@end
+
+@interface NSNumber
++ (NSNumber *)numberWithInt:(int)value;
+@end
+
+@interface NSArray <NSFastEnumeration>
++ (id) alloc;
++ (id)arrayWithObjects:(const id [])objects count:(NSUInteger)cnt;
+@end
+
+id test_dict()
+{
+ return @{
+ @"a": [](){},
+ @"b": [](){},
+ @"c": [](){}
+ };
+}
+
+id test_arr()
+{
+ return @[
+ [](){},
+ [](){}
+ ];
+}
- Thanks, Fariborz
>
> John.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
