PR32382: Adapt to LLVM changes in DIExpression.

[aprantl]

Summary

LLVM has changes the semantics of dbg.declare for describing function arguments. After this patch a dbg.declare always takes the *address* of a variable as the first argument, even if the argument is not an alloca.

https://bugs.llvm.org/show_bug.cgi?id=32382
rdar://problem/31205000

[dblaikie]

I'm a bit confused - the alloca was only emitted at -O0, by the looks of it. Presumably it's pessimizing in some way at higher optimization levels? Or is that not the case?

Also, it looks like this change lost the "if > gmlt" test, so might cause variable declarations (& thus types) to leak into GMLT, which is undesirable.

[aprantl]

In D31440#713308, @dblaikie wrote:

I'm a bit confused - the alloca was only emitted at -O0, by the looks of it. Presumably it's pessimizing in some way at higher optimization levels? Or is that not the case?

I think it is really working around the odd behavior of LLVM here. What gives it away is the we key the addition of the extra DW_OP_deref on whether it is an alloca or not. But note that this is not how dbg.declare works: dbg.declare(%alloca, !DIExpression()) is (kind of) equivalent to dbg.value(%alloca, !DIExpression(DW_OP_deref)). So we are using the presence of the alloca as a proxy for how the backend happens to compile the DwarfExpression here and work around its idiosyncrasy by emitting an extra DW_OP_deref.

Also, it looks like this change lost the "if > gmlt" test, so might cause variable declarations (& thus types) to leak into GMLT, which is undesirable.

Oh.. that was unintentional collateral damage. Thanks for noticing!

[aprantl]

Add accidentally removed check for the debug info level back in.

[dblaikie]

In D31440#713396, @aprantl wrote:

In D31440#713308, @dblaikie wrote:

I'm a bit confused - the alloca was only emitted at -O0, by the looks of it. Presumably it's pessimizing in some way at higher optimization levels? Or is that not the case?

I think it is really working around the odd behavior of LLVM here. What gives it away is the we key the addition of the extra DW_OP_deref on whether it is an alloca or not. But note that this is not how dbg.declare works: dbg.declare(%alloca, !DIExpression()) is (kind of) equivalent to dbg.value(%alloca, !DIExpression(DW_OP_deref)). So we are using the presence of the alloca as a proxy for how the backend happens to compile the DwarfExpression here and work around its idiosyncrasy by emitting an extra DW_OP_deref.

I'm still not really following, sorry - perhaps it'd help me if you could describe the state of things in ToT currently, the state this change attempts to create, and the state (if distinct from the previous) that might be ideal. (it's not clear to me if there's still a "workaround" after your change - sounds like there should be/is, if previously there was no alloca above O0 but after this change there will be... )

Also, it looks like this change lost the "if > gmlt" test, so might cause variable declarations (& thus types) to leak into GMLT, which is undesirable.

Oh.. that was unintentional collateral damage. Thanks for noticing!

[echristo]

Sounds like Dave is asking for changes so I'll put it down as Request Changes to get it out of my queue. :)

[aprantl]

I think I may haved screwed up the phabricator process for this one.
This landed as r300523.

