[CGBlocks] Improve line info in backtraces containing *_helper_block

[vsk]

Summary

Instead of only setting a non-zero debug location on the return
instruction in *_helper_block functions, set a proper location on all
instructions within these functions. Pick the start location of the
block literal expr for maximum clarity.

The debugger does not step into *_helper_block functions during normal
single-stepping because we mark their parameters as artificial. This is
what we want (the functions are implicitly generated and uninteresting
to most users). The stepping behavior is unchanged by this patch.

rdar://32907581

[vsk]

• Simplify the checks by not hardcoding a line number.

[JDevlieghere]

lib/CodeGen/CGBlocks.cpp

1651 ↗

(On Diff #120342)

You probably have a good reason for doing so, but why do we need {}-initialization?

[vsk]

lib/CodeGen/CGBlocks.cpp

1651 ↗

(On Diff #120342)

It's just a personal idiosyncrasy. I like braced-init because it doesn't have a vexing parse issue, it works with list/aggregate init, and it makes it clear that operator= isn't being called.

[aprantl]

Thanks! This looks generally good, I have one pending question inline.

test/CodeGenObjC/debug-info-blocks.m

13 ↗

(On Diff #120342)

We nowadays usually try to avoid rdar links in anything but commit messages because it is not helpful information for the LLVM community. It's better to just describe the issue at hand (which you are doing!) or create a PR with that contents and link to it.

20 ↗

(On Diff #120342)

What's the location used for the ret? I think it should also be` ![[DBG_LINE]]` since we are not actually executing the block.

[JDevlieghere]

lib/CodeGen/CGBlocks.cpp

1651 ↗

(On Diff #120342)

Fair enough. Thanks for clarifying!

[vsk]

test/CodeGenObjC/debug-info-blocks.m

13 ↗

(On Diff #120342)

Got it.

20 ↗

(On Diff #120342)

We're using COPY_LINE, which is the same location used for the load instruction below.

What's the semantic difference between DBG_LINE (line 0) and COPY_LINE (line 68) anyway? Why do we have two different locations for the arguments to this function?

[aprantl]

test/CodeGenObjC/debug-info-blocks.m

20 ↗

(On Diff #120342)

The debugger will skip over line 0 locations when single-stepping or when setting breakpoints. I can't tell without reading the code why we decide to put a line 0 on the call.

20 ↗

(On Diff #120342)

The important thing is that the testcase should check that the ret has either COPY_LINE or line 0 on it and not line 71.

[vsk]

• Tighten test case to show the dbg loc on return instructions.

test/CodeGenObjC/debug-info-blocks.m

20 ↗

(On Diff #120342)

I'll fix up the test case.

It looks like the zero location is an artifact of CodeGenFunction::StartFunction:

1128   // Emit a location at the end of the prologue.                                                                                                                                                            
1129   if (CGDebugInfo *DI = getDebugInfo())                                                                                                                                                                     
1130     DI->EmitLocation(Builder, StartLoc);

I think it's unnecessary, but maybe we can look into that separately?

[aprantl]

test/CodeGenObjC/debug-info-blocks.m

20 ↗

(On Diff #120342)

... and StartLoc is empty for this function? (which would make sense)
I think I'm fine with leaving this as is unless you feel strongly about it.

[vsk]

Thanks!

test/CodeGenObjC/debug-info-blocks.m

20 ↗

(On Diff #120342)

Yes, StartLoc is empty for the function, but I misunderstood the source of the zero location. I think it actually comes from CGDebugInfo::EmitFunctionStart, in the same logic that sets the artificial flag on parameters. It looks like this is intentional so I won't touch it. I'll go ahead and land this fix.

cfe/trunk/lib/CodeGen/CGBlocks.cpp

FunctionDecl *FD = FunctionDecl::Create(C,
                                           SourceLocation(),
                                           SourceLocation(), II, C.VoidTy,
                                           nullptr, SC_Static,
                                           false,
                                           false);
 
   CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);
 
-  auto NL = ApplyDebugLocation::CreateEmpty(*this);
   StartFunction(FD, C.VoidTy, Fn, FI, args);
-  // Create a scope with an artificial location for the body of this function.
-  auto AL = ApplyDebugLocation::CreateArtificial(*this);
+  ApplyDebugLocation NL{*this, blockInfo.getBlockExpr()->getLocStart()};
   llvm::Type *structPtrTy = blockInfo.StructureType->getPointerTo();
 
   Address src = GetAddrOfLocalVar(&SrcDecl);
   src = Address(Builder.CreateLoad(src), blockInfo.BlockAlign);
   src = Builder.CreateBitCast(src, structPtrTy, "block.source");
 
   Address dst = GetAddrOfLocalVar(&DstDecl);
   dst = Address(Builder.CreateLoad(dst), blockInfo.BlockAlign);
CodeGenFunction::GenerateDestroyHelperFunction(const CGBlockInfo &blockInfo) {
   FunctionDecl *FD = FunctionDecl::Create(C, C.getTranslationUnitDecl(),
                                           SourceLocation(),
                                           SourceLocation(), II, C.VoidTy,
                                           nullptr, SC_Static,
                                           false, false);
 
   CGM.SetInternalFunctionAttributes(nullptr, Fn, FI);
 
-  // Create a scope with an artificial location for the body of this function.
-  auto NL = ApplyDebugLocation::CreateEmpty(*this);
   StartFunction(FD, C.VoidTy, Fn, FI, args);
-  auto AL = ApplyDebugLocation::CreateArtificial(*this);
+  ApplyDebugLocation NL{*this, blockInfo.getBlockExpr()->getLocStart()};
 
   llvm::Type *structPtrTy = blockInfo.StructureType->getPointerTo();
 
   Address src = GetAddrOfLocalVar(&SrcDecl);
   src = Address(Builder.CreateLoad(src), blockInfo.BlockAlign);
   src = Builder.CreateBitCast(src, structPtrTy, "block");
 
   CodeGenFunction::RunCleanupsScope cleanups(*this);

cfe/trunk/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 !{{.*}})
 
-// 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.
+// Test that we do emit scope info for the helper functions, and that the
+// parameters to these functions are marked as artificial (so the debugger
+// doesn't accidentally step into the function).
 // 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: ret void, !dbg ![[COPY_LINE]]
 // CHECK: define {{.*}} @__destroy_helper_block_{{.*}}(i8*)
 // CHECK-NOT: ret
 // CHECK: load {{.*}}, !dbg ![[DESTROY_LINE:[0-9]+]]
+// CHECK: ret void, !dbg ![[DESTROY_LINE]]
 
-// 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_"
 typedef unsigned int NSUInteger;
 
 @protocol NSObject
 @end  
    
 @interface NSObject <NSObject>
 - (id)init;
 + (id)alloc;
static void run(void (^block)(void))
     block();
 }
 
 @implementation A
 
 - (id)init
 {
     if ((self = [super init])) {
+      // CHECK-DAG: [[DBG_LINE]] = !DILocation(line: 0, scope: ![[COPY_SP:[0-9]+]])
+      // CHECK-DAG: [[COPY_LINE]] = !DILocation(line: [[@LINE+7]], scope: ![[COPY_SP:[0-9]+]])
+      // CHECK-DAG: [[COPY_SP]] = distinct !DISubprogram(name: "__copy_helper_block_"
+      // CHECK-DAG: [[DESTROY_LINE]] = !DILocation(line: [[@LINE+5]], scope: ![[DESTROY_SP:[0-9]+]])
+      // CHECK-DAG: [[DESTROY_SP]] = distinct !DISubprogram(name: "__destroy_helper_block_"
+      // CHECK-DAG: !DILocalVariable(arg: 1, scope: ![[COPY_SP]], {{.*}}, flags: DIFlagArtificial)
+      // CHECK-DAG: !DILocalVariable(arg: 2, scope: ![[COPY_SP]], {{.*}}, flags: DIFlagArtificial)
+      // CHECK-DAG: !DILocalVariable(arg: 1, scope: ![[DESTROY_SP]], {{.*}}, flags: DIFlagArtificial)
       run(^{
           // CHECK-DAG: ![[SELF]] = !DILocalVariable(name: "self", scope:{{.*}}, line: [[@LINE+4]],
           // CHECK-DAG: ![[D]] = !DILocalVariable(name: "d", scope:{{.*}}, line: [[@LINE+1]],
           NSMutableDictionary *d = [[NSMutableDictionary alloc] init]; 
           ivar = 42 + (int)[d count];
         });
     }
     return self;