Page MenuHomePhabricator

[DWARF] Incorrect prologue end line record.
ClosedPublic

Authored by CarlosAlbertoEnciso on Sep 8 2017, 7:30 AM.

Details

Summary

The prologue-end line record must be emitted after the last instruction that is part of the function frame setup code and before the instruction that marks the beginning of the function body.

For the below test:

1 extern int get_arg();
2 extern void func(int x);
3
4 int main()
5 {
6 int a;
7 func(get_arg());
8 }
9

The prologue-end line record is emitted with an incorrect associated address, which causes a debugger to show the beginning of function body to be inside the prologue.

This can be seen in the following trimmed assembler output:

main:
  ...
  .loc	1 7 0 prologue_end
  pushq	%rax
.Lcfi0:
  .cfi_def_cfa_offset 16
  callq	_Z7get_argv
  ...
  retq

The instruction 'pushq %rax' is part of the frame setup code.

The correct location for the prologue-end line information is just before the call to '_Z7get_argv', as illustrated in the following trimmed assembler output:

main:
  ...
  pushq	%rax
.Lcfi0:
  .cfi_def_cfa_offset 16
  .loc	1 7 0 prologue_end
  callq	_Z7get_argv
  ...
  retq

Diff Detail

Repository
rL LLVM

Event Timeline

probinson edited edge metadata.Sep 8 2017, 11:27 AM

I think the argument (and the test) would be stronger if it was based on the instruction sequence rather than offsets. For example if you generate asm and then show prologue_end is immediately before the call to get_arg.
Also we generally like to trim the IR source by removing unnecessary attribute sets.

I'm actually more curious why part of the prologue is associated with line 5 and part with line 7. That feels like there's some pass mis-handling debug locations, and it would be preferable to find that rather than have DwarfDebug cover it up.

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Sep 11 2017, 6:33 AM
CarlosAlbertoEnciso edited the summary of this revision. (Show Details)Sep 11 2017, 6:53 AM

Following the review comments:

  1. Changed the test to be based on the instruction sequence (asm) rather than offsets.
  2. Removed unnecessary attribute sets from the IR source.

I'm actually more curious why part of the prologue is associated with line 5 and part with line 7. That feels like there's some pass mis-handling debug locations, and it would be preferable to find that rather than have DwarfDebug cover it up.

I understand your valid point, but I think the check introduced by this proposed patch will guarantee that no line records are associated to instructions that are part of the frame setup code.

May I suggest, to keep the extra check and investigate why part of the prologue is associated with line 5 and part with line 7 as a second part of the problem, unless you feel otherwise.

aprantl added inline comments.Sep 11 2017, 8:30 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1161 ↗(On Diff #114590)

Duplicate line?

probinson accepted this revision.Sep 11 2017, 10:51 AM

I see that findPrologueEndLoc() is also checking the FrameSetup flag, so at least this makes the handling consistent, and relying on FrameSetup actually seems cleaner than relying on source locations. If you have time to investigate why the source locations seem wrong, that would be great.
LGTM after you fix the comment Adrian pointed out.

This revision is now accepted and ready to land.Sep 11 2017, 10:51 AM

Following the review comments:

Removed a duplicated line.

CarlosAlbertoEnciso marked an inline comment as done.Sep 12 2017, 7:38 AM

As I do not have commit access, Robert Lougher will submit this for me.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1161 ↗(On Diff #114590)

Thanks. You are correct; that is a duplicated line. Fixed.

This revision was automatically updated to reflect the committed changes.

This was reverted in r313057 as it was causing buildbot failure (lldb inline stepping tests).

bjope added a subscriber: bjope.Sep 29 2017, 12:41 AM
bjope removed a subscriber: bjope.
pmatos added a subscriber: pmatos.Oct 19 2017, 2:41 AM

Was this ever fixed and reapplied? The last I can find on this was r313057 where @rob.lougher reverted r313047.

CarlosAlbertoEnciso marked an inline comment as done.Oct 24 2017, 11:23 PM

Was this ever fixed and reapplied? The last I can find on this was r313057 where @rob.lougher reverted r313047.

Hi,

Sorry about my delay in replying.

The compiler fix, caused couple of LLDB tests to fail.

llvm/tools/lldb/packages/Python/lldbsuite/test/functionalities/inline-stepping

Those tests have been under discussion with the LLDB people. The final changes to be submitted for review.

After that, the compiler fix should be reapplied.

Thanks.

davide added a subscriber: davide.Jan 5 2018, 7:36 AM