This is an archive of the discontinued LLVM Phabricator instance.

Another prototype fix for lld DWARF parsing of base address selection entries in range lists
ClosedPublic

Authored by grimar on Aug 28 2017, 8:12 AM.

Details

Summary

This is based on David Blaikie's patch D36097 and
solves issue of wrong section index evaluating for ranges when
base address is used.

Diff Detail

Event Timeline

grimar created this revision.Aug 28 2017, 8:12 AM
grimar updated this revision to Diff 113255.Aug 30 2017, 6:56 AM
grimar edited edge metadata.
grimar edited the summary of this revision. (Show Details)
grimar edited reviewers, added: dblaikie, rafael; removed: grimar.
grimar added subscribers: llvm-commits, grimar, evgeny777.
  • Added testcase.
dblaikie added inline comments.Aug 30 2017, 1:39 PM
include/llvm/DebugInfo/DWARF/DWARFUnit.h
270–271

So why is this bit necessary? Should callers just not try to set a base address if they don't want to, rather than having setBaseAddress ignore the request?

lib/DebugInfo/DWARF/DWARFDebugRangeList.cpp
71–72

Is this clang-formatted? I'd expect no spaces inner sides of the {}

grimar updated this revision to Diff 113364.Aug 31 2017, 1:07 AM
  • Addressed review comments.
include/llvm/DebugInfo/DWARF/DWARFUnit.h
270–271

It is for dwarfdump-ranges-unrelocated.s testcase. It is based on following code:

test.cpp:
   void foo1() { }  
   void foo2() { }  
clang test.cpp -S -o test.s -gmlt -ffunction-sections

And output produced is:

	.section	.debug_info,"",@progbits
.Lcu_begin0:
	.long	38                      # Length of Unit
	.short	4                       # DWARF version number
	.long	.debug_abbrev           # Offset Into Abbrev. Section
	.byte	8                       # Address Size (in bytes)
	.byte	1                       # Abbrev [1] 0xb:0x1f DW_TAG_compile_unit
	.long	.Linfo_string0          # DW_AT_producer
	.short	4                       # DW_AT_language
	.long	.Linfo_string1          # DW_AT_name
	.long	.Lline_table_start0     # DW_AT_stmt_list
	.long	.Linfo_string2          # DW_AT_comp_dir
	.quad	0                       # DW_AT_low_pc
	.long	.Ldebug_ranges0         # DW_AT_ranges
	.section	.debug_ranges,"",@progbits
.Ldebug_ranges0:
	.quad	.Lfunc_begin0
	.quad	.Lfunc_end0
	.quad	.Lfunc_begin1
	.quad	.Lfunc_end1
	.quad	0
	.quad	0

So it has ranges but also unrelocated zero base address is emited in debug info. Since it has no
corresponding relocation (SectionIndex == -1ULL), I am ignoring it to prevent using
-1ULL as section index name. That looks a bit hacky, but I currently see no other way to distinguish
between such "dummy" base address and real one.
In testcase it is shown that base address can be 0 after relocation is applied, so I think we need
to check both SectionIndex and relocated value, just like I do.

I moved that code to caller side, thats probably a bit more clear and localizes the hack.

lib/DebugInfo/DWARF/DWARFDebugRangeList.cpp
71–72

You right, fixed.

grimar updated this revision to Diff 113529.Sep 1 2017, 3:33 AM
  • Added testcase for executables.
  • Updated code accordinng to discussion in corresponding thread.
dblaikie accepted this revision.Sep 1 2017, 1:01 PM

Couple of minor things to address before you commit this (no need to send it for further review though - I'll catch anything else in post-commit), but otherwise looks good.

lib/DebugInfo/DWARF/DWARFFormValue.cpp
296

should getRelocatedValue handle this case (if there is no section, it coudl set the SectionIndex to -1ULL) so SectionIndex is welldefined after a call to getRelocatedValue, even when the section index is not known?

test/DebugInfo/X86/dwarfdump-ranges-baseaddr.s
65–68

Add a base address selection entry in this list as well (so let the first entry rely on the CU's default base address, then use a base address selection entry to switch base address (maybe switch section too to make it obvious in the CHECKs)), to test that codepath.

This revision is now accepted and ready to land.Sep 1 2017, 1:01 PM
grimar added a comment.Sep 4 2017, 3:31 AM

Thanks for review, David ! All your comments were addressed.

This revision was automatically updated to reflect the committed changes.