[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D32210#810292, @ahatanak wrote:

> Address review comments.
>
> - Allow attaching "noescape" to pointers other than block pointers. Update the docs accordingly.
> - Attach attribute "nocapture" to parameters that are annotated with "noescape".
> - Call Sema::isValidPointerAttrType to determine whether "noescape" can be applied to a parameter. Also, use the existing warning "warn_attribute_pointers_only" rather than defining a new warning that will be used just for noescape.
>
>   I also thought about what else we can do in the front-end when a parameter has 'noescape".  One idea is to do something similar to what r301667 did and omit emitting retains and releases of block captures when the block is passed to a function taking a noescape parameter. If that is viable, I can look into it after committing this patch.


Hmm.  Unfortunately, I'm not sure that's valid.  The retains and releases of block captures don't protect against anything related to escaping the block; they protect against the original variables being modified during the lifetime of the block.  It is true that a block literal passed to a noescape parameter has a shorter effective lifetime — we know for certain that it will be unused after the call, and ARC only promises that a value in an imprecise-lifetime strong variable like a block capture will be valid until the last load of that value from the variable.  But that duration is still long enough for someone to modify the original variable in a way that is properly sequenced after the formation of the block.

Now, if you could prove that the variable was not modified for the duration of the call, that would be sufficient.  And that would be easy to do in the common case by just proving that the address of the variable never escapes.  Unfortunately, we don't have that information readily available because Sema doesn't collect it.

There are some other ways you could optimize blocks that are known not to escape, though.  One big caveat is that you have to make sure the block behaves reasonably in response to being copied, because being noescape doesn't guarantee that the callee won't try to copy the block.  However:

- Copying a global block is always a no-op.  If you gave the non-escaping stack block a global block isa, that would allow the blocks runtime to avoid doing extra work when a non-escaping block is spuriously copied, and it would allow the compiler to completely avoid emitting copy and destroy helpers for the block.  Please clear this with Greg Parker first, though.
- Even if that's not possible, you can still avoid emitting copy and destroy helpers in the common case where none of the captures are address-sensitive, i.e. __weak references or non-trivally-copyable C++ types, because the memcpy from the original block will be sufficient for correctness.
- You can just capture a reference to an outer block instead of copying anything that it captures.
- __block variables which are only captured by non-escaping blocks are themselves known not to escape.  (But remember that a non-escaping block can create an escaping block that captures the __block variable!)  Because __block variable copies are only ever kicked off by block copy helpers, and you won't be generating those (or at least won't be asking them to copy your __block variables for you), you also know that such variables can't be copied.  That in turn means you can completely drop all the nonsense that only exists to supporting the lazy copying of __block variables, like the __block header and copy/destroy helpers and forwarding.  Just generate the variable like a normal local variable and capture it by reference.

John.



================
Comment at: include/clang/AST/Type.h:3156
     };
     unsigned char Data;
 
----------------
Oh!  I hadn't noticed that you were adding this to ExtParameterInfo.  You're modifying the function type system; there's a lot of complexity to do that properly which you haven't done in this patch at all.  That's especially true because, unlike all these other ExtParameterInfo cases, there's a natural subtyping rule for escaping parameters: a function type that takes a non-escaping parameter should be implicitly convertible to be a function type that takes an escaping parameter.  You will also need to update a bunch of things, including the type printer, the mangler, the C++ function conversion rules, the C type compatibility rules, the redeclaration type matcher, and maybe even template argument deduction.  It's a big thing to do.

You will want Sema tests in C, ObjC, C++, and ObjC++.


================
Comment at: lib/CodeGen/CGCall.cpp:2096
+    if (FI.getExtParameterInfo(ArgNo).isNoEscape())
+      Attrs.addAttribute(llvm::Attribute::NoCapture);
+
----------------
You should make sure that building a CGFunctionInfo with an ObjCMethodDecl* collects this from the parameters.  (And add tests!)

For blocks, at least, the ObjC method case is arguably more important than the C function case.


https://reviews.llvm.org/D32210