Page MenuHomePhabricator

Fix over-release of return value of lambda implicitly converted to block/function pointer
ClosedPublic

Authored by danzimm on Dec 9 2017, 1:41 PM.

Details

Summary

Clang allows automatic conversion of lambdas to ObjC blocks and C function pointers (the latter only when nothing is captured). If a lambda returns an ObjC object with arc enabled then it gets over-released. You can see the bad behavior in the program below:

typedef id (^Go)();

int main() {
  Go g = []() -> id {
    return [NSObject new];
  };
  id sss = nil;
  @autoreleasepool {
    id ss = g();
    sss = ss;
  }
  NSLog(@"%@", sss);
  return 0;
}

(and if you change the Go typedef to be a function pointer then the same error occurs, which is a crash).

The test I wrote relies on the optimizer inlining the lambda invocation -- I'm not sure if that's an ok assumption to make. The IR generated previous to this patch is kind of fun:

define internal i8* @___Z5test0P11objc_object_block_invoke(i8* nocapture readonly %.block_descriptor) #0 {
entry:
  %block.capture.addr = getelementptr inbounds i8, i8* %.block_descriptor, i64 32
  %0 = bitcast i8* %block.capture.addr to i8**
  %1 = load i8*, i8** %0, align 8, !tbaa !7
  %2 = tail call i8* @objc_retain(i8* %1) #2
  %3 = tail call i8* @objc_autoreleaseReturnValue(i8* %1) #2
  %4 = tail call i8* @objc_autoreleaseReturnValue(i8* %1) #2
  ret i8* %1
}

Diff Detail

Event Timeline

danzimm created this revision.Dec 9 2017, 1:41 PM
trentxintong edited edge metadata.

@gottesmm can you please take a look ? Thanks.

gottesmm edited edge metadata.

I do not work on objcarc any longer. +CC Duncan.

dexonsmith resigned from this revision.Dec 10 2017, 10:34 AM
dexonsmith added a subscriber: dexonsmith.

Akira and/or John should take a look instead of me.

Alternative #1 is actually the right option — only it shouldn't be an objc_retain, it should be an objc_retainAutoreleasedReturnValue.

@rjmccall ah right, good point -- I think it's ok to elide that objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue pair in this case though, since all of this is generated within clang itself (and thus we can make the guarentee that the object will properly live until the return of the enclosing block) -- it reduces retain count churn this way too

Or do we need to abide by those semantics strictly here? Could you expand on why that is, if that's the case?

Or do we need to abide by those semantics strictly here? Could you expand on why that is, if that's the case?

The autoreleased-return-value optimization cannot be understood purely in terms of its impact on the retain count; it's a hand-off from the callee to the caller the requires a specific code-generation pattern in order to work.

Here, calling retainAutoreleasedRV in the block is necessary in order to prevent the return value from the lambda from actually being autoreleased. The block then owns a retain on the return value and must call autoreleaseRV in order to balance that out and transfer the object to its caller. The caller will hopefully also call retainAutoreleasedRV, thus completing the transfer of the return value all the way to the ultimate caller without actually performing any retains or releases.

John.

danzimm updated this revision to Diff 126628.Dec 12 2017, 2:28 PM

Call objc_retainAutoreleasedReturnValue after invoking a wrapped lambda

danzimm edited the summary of this revision. (Show Details)Dec 12 2017, 2:30 PM

@rjmccall aha, right - thanks for explaining that to me (sorry for the bad logic I had earlier).

It turns out this was also broken for a lambda that was auto-converted to a function pointer who returned an ObjC object. I changed the test case to reflect the more general situation, but I'm still unsure about the quality of the tests. Is it ok to rely on optimization passes to inline certain calls?

danzimm updated this revision to Diff 126633.Dec 12 2017, 2:35 PM

Remove unnecessary change of braces

danzimm retitled this revision from Fix over-release of return value of lambda implicitly converted to block to Fix over-release of return value of lambda implicitly converted to block/function pointer.Dec 12 2017, 2:49 PM

We do try to avoid relying on optimizer behavior in our tests. I don't know why you need that here.

danzimm updated this revision to Diff 126763.Dec 13 2017, 7:48 AM

Change tests to use non-O2 generated IR. It looks like the combined objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue calls annihilate each other and we just get a call/ret.

Ah, just found test/Transforms/ObjCARC/rv.ll test3:

; Delete a redundant retainRV,autoreleaseRV when forwaring a call result
; directly to a return value.

