Page MenuHomePhabricator

[InstCombine] Add a flag to disable LowerDbgDeclare
ClosedPublic

Authored by rnk on Aug 10 2017, 3:48 PM.

Details

Summary

This should improve optimized debug info for address-taken variables at
the cost of inaccurate debug info in some situations.

We patched this into clang and deployed this change to Chromium
developers, and this significantly improved debuggability of optimized
code. The long-term solution to PR34136 seems more and more like it's
going to take a while, so I would like to commit this change under a
flag so that it can be used as a stop-gap measure.

This flag should really help so for C++ aggregates like std::string and
std::vector, which are typically address-taken, even after inlining, and
cannot be SROA-ed.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Aug 10 2017, 3:48 PM
aprantl edited edge metadata.Aug 10 2017, 4:03 PM

What happens when there are only loads from the alloca, shouldn't they be handled similarly?

int i;
f(&i)
return i+1;
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2061 ↗(On Diff #110643)

What's the effect of this change?

2097 ↗(On Diff #110643)

Maybe "If we are removing an alloca ..."?

It can also be problematic if stack coloring arranges to reuse the stack memory for other data.

If lifetime intrinsics survive into MIR, then we could teach DebugValueHistoryCalculator about them. Otherwise we could have the stack coloring pass lower dbg.declares describing allocas with lifetime intrinsics into a dbg.value(%alloca, ...) at the beginning and a dbg.value(null, ...) at the end of the live range.

Oh, and we'd have to teach LiveDebugValues about memory locations (probably not very hard).

It is possible that this change could regress debug info quality when other passes like GVN delete stores to locals.

Could we create a utility function updateDebugInfoForDeletedStore that finds any dbg.declare describing the alloca and runs ConvertDebugDeclareToDebugValue?

Sorry for updating this so often :-)
If the stack coloring change I outlined above turns out to be too complicated and the deferred ConvertDebugDeclareToDebugValue in GVN works, we could do the same thing in the stack coloring pass. (Perhaps even hook the functionality into the llvm::salvageDebugInfo() API that is there to be called on instructions before they are deleted).

rnk added a comment.Aug 14 2017, 9:48 AM

Long term, yes, I think we'll want to push the lifetime intrinsics down to MI so we can get better PC ranges for variable locations. I don't know when they are removed right now.

Trying to think more broadly, here's how things work today:

  • In the C abstract machine, variables live in memory. LLVM's job is to erode that abstraction.
  • dbg.declare says simply "this variable lives here in memory, its value changes whenever it is stored to"
  • Any pass that deletes dead stores (or more generally dead memset/memcpy/writes) will cause the user to see a stale value in the debugger from a missed write
  • dbg.value gives us a way to describe dead stores in debug info

The problem is that we can't mix dbg.value and dbg.declare. For variables that we can promote to SSA, we can describe all the stores with dbg.value, and that's good enough. It sounds like we have problems for values that are spilled from registers, but that's a QoI issue that doesn't require redesign.

For everything else, we need a way of describing the effects of more powerful optimizations like GVN, DSE, and memcpyopt. We need a way to say, "this variable usually lives in memory, but I deleted the store at .Ltmp1 because it was made dead by the store at .Ltmp2. The value at .Ltmp1 is 42, and at .Ltmp2 it is whatever is in memory." I'm starting to think we should basically use dbg.value with DW_OP_deref as a way to say "at this program point, the variable lives in memory". If that works, we could probably eliminate dbg.declare in favor of dbg.value+DW_OP_deref. Most store-eliminating optimizations know exactly what makes the store dead (free, lifetime.end, memset, or plain store), so they know where to insert the dbg.value+DW_OP_deref to put the variable back in memory.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2061 ↗(On Diff #110643)

Now that we have ValueAsMetadata, these no longer appear in use lists. These case labels are just stale. I'll just commit this separately.

rnk added a comment.Aug 14 2017, 10:07 AM

I think I forgot to write my conclusion, i.e. what we should do with this patch. :)

I think this patch is an incremental step towards what I sketched out. In the long run, the frontend wouldn't emit dbg.declare, it would emit dbg.value+DW_OP_deref. We would never call LowerDbgDeclare. Passes that remove stores would use utilities similar to salvageDebugInfo and ConvertDebugDeclareToDebugValue to insert dbg.values when deleting stores.

I think this patch is an incremental step towards what I sketched out. In the long run, the frontend wouldn't emit dbg.declare, it would emit dbg.value+DW_OP_deref.

That's something I'm not yet sure about. With a dbg.declare it is very clear that the location is not just describing the value of the source variable, but can also be written to by the debugger, when the user wants to assign the variable a new value. The DWARF standard call this implicit vs. explicit location descriptions. We don't really distinguish between these two cases in LLVM yet, but dbg.value vs dbg.declare seems to make sense for it.

That said, this is not something we need to decide on for this patch :-)

