This is an archive of the discontinued LLVM Phabricator instance.

Generate better location ranges for some register-described variables.
Needs ReviewPublic

Authored by samsonov on May 27 2014, 5:41 PM.

Details

Reviewers
dblaikie
Summary

Don't terminate location ranges for register-described variables
at the end of machine basic block if this register is never modified
in the function body (except for the frame setup and the last machine
basic block).

This patch is mostly targeted to fix non-trivial debug locations for
variables addressed via stack and frame pointers (%rsp, %rbp).

It is not really a generic fix. We can still produce poor debug info
for register-described variables if this register *is* modified somewhere
in the function, but in unrelated places. This might be the case for the debug
info in optimized binaries (e.g. for local variables in inlined functions).
However, proper fix requires a control-flow analysis and should probably be
done in a different place (register allocator) and actually modify DBG_VALUE
instructions to keep AsmPrinter straightforward.

I'd like to try and proceed with a fix in this patch, as it is simple
and helps in many important cases, e.g. it is intended to fix completely
broken debug info with AddressSanitizer and fix PR19307 (missing debug info
for by-value std::string arguments at -O0).

Diff Detail

Event Timeline

samsonov updated this revision to Diff 9857.May 27 2014, 5:41 PM
samsonov retitled this revision from to Generate better location ranges for some register-described variables..
samsonov updated this object.
samsonov edited the test plan for this revision. (Show Details)
samsonov added a reviewer: dblaikie.
samsonov added subscribers: aprantl, probinson, joerg, Unknown Object (MLST).

When determining ChangingRegs, we should also ignore the function's epilogue. Otherwise we will drop debug info for (e.g., spilled) variables [FrameIndex+Offset], because the frame pointer itself is restored in the epilogue and thus marked as changing when it really isn't. Unfortunately there is no FrameSetup flag for epilogue instructions, but I think it would be safe to just make an exception for the frame pointer.

Also I realized we need to have the spiller insert DBG_VALUEs after reloads, but that's only tangentially related.

Yes... I don't know a good way to find out what the function epilogue is at this stage. Current code will work for frame pointer and stack pointer if we are lucky enough and epilogue is actually the last machine basic block in the function (it is so for the simple cases).

We could do something similar to the prologue and actually require
(well, try to require) backends to annotate epilogue instructions
appropriately.

It's probably the easiest idea at least.

-eric

Yeah, well, we can add "FrameTeardown" machine instruction flag in addition to "FrameSetup", and annotate machine instructions added in frame lowering code. But do we expect many users of this flag (assuming that this usage is also somewhat questionable)?

I can also make this patch even less general and just special-case the frame pointer and the stack pointer registers, so that the code wouldn't pretend to solve a general problem we're facing.

samsonov updated this revision to Diff 10112.Jun 4 2014, 5:06 PM

Change the strategy for discovering function epilogue following
suggestion by Adrian. Assume that instruction is in epilogue if:

  1. its basic block ends with a return instruction, and
  2. its debug location is the same as debug location of this return instruction.
dblaikie edited edge metadata.Jun 4 2014, 5:18 PM

I do rather feel this perhaps rises beyond my idea comfort level for "work around" to "we should probably fix this the right way", but I realize I'll probably be argued down on this.

Out of curiosity, does anyone happen to be able to explain LiveDebugVariables.cpp:956-966. It /looks/ like this code, based on the comment, is trying to do what we want: add dbg.value intrinsics to every basic block for which the variable is live. Any idea if this works? Why it doesn't? etc...