[DebugInfo] Preserve scope in auto generated StoreInst

[gramanas]

Summary

While searching for the cause of missing Debug Loc in a phi instruction after the SROA pass
I narrowed down the problem in a mem2reg call from SROA.

Mem2reg copied the metadata from the store instructions to the phi one.

We figured it should be a problem with clang that doesn't produce the dbg info in the first place.
The store with the missing instructions is emitted in the function prologue and doesn't contain
any DL at all. If we assign it an Artificial DL it can then pass this information to the phi
created in mem2reg witch results in the phi no longer missing scope information.

The first test is this exact phi instruction that doesn't contain any Debug Loc.
The second one is a test for the store in the function prologue.

This patch instructs clang to add an artificial location to those store instructions using
ApplyDebugLocation::CreateArtificial() so that we can ensure scope is preserved.

[vsk]

I think CodeGenFunction::EmitParmDecl is the right place to set up an ApplyDebugLocation instance. You can look at CodeGenFunction::EmitAutoVarInit for an example of how to use ApplyDebugLocation.

test/CodeGen/debug-info-preserve-scope.c

2

The -O0 flag isn't needed here.

12

To check that we set the right location on the store, you might add:
; CHECK: ![[dbgLocForStore]] = !DILocation(line: 0

[gramanas]

• Apply debug location

[gramanas]

Update according to the comments

[vsk]

lib/CodeGen/CGDecl.cpp

2078

There are two issues here:

1. ApplyDebugLocation is a RAII helper, which means that it installs the proper debug location in its constructor, and restores the old debug location in its destructor. Since you are not assigning the result of the static constructor (CreateArtificial) to anything, the ApplyDebugLocation instance is immediately destroyed, so it's a no-op.
2. This is being applied in the wrong place, because it sets a debug location *after* the store is emitted. The right location needs to be applied before the store or alloca are emitted.

[gramanas]

Update diff

[vsk]

lib/CodeGen/CGDecl.cpp

2074

Ideally this would precede the calls to CreateMemTemp which set up the allocas.

test/CodeGen/debug-info-preserve-scope.c

11

Can you check that the alloca gets the same location as well? You can do this with:

CHECK: alloca {{.*}} !dbg ![[dbgLocForStore:[0-9]+]]
CHECK: store i32 {{.*}} !dbg ![[dbgLocForStore]]

[gramanas]

Move ApplyDebugLocation before CreateMemTemp

[vsk]

test/CodeGen/debug-info-preserve-scope.c

11

In these two check lines, you're capturing the variable dbgLocForStore twice. That means that if the !dbg location on the alloca were different from the location on the store, this test would still pass.

To fix that, just capture the dbgLocForStore variable once, the first time you see it on the alloca. In the second check line, you can simply refer to the captured variable with [[dbgLocForStore]].

[gramanas]

Set debug location to the entry block alloca

[gramanas]

Remove redundant this

[vsk]

lib/CodeGen/CGExpr.cpp

72 ↗

(On Diff #148200)

I think we need to be a bit more careful here. The current debug location stored in the builder may not be an artificial 0-location. This may cause non-linear single-stepping behavior. Consider this example:

void foo() {
  bar();
  if (...) {
    int var = ...; //< Clang emits an alloca for "var".
  }
...

The current debug location at the line "int var = ..." would be at line 4. But the alloca is emitted in the entry block of the function. In the debugger, this may result in strange single-stepping behavior when stepping into foo(). You could step to line 4, then line 2, then line 3, then line 4 again.

I think we can avoid that by setting an artificial location on allocas.

105 ↗

(On Diff #148200)

Why not apply the location here to cover more cases?

test/CodeGen/debug-info-preserve-scope.c

12

Why is "B" captured?

[aprantl]

lib/CodeGen/CGExpr.cpp

72 ↗

(On Diff #148200)

I think we can avoid that by setting an artificial location on allocas.

An alloca doesn't really generate any code (or rather.. the code it generates is in the function prologue). In what situation would the debug location on an alloca influence stepping? Are you thinking about the alloca() function?

[vsk]

lib/CodeGen/CGExpr.cpp

72 ↗

(On Diff #148200)

An alloca instruction can lower to a subtraction (off the stack pointer) though: https://godbolt.org/g/U4nGzJ.

dwarfdump shows that this subtraction instruction is actually assigned a location -- it currently happens to be the first location in the body of the function. I thought that assigning an artificial location to the alloca would be a first step towards fixing this.

Also, using an artificial location could mitigate possible bad interactions between code motion passes and IRBuilder. If, say, we were to set the insertion point right after an alloca, we might infer some arbitrary debug location. So long as this inference happens, it seems safer to infer an artificial location.

[aprantl]

lib/CodeGen/CGExpr.cpp

72 ↗

(On Diff #148200)

This may have unintended side-effects: By assigning a debug location to an alloca you are moving the end of the function prolog to before the alloca instructions, since LLVM computes the end of the function prologue as the first instruction with a non-empty debug location. Moving the end of the function prologue to before that stack pointer is adjusted is wrong, since that's the whole point of the prologue_end marker.

To me it looks more like a bug in a much later stage. With the exception of shrink-wrapped code, the prologue_end should always be after the stack pointer adjustment, I think.

[vsk]

lib/CodeGen/CGExpr.cpp

72 ↗

(On Diff #148200)

Thanks for explaining, I didn't realize that's how the end of the function prologue is computed! Should we leave out the any debug location changes for allocas in this patch, then?

[aprantl]

lib/CodeGen/CGExpr.cpp

72 ↗

(On Diff #148200)

I think that would be better. You might want to double-check PrologueEpilogueInserter and how the FrameSetup attribute is attached to MachineInstrs in case my knowledge is out-of-date.

[gramanas]

Adress review comments.
Limit changes to the storeInst

[probinson]

Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code.

[gramanas]

In D47097#1111149, @probinson wrote:

Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code.

Isn't this the reason the artificial debug loc exists? The store in the prolog might not be real code but it should still have the scope that the rest of the function has.

[aprantl]

In D47097#1111223, @gramanas wrote:

In D47097#1111149, @probinson wrote:

Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code.

Isn't this the reason the artificial debug loc exists? The store in the prolog might not be real code but it should still have the scope that the rest of the function has.

Instructions that are part of the function prologue are supposed to have no debug location, not an artificial one. The funciton prologue ends at the first instruction with a nonempty location.

[vsk]

In D47097#1111248, @aprantl wrote:

In D47097#1111223, @gramanas wrote:

In D47097#1111149, @probinson wrote:

Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code.

The problem @gramanas is trying to address appears after SROA. SROA invokes mem2reg, which tries to assign scope information to the phis it creates (see https://reviews.llvm.org/D45397). Subsequent passes which touch these phis can use these debug locations. This makes it easier for the debugger to find the right scope when handling a machine exception.

Store instructions in the prolog without scope information cause mem2reg to create phis without scope info. E.g:

// foo.c
extern int map[];
void f(int a, int n) {
  for (int i = 0; i < n; ++i)
    a = map[a];
}

$ clang foo.c -S -emit-llvm -o - -g -O1 -mllvm -opt-bisect-limit=2
..
    %a.addr.0 = phi i32 [ %a, %entry ], [ %0, %for.body ]

(Side note: @gramanas, it would help to have the full context/motivation for changes in the patch summary.)

Isn't this the reason the artificial debug loc exists? The store in the prolog might not be real code but it should still have the scope that the rest of the function has.

Instructions that are part of the function prologue are supposed to have no debug location, not an artificial one. The funciton prologue ends at the first instruction with a nonempty location.

I've been reading through PEI but I'm having a hard time verifying this. How does llvm determine where the function prologue ends when there isn't any debug info? What would go wrong if llvm started to behave as if the prologue ended sooner than it should? Also, why isn't this a problem for the swift compiler, which appears to always assign debug locations to each instruction?

[gramanas]

I added a test that illustrates what @vsk is talking about.

Sorry for not providing the relevant information but I thought this would
be a simple one liner like rL327800. I will update the revision's
description to include everything I know about this.

[gramanas]

What about this? Ping!

[aprantl]

lib/CodeGen/CGDecl.cpp

1949

Can you make that comment elaborate more about why this is being done? For example, you could add an "otherwise mem2reg will ..."

Also please note that all comments in LLVM need to be full sentences with a "." at the end :-)

[gramanas]

Make more elaborate comment.

lib/CodeGen/CGDecl.cpp

if (BlockInfo) {
       llvm::Value *V = Arg.isIndirect()
                            ? Builder.CreateLoad(Arg.getIndirectAddress())
                            : Arg.getDirectValue();
       setBlockContextParameter(IPD, ArgNo, V);
       return;
     }
   }
 
+  // Set artificial debug location in order to preserve the scope
+  auto DL = ApplyDebugLocation::CreateArtificial(*this);
+
   Address DeclPtr = Address::invalid();
   bool DoStore = false;
   bool IsScalar = hasScalarEvaluationKind(Ty);
   // If we already have a pointer to the argument, reuse the input pointer.
   if (Arg.isIndirect()) {
     DeclPtr = Arg.getIndirectAddress();
     // If we have a prettier pointer type at this point, bitcast to that.
     unsigned AS = DeclPtr.getType()->getAddressSpace();
if (Qualifiers::ObjCLifetime lt = qs.getObjCLifetime()) {
           DoStore = false; // The weak init is a store, no need to do two.
         }
       }
 
       // Enter the cleanup scope.
       EmitAutoVarWithLifetime(*this, D, DeclPtr, lt);
     }
   }
 
   // Store the initial value into the alloca.
   if (DoStore)
     EmitStoreOfScalar(ArgVal, lv, /* isInitialization */ true);
 
   setAddrOfLocalVar(&D, DeclPtr);
 
   // Emit debug info for param declaration.
   if (CGDebugInfo *DI = getDebugInfo()) {
     if (CGM.getCodeGenOpts().getDebugInfo() >=
         codegenoptions::LimitedDebugInfo) {
       DI->EmitDeclareOfArgVariable(&D, DeclPtr.getPointer(), ArgNo, Builder);
     }

test/CodeGen/debug-info-preserve-scope.c

+// RUN: %clang_cc1 -debug-info-kind=limited -emit-llvm -o - %s | FileCheck %s
+
+static int a;
+
+// CHECK-LABEL: define void @f
+void f(int b) {
+  a = b;
+}
+
+// CHECK: store i32 {{.*}} !dbg ![[artificialDbgLoc:[0-9]+]]
+
+// CHECK: ![[artificialDbgLoc]] = !DILocation(line: 0