Begin forwarded message:
>>
>> 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 @[
+ [](){},
+ [](){}
+ ];
+}
>
>
If there is no additional comment or feedback, I would like to check this in.
> - Thanks, Fariborz
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
