Page MenuHomePhabricator

[LiveDebugValues] Extend var ranges through artificial blocks
ClosedPublic

Authored by vsk on Oct 4 2018, 6:59 PM.

Details

Summary

ASan often introduces basic blocks consisting exclusively of
instructions without debug locations, or with line 0 debug locations.

LiveDebugValues needs to extend variable ranges through these artificial
blocks. Otherwise, a lot of variables disappear -- even at -O0.

Typically, LiveDebugValues does not extend a variable's range into a
block unless the block is essentially "part of" the variable's scope
(for a precise definition, see LexicalScopes::dominates). This patch
relaxes the lexical dominance check for artificial blocks.

This makes the following Swift program debuggable at -O0:

1| var x = 100
2| print("x = \(x)")

rdar://39127144

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Oct 4 2018, 6:59 PM

Looks good to me. Just one question, why didn't you use MIR as test file?

Good find! We spent a considerable amount of time tweaking the performance of this pass. Before the scope checking was added functions would often have thousands of dead dbg.values inserted.

May I ask you to do a time ninja clang of both an ASANified/UBSANified clang and a RelWithDebInfo clang to make sure this doesn't regress performance? It would also be good to spot-check the memory usage (but runtime is likely a good proxy).

aprantl added inline comments.Oct 5 2018, 9:02 AM
llvm/lib/CodeGen/LiveDebugValues.cpp
772 ↗(On Diff #168429)

For readability I think I would prefer this to be in a separate collect/initCompilerGeneratedBlocks helper function.

vsk updated this revision to Diff 168525.Oct 5 2018, 2:16 PM
vsk marked an inline comment as done.

Address review feedback.

  • @NikolaPrica The MIR parser has bugs causing it to be extremely sensitive around comments. I couldn't write a reasonable a set of CHECK lines the MIR parser would accept.
  • Split out logic to label artificial blocks from the initialization step.
vsk added a comment.Oct 5 2018, 2:16 PM

@aprantl I built stage2 ASan+UBsan+RelWithDebInfo versions of clang with unpatched and patched versions of clang.

The unpatched clang finished in 936 seconds with a peak RSS of 999,268 KB (0.95 GB). The patched clang finished in 853 seconds with a peak RSS of 999,952 KB (0.95 GB). I made measurements by sampling ps x -o rss,command | grep clang every second and keeping track of the entry with max RSS. I can share the measurement script if there’s interest.

I’m confident that these measurements show this patch doesn’t cause unbounded memory consumption. I don’t know what we can say about the compile-time impact due to the low sample size (2 builds). However, I expect compile times to improve. Instead of spending time re-checking instruction locations in LexicalScopes::dominates(), this patch just does a set lookup.

I looked at the final dSYMs to see what happens to the debug info. This patch increases the percentage of scope bytes covered by 1% (expected). It doesn’t really increase the number of variables with locations (also expected; their ranges simply grow).

aprantl accepted this revision.Oct 5 2018, 2:27 PM

Thanks, looks like we didn't accidentally introduce a regression!

This revision is now accepted and ready to land.Oct 5 2018, 2:27 PM
This revision was automatically updated to reflect the committed changes.