Do you think you could factor the code in this patch into llvm::salvageDebugInfo() (found in Local.cpp)? There doesn't seem to be anything specific to InstCombine and this will make it easier to apply it to other passes later.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2061 ↗(On Diff #110643)

Makes sense, thanks!

2138 ↗(On Diff #110643)

I think this got lost in my flurry of other comments...
Don't we need to handle loads (and potentially calls) similarly (see the example in my previous comment)?

rnk added a comment.Aug 14 2017, 5:01 PM

I think this patch is an incremental step towards what I sketched out. In the long run, the frontend wouldn't emit dbg.declare, it would emit dbg.value+DW_OP_deref.

That's something I'm not yet sure about. With a dbg.declare it is very clear that the location is not just describing the value of the source variable, but can also be written to by the debugger, when the user wants to assign the variable a new value. The DWARF standard call this implicit vs. explicit location descriptions. We don't really distinguish between these two cases in LLVM yet, but dbg.value vs dbg.declare seems to make sense for it.

Hm, I hadn't considered that. I skimmed section 2.6 of the dwarf 5 standard, and it talks about "single location descriptions" and "location lists". I assumed that the single location case was just an encoding size optimization, and didn't affect whether the variable could be assigned.

I think variable assignment will be even harder to make work than ensuring that we always present accurate debug variable information. Passes like GVN will eliminate loads that were present in the source program. If the user updates a variable, it may not be reflected in the load immediately on the next line. We might want to disable variable assignment completely when a function isn't marked optnone.

That said, this is not something we need to decide on for this patch :-)

Do you think you could factor the code in this patch into llvm::salvageDebugInfo() (found in Local.cpp)? There doesn't seem to be anything specific to InstCombine and this will make it easier to apply it to other passes later.

Are you suggesting that salvageDebugInfo should call ConvertDbgDeclareToDbgValue(StoreInst*)? Seems doable.


I put together a test case that shows problems after this patch:

void escape(int *);
void g(void);
void f(void) {
  int x = 1;
  g();
  x = 2;
  escape(&x);
}

Without this patch, instcombine inserts dbg.value calls. After this patch, if you break on g, x will probably appear uninitialized. It probably isn't hard to fix DSE in this patch, but I suspect there is a long tail of other passes that delete stores as well.

I wasn't able to craft a test case that causes stack coloring to do anything mean.

rnk retitled this revision from [InstCombine] Don't convert all dbg.declares to dbg.values to [InstCombine] Add a flag to disable LowerDbgDeclare.Sep 6 2017, 2:45 PM
rnk edited the summary of this revision. (Show Details)
rnk updated this revision to Diff 114078.Sep 6 2017, 2:46 PM
  • Conditionalize things under -instcombine-lower-dbg-declare
rnk added a comment.Sep 6 2017, 2:51 PM

I rewrote this to use a cl::opt so that users can try this patch out with an -mllvm flag. It should allow them to get more debug info at the cost of possible innacurate debug info when DSE kicks in as described in PR34136. The long-term solution to PR34136 looks like it's going to take a while, so I want to have a workaround that we can offer to Chromium in the meantime. I expect they might actually use it for Linux and Android crash analysis as well as on Windows.

Does that seem reasonable?

Is this obsoleted by the dbg.addr proposal or orthogonal?

rnk added a comment.Sep 7 2017, 3:18 PM

Is this obsoleted by the dbg.addr proposal or orthogonal?

Orthogonal, I want to do both.

Ok. I think there should be a PR to revert this change that is blocked on PR34136. The description of the option should explicitly spell out a big warning that this will introduce false positives and that it is experimental/temporary only. We should aim to get this reverted as soon as possible.

And we should also stake out a clear path to what is necessary to make this patch obsolete and document what is missing.

rnk added a comment.Sep 11 2017, 1:09 PM

Ok. I think there should be a PR to revert this change that is blocked on PR34136. The description of the option should explicitly spell out a big warning that this will introduce false positives and that it is experimental/temporary only. We should aim to get this reverted as soon as possible.

Are you sure we need a new PR? To fix PR34136, we effectively need to enable the new behavior all the time: stop converting dbg.declare or dbg.addr to dbg.value in instcombine. We just can't do that today because we don't have the infrastructure to fix all the inaccurate debug info that such a change would create. Once we have dbg.addr, we can fix DSE to avoid inaccuracies, but only if we leave dbg.addr alone in instcombine.

Anyway, uploading a new patch shortly with a FIXME comment describing what the flag does and when it can be removed.

rnk updated this revision to Diff 114659.Sep 11 2017, 1:10 PM
  • FIXME comment
hans added a subscriber: hans.Sep 11 2017, 3:00 PM
rnk added a comment.Sep 12 2017, 3:06 PM

Does this look good as is? I'd like to use this to gather some user feedback.

aprantl accepted this revision.Sep 12 2017, 3:36 PM

With the FIXME in the code this LGTM since it leaves no doubt that we want to migrate away from it.

This revision is now accepted and ready to land.Sep 12 2017, 3:36 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 1:45 AM