Page MenuHomePhabricator

[DebugInfo] Preserve scope in auto generated StoreInst
Needs ReviewPublic

Authored by gramanas on May 18 2018, 5:31 PM.

Details

Reviewers
vsk
aprantl
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.

Diff Detail

Event Timeline

gramanas created this revision.May 18 2018, 5:31 PM
vsk added a comment.May 21 2018, 9:46 AM

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 updated this revision to Diff 147838.May 21 2018, 12:40 PM
  • Apply debug location
gramanas updated this revision to Diff 147840.May 21 2018, 12:43 PM
gramanas marked an inline comment as done.

Update according to the comments

gramanas marked an inline comment as done.May 21 2018, 12:43 PM
vsk added inline comments.May 21 2018, 12:46 PM
lib/CodeGen/CGDecl.cpp
2079

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 retitled this revision from [WIP][DebugInfo] Preserve scope in auto generated StoreInst to [DebugInfo] Preserve scope in auto generated StoreInst.May 21 2018, 1:58 PM
gramanas edited the summary of this revision. (Show Details)
gramanas marked an inline comment as done.May 21 2018, 2:00 PM
gramanas updated this revision to Diff 147876.May 21 2018, 2:42 PM

Update diff

vsk added inline comments.May 21 2018, 2:48 PM
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 updated this revision to Diff 148034.May 22 2018, 9:56 AM

Move ApplyDebugLocation before CreateMemTemp

gramanas marked 2 inline comments as done.May 22 2018, 9:57 AM
vsk added inline comments.May 22 2018, 10:00 AM
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 updated this revision to Diff 148198.May 23 2018, 6:22 AM

Set debug location to the entry block alloca

gramanas updated this revision to Diff 148200.May 23 2018, 6:28 AM
gramanas marked an inline comment as done.

Remove redundant this

vsk added inline comments.May 23 2018, 10:39 AM
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 added inline comments.May 23 2018, 11:43 AM
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 added inline comments.May 23 2018, 1:01 PM
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 added inline comments.May 23 2018, 1:17 PM
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 added inline comments.May 23 2018, 1:19 PM
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 added inline comments.May 23 2018, 1:45 PM
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 updated this revision to Diff 148414.May 24 2018, 7:48 AM

Adress review comments.
Limit changes to the storeInst

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.

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.

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 added a comment.May 24 2018, 11:00 AM

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 updated this revision to Diff 148588.May 25 2018, 5:04 AM

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 edited the summary of this revision. (Show Details)May 25 2018, 5:13 AM
davide added a subscriber: davide.May 25 2018, 4:41 PM

What about this? Ping!

aprantl added inline comments.Jun 6 2018, 7:46 AM
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 updated this revision to Diff 150357.Jun 7 2018, 10:09 AM

Make more elaborate comment.

gramanas marked an inline comment as done.Jun 7 2018, 10:11 AM