r177819 - Make clang to mark static stack allocations with lifetime markers to enable a more aggressive stack coloring.

[John McCall]

On Mar 26, 2013, at 7:55 AM, Alexey Samsonov <samsonov at google.com> wrote:
> On Mon, Mar 25, 2013 at 11:23 PM, Alexey Samsonov <samsonov at google.com> wrote:
> Hi Nadav!
> 
> Cool you guys are working on it! I have a raw local patch that does the same (emits llvm.lifetime start/end intrinsics), and
> I hope to provide some feedback tomorrow, when I look at this commit in more details. Some of my concerns:
> 1) Rafael mentioned a thread where I asked about llvm.lifetime intrinsics semantics and behavior - you may be
> interested in it. I think we may resurrect it and clarify their semantics.
> 2) I'm afraid that immediate enabling of stack coloring may break things - I've seen that similar gcc option
> had to be disabled on some codebases because of the bugs in the code where a local variable was used after
> it went out of scope.
> 3) I actually want to emit lifetime.start/end intrinsics in one of AddressSanitizer modes (to implement -fsanitize=use-after-scope
> option) to reliably detect bugs in (2).
> 
> 
> 
> On Sat, Mar 23, 2013 at 10:43 AM, Nadav Rotem <nrotem at apple.com> wrote:
> Author: nadav
> Date: Sat Mar 23 01:43:35 2013
> New Revision: 177819
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=177819&view=rev
> Log:
> Make clang to mark static stack allocations with lifetime markers to enable a more aggressive stack coloring.
> Patch by John McCall with help by Shuxin Yang.
> rdar://13115369
> 
> 
> Added:
>     cfe/trunk/test/CodeGen/lifetime2.c
> Modified:
>     cfe/trunk/lib/CodeGen/CGDecl.cpp
>     cfe/trunk/lib/CodeGen/CGStmt.cpp
>     cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
>     cfe/trunk/lib/CodeGen/CodeGenFunction.h
>     cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>     cfe/trunk/lib/CodeGen/CodeGenModule.h
>     cfe/trunk/test/CodeGenObjC/arc-blocks.m
>     cfe/trunk/test/CodeGenObjC/arc.m
> 
> Modified: cfe/trunk/lib/CodeGen/CGDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDecl.cpp?rev=177819&r1=177818&r2=177819&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGDecl.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDecl.cpp Sat Mar 23 01:43:35 2013
> @@ -452,6 +452,22 @@ namespace {
>        CGF.EmitCall(FnInfo, CleanupFn, ReturnValueSlot(), Args);
>      }
>    };
> +
> +  /// A cleanup to call @llvm.lifetime.end.
> +  class CallLifetimeEnd : public EHScopeStack::Cleanup {
> +    llvm::Value *Addr;
> +    llvm::Value *Size;
> +  public:
> +    CallLifetimeEnd(llvm::Value *addr, llvm::Value *size)
> +      : Addr(addr), Size(size) {}
> +
> +    void Emit(CodeGenFunction &CGF, Flags flags) {
> +      llvm::Value *castAddr = CGF.Builder.CreateBitCast(Addr, CGF.Int8PtrTy);
> +      CGF.Builder.CreateCall2(CGF.CGM.getLLVMLifetimeEndFn(),
> +                              Size, castAddr)
> 
> I don't think you need getLLVMLifetimeEndFn() methods, lazy declarations of llvm.lifetime intrinsics etc.

It's not strictly necessary, no, but that doesn't mean it's not better to cache them.  Some code can end up fetching these declarations a *lot*.

> You can use CGF.Builder.CreateLifetimeEnd(llvm::AllocaInst *AI, llvm::ConstantInt *AllocaSize)
> and same for CGF.Builder.CreateLifetimeStart(...).
> It will automatically do a sanity check: your Addr should in fact come from AllocaInst, and AllocaSize should be constant.

The verifier should be checking these, so we're not ultimately losing safety.

> I used more generic function for determining alloca size that is passed to llvm.lifetime intrinsic:

Clang never emits the array form of alloca except for VLAs, where the lifetime is explicitly managed anyway.

> +llvm::Constant *CodeGenModule::getLLVMLifetimeStartFn() {
> +  if (LifetimeStartFn) return LifetimeStartFn;
> +  LifetimeStartFn = llvm::Intrinsic::getDeclaration(&getModule(),
> +                                            llvm::Intrinsic::lifetime_start);
> +  return LifetimeStartFn;
> +}
> +
> +/// Lazily declare the @llvm.lifetime.end intrinsic.
> +llvm::Constant *CodeGenModule::getLLVMLifetimeEndFn() {
> +  if (LifetimeEndFn) return LifetimeEndFn;
> +  LifetimeEndFn = llvm::Intrinsic::getDeclaration(&getModule(),
> +                                              llvm::Intrinsic::lifetime_end);
> +  return LifetimeEndFn;
> +}
> +
>  namespace {
>    /// A cleanup to perform a release of an object at the end of a
>    /// function.  This is used to balance out the incoming +1 of a
> 
> Modified: cfe/trunk/lib/CodeGen/CGStmt.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGStmt.cpp?rev=177819&r1=177818&r2=177819&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGStmt.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGStmt.cpp Sat Mar 23 01:43:35 2013
> @@ -319,6 +319,12 @@ CodeGenFunction::getJumpDestForLabel(con
>  }
> 
>  void CodeGenFunction::EmitLabel(const LabelDecl *D) {
> +  // Add this label to the current lexical scope if we're within any
> +  // normal cleanups.  Jumps "in" to this label --- when permitted by
> +  // the language --- may need to be routed around such cleanups.
> +  if (EHStack.hasNormalCleanups() && CurLexicalScope)
> +    CurLexicalScope->addLabel(D);
> 
> Why do you need this?

See the comment.  We're using the cleanups mechanism, but that creates a
situation where it's valid to goto into a cleanup, and so we need to adjust
label depths when we leave that scope.

Note that we'll likely lose any useful ability to optimize lifetimes in this case
because post-dominance will be broken.

Thanks for the review!

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130326/98621638/attachment.html>