; CHECK-LABEL: define i8* @test3(
; CHECK: call i8* @returner()
; CHECK-NEXT: ret i8* %call
define i8* @test3() {
entry:
  %call = call i8* @returner()
  %0 = call i8* @objc_retainAutoreleasedReturnValue(i8* %call) nounwind
  %1 = call i8* @objc_autoreleaseReturnValue(i8* %0) nounwind
  ret i8* %1
}

Change tests to use non-O2 generated IR. It looks like the combined objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue calls annihilate each other and we just get a call/ret.

Is that really happening at -O0? Maybe you need to add -disable-llvm-optzns to the test line if so.

Change tests to use non-O2 generated IR. It looks like the combined objc_retainAutoreleasedReturnValue/objc_autoreleaseReturnValue calls annihilate each other and we just get a call/ret.

Is that really happening at -O0? Maybe you need to add -disable-llvm-optzns to the test line if so.

Unfortunately it looks like adding -disable-llvm-optzns and/or -disable-llvm-passes (I also manually added -O0 since I don't know what clang defaults to) doesn't generate the explicit call to objc_retainAutoreleasedReturnValue. Should we add a flag to disable that merging?

I just dug into how the ARC optimization pass is invoked... it really shouldn't be invoked if -disable-llvm-passes is passed (or -disable-llvm-optzns which appears to just alias the first). Can you verify that my command is sane:

/Users/danzimm/oss/build/bin/clang -cc1 -internal-isystem /Users/danzimm/oss/build/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-macosx10.12.0 -disable-llvm-passes -emit-llvm -fblocks -fobjc-arc -std=c++11 -O0 -o - /Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm
danzimm updated this revision to Diff 126892.Dec 13 2017, 8:20 PM

Pass -disable-llvm-passes with -O3 so that we can test that the IR we generate actually is generated

danzimm updated this revision to Diff 126893.Dec 13 2017, 8:24 PM

Remove unnecessary checks from tests (sorry for unbatched updates)

rjmccall accepted this revision.Dec 13 2017, 9:44 PM

Heh, alright, that works. It's unfortunate that -disable-llvm-passes doesn't suppress running the pass under -O0; that seems like an oversight.

Anyway, LGTM.

This revision is now accepted and ready to land.Dec 13 2017, 9:44 PM

Heh, alright, that works. It's unfortunate that -disable-llvm-passes doesn't suppress running the pass under -O0; that seems like an oversight.

Anyway, LGTM.

I suspect -O0 just generates different IR. You can confirm what passes are executed by adding -mllvm -debug-pass=Executions.

Oh, yes, that could be.

danzimm added a comment.EditedDec 14 2017, 6:50 AM

@dexonsmith Here are my results after passing those extra flags with -O3 (/Users/danzimm/oss/build/bin/clang -cc1 -internal-isystem /Users/danzimm/oss/build/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-macosx10.12.0 -emit-llvm -disable-llvm-passes -O3 -fblocks -fobjc-arc -fobjc-runtime-has-weak -std=c++11 -mllvm -debug-pass=Executions -o ~/foo.ll /Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm):

Pass Arguments:  -tti
Target Transform Information
  FunctionPass Manager
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager
    Print Module IR
[2017-12-14 09:47:12.252472000] 0x7f87b90016f0   Executing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
[2017-12-14 09:47:12.254702000] 0x7f87b90016f0    Freeing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager

and with -O0 (/Users/danzimm/oss/build/bin/clang -cc1 -internal-isystem /Users/danzimm/oss/build/lib/clang/6.0.0/include -nostdsysteminc -triple x86_64-apple-macosx10.12.0 -emit-llvm -disable-llvm-passes -O0 -fblocks -fobjc-arc -fobjc-runtime-has-weak -std=c++11 -mllvm -debug-pass=Executions -o ~/foo.ll /Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm):

Pass Arguments:  -tti
Target Transform Information
  FunctionPass Manager
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager
    Print Module IR
[2017-12-14 09:47:20.884147000] 0x7ff71ed097d0   Executing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
[2017-12-14 09:47:20.886182000] 0x7ff71ed097d0    Freeing Pass 'Print Module IR' on Module '/Users/danzimm/oss/llvm/tools/clang/test/CodeGenObjCXX/arc-forwarded-lambda-call.mm'...
Pass Arguments:  -tti
Target Transform Information
  ModulePass Manager

Also, I don't have commit access. Could someone else commit this for me?

rjmccall closed this revision.Dec 14 2017, 10:22 AM

Sure, r320721.