This is an archive of the discontinued LLVM Phabricator instance.

2/2: [lldb] Fix DW_TAG_GNU_call_site-DW_AT_low_pc as produced by GCC
ClosedPublic

Authored by jankratochvil on Jun 6 2020, 12:46 PM.

Details

Summary

D80519 added support for DW_TAG_GNU_call_site but Bug 45886 found one case did not work.
There is:

0x000000b1:     DW_TAG_GNU_call_site
                  DW_AT_low_pc  (0x000000000040111e)
                  DW_AT_abstract_origin (0x000000cc "a")
...
0x000000cc:   DW_TAG_subprogram
                DW_AT_name      ("a")
                DW_AT_prototyped        (true)
                DW_AT_low_pc    (0x0000000000401109)
                ^^^^^^^^^^^^ - here it did overwrite the 'low_pc' variable containing value 0x40111e we wanted
                DW_AT_high_pc   (0x0000000000401114)
                DW_AT_frame_base        (DW_OP_call_frame_cfa)
                DW_AT_GNU_all_call_sites        (true)

Diff Detail

Event Timeline

jankratochvil created this revision.Jun 6 2020, 12:46 PM
jankratochvil planned changes to this revision.Jun 6 2020, 1:27 PM

It regressed other testcases, I have to check it more.

The new -1 value for GetAttributes is a bit ugly. But otherwise either

  • CollectCallEdges would need to reimplement GetAttributes.

or

  • trying to guess which attribute belongs to which DIE from DIEOffsetAtIndex but that is also not nice because it is offset of the DIE's attribute, not offset of the DIE itself.
jankratochvil retitled this revision from [lldb] Fix DW_TAG_GNU_call_site-DW_AT_low_pc to [lldb] Fix DW_TAG_GNU_call_site-DW_AT_low_pc as produced by GCC.
# This tests that lldb is compatible with DWARF-4 entry values GNU extension
# with DW_TAG_GNU_call_site attributes order as produced by GCC:
# 0x000000b1:     DW_TAG_GNU_call_site
#                   DW_AT_low_pc  (0x000000000040111e)
#                   DW_AT_abstract_origin (0x000000cc "a")
# clang produces the attributes in opposite order:
# 0x00000064:     DW_TAG_GNU_call_site
#                   DW_AT_abstract_origin (0x0000002a "a")
#                   DW_AT_low_pc  (0x0000000000401146)
labath added a comment.Jun 8 2020, 2:43 AM

Thanks for tracking this down, and for creating a nice test case. I think the implementation would be cleaner with a new argument, as per the inline comment.

lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.h
113–115

Using -1 to prevent recursion is pretty unobvious. I think it would be better to add a bool recurse = true argument between attributes and depth. In fact, I'd consider even deleting the depth argument -- it's an internal detail that noone except DWARFDebugInfoEntry should be using and the single usage there can be easily changed to spec_die.GetDIE()->GetAttributes(spec_die.GetUnit(), attributes, recurse, depth+1)

lldb/test/Shell/SymbolFile/DWARF/DW_TAG_GNU_call_site-DW_AT_low_pc.s
15

Maybe place an int3 at the place where you want this to stop? Then you can delete all of the line table directives and the break location will be more obvious...

jankratochvil retitled this revision from [lldb] Fix DW_TAG_GNU_call_site-DW_AT_low_pc as produced by GCC to 2/2: [lldb] Fix DW_TAG_GNU_call_site-DW_AT_low_pc as produced by GCC.
jankratochvil marked 4 inline comments as done.Jun 8 2020, 1:03 PM
jankratochvil added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.h
113–115

I had this idea. :-) The problem is bool recurse is type-compatible with uint32_t depth which leads to uncaught bugs. Given that non-recursive call makes no sense with depth>0 I find the special value depth = -1 to be suitable for non-recursive calls.
Anyway implemented the recurse parameter in a safe way if it is worth it to separate it.

lldb/test/Shell/SymbolFile/DWARF/DW_TAG_GNU_call_site-DW_AT_low_pc.s
15

Done, nice.

labath accepted this revision.Jun 9 2020, 2:16 AM
labath added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.h
113–115

Right, I see. I don't think that a special tagged enum is really necessary here and flushing out the callers via two patches (like you did) would be enough. The only ones who that may impact are downstream users who merge in bulk, but: a) I doubt the downstream users are calling this function in their own code; b) we don't have to work extra hard to safeguard them.

However, given that you've already done it, I think it's fine to keep it -- though I'll probably change it back next time I work on this function.

lldb/test/Shell/SymbolFile/DWARF/DW_TAG_GNU_call_site-DW_AT_low_pc.s
47

This is formatted strangely. Tab issues?

This revision is now accepted and ready to land.Jun 9 2020, 2:16 AM
jankratochvil marked 5 inline comments as done.Jun 9 2020, 4:48 AM
jankratochvil added inline comments.
lldb/test/Shell/SymbolFile/DWARF/DW_TAG_GNU_call_site-DW_AT_low_pc.s
47

Yes, thanks. Some *.s files have tabs some spaces so I kept it with tabs (and fixed this one line).

This revision was automatically updated to reflect the committed changes.
jankratochvil marked an inline comment as done.
llunak added a subscriber: llunak.Aug 2 2020, 9:11 AM

The DWARF in the source file appears to be broken, the test fails for me on openSUSE 15.2 x86_64, with errors about DIE references outside of its CU. It fails even if I try to manually compile and debug the file, with Clang git or 9, GCC 7.5 or 9, LLDB git or 9, GDB 9.2.

seli@slim:~/tmp> clang -o DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp DW_TAG_GNU_call_site-DW_AT_low_pc.s
seli@slim:~/tmp> lldb DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp 
(lldb) target create "DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp"
Current executable set to '/home/seli/tmp/DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp' (x86_64).
(lldb) r
Process 20675 launched: '/home/seli/tmp/DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp' (x86_64)
error: DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp DW_FORM_ref* DIE reference 0x24f is outside of its CU
error: DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp DW_FORM_ref* DIE reference 0x256 is outside of its CU
Process 20675 stopped
* thread #1, name = 'DW_TAG_GNU_call', stop reason = signal SIGTRAP
    frame #0: 0x0000000000400520 DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp`a(p=<unavailable>) + 11
DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp`a:
->  0x400520 <+11>: retq   

DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp`main:
    0x400521 <+0>:  movl   $0x1, %edi
    0x400526 <+5>:  callq  0x400515                  ; a
    0x40052b <+10>: movl   $0x0, %eax
(lldb) p p
error: DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp DW_FORM_ref* DIE reference 0x24f is outside of its CU
error: DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp DW_FORM_ref* DIE reference 0x24f is outside of its CU
error: <lldb wrapper prefix>:43:31: no member named 'p' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::p;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'p'
p
^
seli@slim:~/tmp> gcc -o DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp DW_TAG_GNU_call_site-DW_AT_low_pc.s
seli@slim:~/tmp> gdb -q DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp
Reading symbols from DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp...
Dwarf Error: Cannot find DIE at 0x24f referenced from DIE at 0x158 [in module /home/seli/tmp/DW_TAG_GNU_call_site-DW_AT_low_pc.s.tmp]
jankratochvil added a comment.EditedAug 2 2020, 1:45 PM

The DWARF in the source file appears to be broken, the test fails for me on openSUSE 15.2 x86_64, with errors about DIE references outside of its CU. It fails even if I try to manually compile and debug the file, with Clang git or 9, GCC 7.5 or 9, LLDB git or 9, GDB 9.2.

Thanks for the bugreport. SuSE must be linking some DWARF by default into all executables. Checked in a fix.