[PATCH] D143637: StackProtector: add unwind cleanup paths for instrumentation.

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added a comment.

I think I'd like to reapply this one.

The `__gxx_personality_v0` error only reproduces for me if I compile libunwind with `-fexceptions` (or perhaps more likely remove the LLVM default `-fno-exceptions`), and I think that's not something that should be possible. It conceptually introduces exactly that dependence from `libunwind` -> `libc++abi` lld is complaining about, which is in the wrong direction (and libunwind doesn't actually do exceptions anyway). I've been unable to get this to happen in an actual CMake build though, `LLVM_ENABLE_EH` doesn't get forwarded to the `LLVM_EANBLE_RUNTIMES` builds in my tests.

So if necessary I think we should add logic to libunwind to ensure `-fno-exceptions`, but I haven't updated the patch for now because I can't find a scenario where it triggers.

And, having read up a bit on ObjC lifetime semantics, I think the test-case there (and the Chromium one it's derived from, which is basically the same) is relying on lifetime optimizations that aren't actually guaranteed. In main:

  int main() {
    NSObject *p = create();
    __weak NSObject *q = p;
    @autoreleasepool {
      p = nil;
    }
    printf("p = %p\n", p);
    printf("q = %p\n", q); // Both p and q should be nil.
    return 0;
  }

`create` returns an object that's notionally got a retain count of 1 and a queued autorelease.

Before this change, the retain that was part of assigning to `p` cancelled out the autorelease (the call to `_objc_retainAutoreleasedReturnValue` was right after `create`) so it went into the `autoreleasepool` block with a count of 1 and nothing pending. Assigning `nil` decremented the refcount and freed the object (zeroing weak ref `q`).

After this change the retain actually happened (the invoke separated the call to `create` from `_objc_retainAutoreleasedReturnValue`). So instead so it went into the block with a refcount of 2 and an autorelease pending. Since the object wasn't deleted, the weak reference never got zeroed out (I assume the real runtime will drain all autoreleasing objects at some point, just not here). But it was only ever an opportunistic optimization.

It's notable that

1. `NSObject *p; p = create();` always failed to be optimized and `q` remains valid even now.
2. This is only at -O0: we actually make a stronger effort to ensure an `_objc_retainAutoreleasedReturnValue` stays with its call at higher levels so the optimization would still happen there.

So I think the Chromium test needs updating in this case (though I don't really know what it was trying to do so I'm not sure how). @hans, what do you think? Is that feasible? Do you disagree with the reasoning?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143637/new/

https://reviews.llvm.org/D143637