lib/CodeGen/CGBlocks.cpp

   return Result;
 }
 
 void CodeGenFunction::setBlockContextParameter(const ImplicitParamDecl *D,
                                                unsigned argNum,
                                                llvm::Value *arg) {
   assert(BlockInfo && "not emitting prologue of block invocation function?!");
 
-  llvm::Value *localAddr = nullptr;
-  if (CGM.getCodeGenOpts().OptimizationLevel == 0) {
-    // Allocate a stack slot to let the debug info survive the RA.
-    Address alloc = CreateMemTemp(D->getType(), D->getName() + ".addr");
-    Builder.CreateStore(arg, alloc);
-    localAddr = Builder.CreateLoad(alloc);
-  }
+  // Allocate a stack slot to let the debug info survive the RA.
+  Address LocalAddrAlloca = CreateMemTemp(D->getType(), D->getName() + ".addr");
+  Builder.CreateStore(arg, LocalAddrAlloca);
+  auto *localAddr = Builder.CreateLoad(LocalAddrAlloca);
 
   if (CGDebugInfo *DI = getDebugInfo()) {
-    if (CGM.getCodeGenOpts().getDebugInfo() >=
-        codegenoptions::LimitedDebugInfo) {
-      DI->setLocation(D->getLocation());
-      DI->EmitDeclareOfBlockLiteralArgVariable(*BlockInfo, arg, argNum,
-                                               localAddr, Builder);
-    }
+    DI->setLocation(D->getLocation());
+    DI->EmitDeclareOfBlockLiteralArgVariable(*BlockInfo, arg->getName(),
+                                             LocalAddrAlloca.getPointer(),
+                                             argNum, Builder);
   }
 
   SourceLocation StartLoc = BlockInfo->getBlockExpr()->getBody()->getLocStart();
   ApplyDebugLocation Scope(*this, StartLoc);
 
   // Instead of messing around with LocalDeclMap, just set the value
   // directly as BlockPointer.
   BlockPointer = Builder.CreatePointerCast(
-      arg,
+      localAddr,
       BlockInfo->StructureType->getPointerTo(
           getContext().getLangOpts().OpenCL
               ? getContext().getTargetAddressSpace(LangAS::opencl_generic)
               : 0),
       "block");
 }
 
 Address CodeGenFunction::LoadBlockStruct() {

   CGM.SetInternalFunctionAttributes(blockDecl, fn, fnInfo);
 
   // Begin generating the function.
   StartFunction(blockDecl, fnType->getReturnType(), fn, fnInfo, args,
                 blockDecl->getLocation(),
                 blockInfo.getBlockExpr()->getBody()->getLocStart());
 
   // Okay.  Undo some of what StartFunction did.
-
-  // At -O0 we generate an explicit alloca for the BlockPointer, so the RA
-  // won't delete the dbg.declare intrinsics for captured variables.
   llvm::Value *BlockPointerDbgLoc = BlockPointer;
-  if (CGM.getCodeGenOpts().OptimizationLevel == 0) {
-    // Allocate a stack slot for it, so we can point the debugger to it
-    Address Alloca = CreateTempAlloca(BlockPointer->getType(),
-                                      getPointerAlign(),
-                                      "block.addr");
-    // Set the DebugLocation to empty, so the store is recognized as a
-    // frame setup instruction by llvm::DwarfDebug::beginFunction().
-    auto NL = ApplyDebugLocation::CreateEmpty(*this);
-    Builder.CreateStore(BlockPointer, Alloca);
-    BlockPointerDbgLoc = Alloca.getPointer();
-  }
+  // Allocate a stack slot for it, so we can point the debugger to it
+  Address Alloca = CreateTempAlloca(BlockPointer->getType(), getPointerAlign(),
+                                    "block.addr");
+  // Set the DebugLocation to empty, so the store is recognized as a
+  // frame setup instruction by llvm::DwarfDebug::beginFunction().
+  auto NL = ApplyDebugLocation::CreateEmpty(*this);
+  Builder.CreateStore(BlockPointer, Alloca);
+  BlockPointerDbgLoc = Alloca.getPointer();
 
   // If we have a C++ 'this' reference, go ahead and force it into
   // existence now.
   if (blockDecl->capturesCXXThis()) {
     Address addr =
       Builder.CreateStructGEP(LoadBlockStruct(), blockInfo.CXXThisIndex,
                               blockInfo.CXXThisOffset, "block.captured-this");
     CXXThisValue = Builder.CreateLoad(addr, "this");

lib/CodeGen/CGDebugInfo.h

public:
   /// Emit call to \c llvm.dbg.declare for an argument variable
   /// declaration.
   void EmitDeclareOfArgVariable(const VarDecl *Decl, llvm::Value *AI,
                                 unsigned ArgNo, CGBuilderTy &Builder);
 
   /// Emit call to \c llvm.dbg.declare for the block-literal argument
   /// to a block invocation function.
   void EmitDeclareOfBlockLiteralArgVariable(const CGBlockInfo &block,
-                                            llvm::Value *Arg, unsigned ArgNo,
-                                            llvm::Value *LocalAddr,
+                                            StringRef Name,
+                                            llvm::Value *Storage,
+                                            unsigned ArgNo,
                                             CGBuilderTy &Builder);
 
   /// Emit information about a global variable.
   void EmitGlobalVariable(llvm::GlobalVariable *GV, const VarDecl *Decl);
 
   /// Emit a constant global variable's debug info.
   void EmitGlobalVariable(const ValueDecl *VD, const APValue &Init);
 

lib/CodeGen/CGDebugInfo.cpp

   if (!Name.empty()) {
     if (VD->hasAttr<BlocksAttr>()) {
       CharUnits offset = CharUnits::fromQuantity(32);
       Expr.push_back(llvm::dwarf::DW_OP_plus);
       // offset of __forwarding field
       offset = CGM.getContext().toCharUnitsFromBits(
           CGM.getTarget().getPointerWidth(0));
       Expr.push_back(offset.getQuantity());
-      Expr.push_back(llvm::dwarf::DW_OP_deref);
       Expr.push_back(llvm::dwarf::DW_OP_plus);
       // offset of x field
       offset = CGM.getContext().toCharUnitsFromBits(XOffset);
       Expr.push_back(offset.getQuantity());
+      Expr.push_back(llvm::dwarf::DW_OP_deref);
 
       // Create the descriptor for the variable.
       auto *D = ArgNo
                     ? DBuilder.createParameterVariable(Scope, VD->getName(),
                                                        *ArgNo, Unit, Line, Ty)
                     : DBuilder.createAutoVariable(Scope, VD->getName(), Unit,
                                                   Line, Ty, Align);
 

 
   const llvm::DataLayout &target = CGM.getDataLayout();
 
   CharUnits offset = CharUnits::fromQuantity(
       target.getStructLayout(blockInfo.StructureType)
           ->getElementOffset(blockInfo.getCapture(VD).getIndex()));
 
   SmallVector<int64_t, 9> addr;
-  if (isa<llvm::AllocaInst>(Storage))
-    addr.push_back(llvm::dwarf::DW_OP_deref);
   addr.push_back(llvm::dwarf::DW_OP_plus);
-  addr.push_back(offset.getQuantity());
+  addr.push_back(offset.getQuantity()); 
+  addr.push_back(llvm::dwarf::DW_OP_deref);
   if (isByRef) {
-    addr.push_back(llvm::dwarf::DW_OP_deref);
-    addr.push_back(llvm::dwarf::DW_OP_plus);
+    addr.push_back(llvm::dwarf::DW_OP_plus); 
     // offset of __forwarding field
     offset =
         CGM.getContext().toCharUnitsFromBits(target.getPointerSizeInBits(0));
-    addr.push_back(offset.getQuantity());
+    addr.push_back(offset.getQuantity()); 
     addr.push_back(llvm::dwarf::DW_OP_deref);
     addr.push_back(llvm::dwarf::DW_OP_plus);
     // offset of x field
     offset = CGM.getContext().toCharUnitsFromBits(XOffset);
     addr.push_back(offset.getQuantity());
+    addr.push_back(llvm::dwarf::DW_OP_deref);
   }
 
   // Create the descriptor for the variable.
   auto Align = getDeclAlignIfRequired(VD, CGM.getContext());
   auto *D = DBuilder.createAutoVariable(
       cast<llvm::DILocalScope>(LexicalBlockStack.back()), VD->getName(), Unit,
       Line, Ty, false, llvm::DINode::FlagZero, Align);
 

   const BlockDecl::Capture *Capture;
 };
 bool operator<(const BlockLayoutChunk &l, const BlockLayoutChunk &r) {
   return l.OffsetInBits < r.OffsetInBits;
 }
 }
 
 void CGDebugInfo::EmitDeclareOfBlockLiteralArgVariable(const CGBlockInfo &block,
-                                                       llvm::Value *Arg,
+                                                       StringRef Name,
+                                                       llvm::Value *Storage,
                                                        unsigned ArgNo,
-                                                       llvm::Value *LocalAddr,
                                                        CGBuilderTy &Builder) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
   ASTContext &C = CGM.getContext();
   const BlockDecl *blockDecl = block.getBlockDecl();
 
   // Collect some general information about the block's location.
   SourceLocation loc = blockDecl->getCaretLocation();
   llvm::DIFile *tunit = getOrCreateFile(loc);

   type = DBuilder.createPointerType(type, CGM.PointerWidthInBits);
 
   // Get overall information about the block.
   llvm::DINode::DIFlags flags = llvm::DINode::FlagArtificial;
   auto *scope = cast<llvm::DILocalScope>(LexicalBlockStack.back());
 
   // Create the descriptor for the parameter.
   auto *debugVar = DBuilder.createParameterVariable(
-      scope, Arg->getName(), ArgNo, tunit, line, type,
-      CGM.getLangOpts().Optimize, flags);
-
-  if (LocalAddr) {
-    // Insert an llvm.dbg.value into the current block.
-    DBuilder.insertDbgValueIntrinsic(
-        LocalAddr, 0, debugVar, DBuilder.createExpression(),
-        llvm::DebugLoc::get(line, column, scope, CurInlinedAt),
-        Builder.GetInsertBlock());
-  }
+      scope, Name, ArgNo, tunit, line, type, CGM.getLangOpts().Optimize, flags);
 
   // Insert an llvm.dbg.declare into the current block.
-  DBuilder.insertDeclare(Arg, debugVar, DBuilder.createExpression(),
+  DBuilder.insertDeclare(Storage, debugVar, DBuilder.createExpression(),
                          llvm::DebugLoc::get(line, column, scope, CurInlinedAt),
                          Builder.GetInsertBlock());
 }
 
 llvm::DIDerivedType *
 CGDebugInfo::getOrCreateStaticDataMemberDeclarationOrNull(const VarDecl *D) {
   if (!D->isStaticDataMember())
     return nullptr;

test/CodeGenObjC/debug-info-block-captured-self.m

 
 // inner block:
 // CHECK: define internal void {{.*}}_block_invoke{{.*}}
 // CHECK:        %[[MEM1:.*]] = alloca i8*, align 8
 // CHECK-NEXT:   %[[MEM2:.*]] = alloca i8*, align 8
 // CHECK-NEXT:   [[DBGADDR:%.*]] = alloca [[BLOCK_T:<{.*}>]]*, align 8
 // CHECK:        store i8* [[BLOCK_DESC:%.*]], i8** %[[MEM1]], align 8
 // CHECK:        %[[TMP0:.*]] = load i8*, i8** %[[MEM1]]
-// CHECK:        call void @llvm.dbg.value(metadata i8* %[[TMP0]], i64 0, metadata ![[BDMD:[0-9]+]], metadata !{{.*}})
-// CHECK:        call void @llvm.dbg.declare(metadata i8* [[BLOCK_DESC]], metadata ![[BDMD:[0-9]+]], metadata !{{.*}})
+// CHECK:        call void @llvm.dbg.declare(metadata i8** %[[MEM1]],
+// CHECK-SAME:                               metadata ![[BDMD:[0-9]+]],
+// CHECK-SAME:                               metadata ![[EMPTY:[0-9]+]])
 // CHECK:        store [[BLOCK_T]]* {{%.*}}, [[BLOCK_T]]** [[DBGADDR]], align 8
-// CHECK:        call void @llvm.dbg.declare(metadata [[BLOCK_T]]** [[DBGADDR]], metadata ![[SELF:.*]], metadata !{{.*}})
+// CHECK:        call void @llvm.dbg.declare(metadata [[BLOCK_T]]** [[DBGADDR]],
+// CHECK-SAME:                               metadata ![[SELF:[0-9]+]],
+// CHECK-SAME:                               metadata ![[EXPR:[0-9]+]])
+
 // make sure we are still in the same function
 // CHECK: define {{.*}}__copy_helper_block_
 // Metadata
 // CHECK: ![[MAIN:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Main"
 // CHECK-SAME:                            line: 23,
 // CHECK: ![[PMAIN:.*]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[MAIN]],
+// CHECK: ![[EMPTY]] = !DIExpression()
+// CHECK: ![[EXPR]] = !DIExpression(DW_OP_plus, 32, DW_OP_deref)
 // CHECK: ![[BDMD]] = !DILocalVariable(name: ".block_descriptor", arg:
 // CHECK: ![[SELF]] = !DILocalVariable(name: "self"
 // CHECK-NOT:                          arg:
 // CHECK-SAME:                         line: 40,

test/CodeGenObjC/debug-info-blocks.m

 // RUN: %clang_cc1 -emit-llvm -fblocks -debug-info-kind=limited  -triple x86_64-apple-darwin10 -fobjc-dispatch-method=mixed -x objective-c < %s -o - | FileCheck %s
 
 // rdar://problem/9279956
 // Test that we generate the proper debug location for a captured self.
 // The second half of this test is in llvm/tests/DebugInfo/debug-info-blocks.ll
 
-// CHECK: define {{.*}}_block_invoke
-// CHECK: %[[BLOCK:.*]] = bitcast i8* %.block_descriptor to <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, %0* }>*, !dbg
-// CHECK-NEXT: store <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, %0* }>* %[[BLOCK]], <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, %0* }>** %[[ALLOCA:.*]], align
-// CHECK-NEXT: call void @llvm.dbg.declare(metadata <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, %0* }>** %[[ALLOCA]], metadata ![[SELF:[0-9]+]], metadata !{{.*}})
-// CHECK-NEXT: call void @llvm.dbg.declare(metadata %1** %d, metadata ![[D:[0-9]+]], metadata !{{.*}})
+// CHECK: define {{.*}}[A init]_block_invoke
+// CHECK-NEXT: entry
+// CHECK-NEXT: %[[DESC_ALLOCA:.*]] = alloca i8*, align 8
+// CHECK-NEXT: %[[BLOCK_ALLOCA:.*]] = alloca <{{.*}}>*, align 8
+// CHECK: store i8* %.block_descriptor, i8** %[[ALLOCA:.*]], align 8
+// CHECK: call void @llvm.dbg.declare(metadata i8** %[[DESC_ALLOCA]],
+// CHECK-SAME:                        metadata ![[BLOCK_VAR:[0-9]+]],
+// CHECK: call void @llvm.dbg.declare(
+// CHECK-SAME: metadata <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, %0* }>**
+// CHECK-SAME:          %[[BLOCK_ALLOCA]], metadata ![[SELF_VAR:[0-9]+]],
+// CHECK-SAME:          metadata ![[SELF_EXPR:[0-9]+]])
+// CHECK: call void @llvm.dbg.declare(metadata %1** %d, metadata ![[D_VAR:[0-9]+]],
 
 // rdar://problem/14386148
 // Test that we don't emit bogus line numbers for the helper functions.
 // Test that we do emit scope info for the helper functions.
 // CHECK: define {{.*}} @__copy_helper_block_{{.*}}(i8*, i8*)
 // CHECK-NOT: ret
 // CHECK: call {{.*}}, !dbg ![[DBG_LINE:[0-9]+]]
 // CHECK-NOT: ret
 // CHECK: load {{.*}}, !dbg ![[COPY_LINE:[0-9]+]]
 // CHECK: define {{.*}} @__destroy_helper_block_{{.*}}(i8*)
 // CHECK-NOT: ret
 // CHECK: load {{.*}}, !dbg ![[DESTROY_LINE:[0-9]+]]
 
-// CHECK-DAG: [[DBG_LINE]] = !DILocation(line: 0, scope: ![[COPY_SP:[0-9]+]])
-// CHECK-DAG: [[COPY_LINE]] = !DILocation(line: 0, scope: ![[COPY_SP:[0-9]+]])
-// CHECK-DAG: [[COPY_SP]] = distinct !DISubprogram(name: "__copy_helper_block_"
-// CHECK-DAG: [[DESTROY_LINE]] = !DILocation(line: 0, scope: ![[DESTROY_SP:[0-9]+]])
-// CHECK-DAG: [[DESTROY_SP]] = distinct !DISubprogram(name: "__destroy_helper_block_"
+// CHECK-DAG: ![[DBG_LINE]] = !DILocation(line: 0, scope: ![[COPY_SP:[0-9]+]])
+// CHECK-DAG: ![[COPY_LINE]] = !DILocation(line: 0, scope: ![[COPY_SP:[0-9]+]])
+// CHECK-DAG: ![[COPY_SP]] = distinct !DISubprogram(name: "__copy_helper_block_"
+// CHECK-DAG: ![[DESTROY_LINE]] = !DILocation(line: 0, scope: ![[DESTROY_SP:[0-9]+]])
+// CHECK-DAG: ![[DESTROY_SP]] = distinct !DISubprogram(name: "__destroy_helper_block_"
+// CHECK-DAG: ![[BLOCK_VAR]] = !DILocalVariable(name: ".block_descriptor"
+// CHECK-DAG: ![[SELF_EXPR]] = !DIExpression(DW_OP_plus, 32, DW_OP_deref)
 typedef unsigned int NSUInteger;
 
 @protocol NSObject
 @end  
    
 @interface NSObject <NSObject>
 - (id)init;
 + (id)alloc;

 }
 
 @implementation A
 
 - (id)init
 {
     if ((self = [super init])) {
       run(^{
-          // CHECK-DAG: ![[SELF]] = !DILocalVariable(name: "self", scope:{{.*}}, line: [[@LINE+4]],
-          // CHECK-DAG: ![[D]] = !DILocalVariable(name: "d", scope:{{.*}}, line: [[@LINE+1]],
+          // CHECK-DAG: ![[SELF_VAR]] = !DILocalVariable(name: "self", scope:{{.*}}, line: [[@LINE+4]],
+          // CHECK-DAG: ![[D_VAR]] = !DILocalVariable(name: "d", scope:{{.*}}, line: [[@LINE+1]],
           NSMutableDictionary *d = [[NSMutableDictionary alloc] init]; 
           ivar = 42 + (int)[d count];
         });
     }
     return self;
 }
 
 @end
 
 int main()
 {
   A *a = [[A alloc] init];
   return 0;
 }