[PATCH] D49214: [DWARF v5] emit DWARF v5 range lists (no support for fission yet)
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 13 11:10:19 PDT 2018
dblaikie added a subscriber: probinson.
dblaikie added inline comments.
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:424
+ if (DD->getDwarfVersion() >= 5)
+ addRnglistsBase();
+ }
----------------
It looks like this would end up adding a range list base address to the CU for every range emitted - wouldn't it?
Yeah, looks like it does:
DW_AT_rnglists_base (0x0000000c)
DW_AT_low_pc (0x0000000000000000)
DW_AT_ranges (0x0000002b
[0x0000000000000000, 0x0000000000000033)
[0x0000000000000040, 0x0000000000000046)
[0x0000000000000000, 0x0000000000000006))
DW_AT_rnglists_base (0x0000000c)
using this input:
bool x;
void f1() {
f1();
if (bool b = x) {
f1();
f1();
}
}
__attribute__((section(".text.foo"))) void f2() { }
void f3() { }
Compile with dwarf5 to LLVM IR, reorder the 3 calls to f1 (inserting the first call between the second and third) to cause the scope of 'b' to have a hole in the middle (for that first call to f1) & so to be described by a range list rather than high/low pc.
Looks like the range list base attribute should be added in the same place DW_AT_GNU_ranges_base is set, I think?
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2016-2021
BaseIsSet = false;
- Asm->OutStreamer->EmitIntValue(-1, Size);
+ if (DwarfVersion >= 5)
+ Asm->OutStreamer->EmitIntValue(dwarf::DW_RLE_base_address, 1);
+ else
+ Asm->OutStreamer->EmitIntValue(-1, Size);
Asm->OutStreamer->EmitIntValue(0, Size);
----------------
I suspect this section doesn't make sense for DWARFv5 style range lists - there would never be a need to reset the base address to zero, because the forms themselves encode whether they are base address relative or not.
(in the old style range lists, you'd have to reset the base address to zero because the address pairs didn't describe whether they were absolute or relative - so setting it to zero would make the following address pairs absolute (relative to zero). It'd look more like "<base address> <relative pair> <relative pair> <base address 0> <abs pair> <abs pair>" in the new form it's "base address, relative pair, absolute pair, relative pair, etc" - absolute and relative pairs can be interleaved and each is self-descriptive about its absolute or relativeness)
Maybe there are some missing test cases too? Though they should be/probably can be covered by the original testing for UseDwarfRangesBaseAddressSpecifier - but so you might be able to reuse the test input for that ( test/DebugInfo/X86/range_reloc.ll ), but use DWARFv5 instead of UseDwarfRangesBaseAddressSpecfier.
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2060-2068
+ // Don't emit a rangelist table if there are no ranges.
+ if (llvm::all_of(CUMap,
+ [](const decltype(CUMap)::const_iterator::value_type &Pair) {
+ DwarfCompileUnit *TheCU = Pair.second;
+ if (auto *Skel = TheCU->getSkeleton())
+ TheCU = Skel;
+ return TheCU->getRangeLists().empty();
----------------
Is it the best tradeoff to produce one shared range list, rather than a separate range list per CU? (I ask/think about that here, because then you wouldn't need a scan through all the CUs to see if any are non-empty)
Looks like the tradeoff is whether the bytes spent in the header are saved by reduced bytes in the debug_info section of being able to use smaller indexes, encoded with shorter encodings. I would imagine in practice for builds with multiple CUs (optimized/LTO situations) there are many ranges and the tradeoff tips in favor of using multiple separate range list lists - one for each CU?
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2092-2095
+ // FIXME: Generate the offsets table and use DW_FORM_rnglistx with the
+ // DW_AT_ranges attribute. Until then set the number of offsets to 0.
+ Asm->emitInt32(0);
+ Asm->OutStreamer->EmitLabel(RnglistTableBaseSym);
----------------
To be honest, I'm not sure why the standard added this offset table - it seems to add extra bytes without any savings I can see (shorter encodings in the debug_info section - using indexes instead of offsets, but it doesn't save relocations or bytes overall, unless I'm missing something?). Meh. (@probinson - any ideas on what the purpose of this indirection is?)
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2109-2110
-/// Emit address ranges into a debug ranges section.
+/// Emit address ranges into the .debug_ranges section or DWARF v5 rangelists
+/// into the .debug_rnglists section.
void DwarfDebug::emitDebugRanges() {
----------------
I might rephrase this as "into the .debug_ranges section or, in DWARFv5 the .debug_rnglists section" - the current phrasing sounds like there are two different kinds of things "address ranges" and "rangelists" - to me they're the same thing, encoded differently (depending on the section).
Perhaps others have a different opinion on how this reads, etc?
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2115-2123
if (!useRangesSection()) {
assert(llvm::all_of(
CUMap,
[](const decltype(CUMap)::const_iterator::value_type &Pair) {
return Pair.second->getRangeLists().empty();
}) &&
"No debug ranges expected.");
----------------
Looks like the precondition that there are some ranges is already tested here - so no need to check it in emitDebugRnglists? the check there could be another assert, perhaps?
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2130-2144
// Start the dwarf ranges section.
Asm->OutStreamer->SwitchSection(
Asm->getObjFileLowering().getDwarfRangesSection());
// Grab the specific ranges for the compile units in the module.
for (const auto &I : CUMap) {
DwarfCompileUnit *TheCU = I.second;
----------------
Might be worth splitting this out into a separate function so it's symmetric with emitDebugRnglists?
================
Comment at: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2222
// DW_AT_low_pc, DW_AT_high_pc, DW_AT_ranges, DW_AT_dwo_name, DW_AT_dwo_id,
-// DW_AT_addr_base, DW_AT_ranges_base.
+// DW_AT_addr_base, DW_AT_ranges_base or DW_AT_rnglists_base.
DwarfCompileUnit &DwarfDebug::constructSkeletonCU(const DwarfCompileUnit &CU) {
----------------
Missing oxford comma between 'DW_AT_ranges_base' and 'or'. Though it also reads a little ambiguously - in English 'or' can often be read as 'exclusive or' (why some people put 'and/or').
Maybe "This DIE can have any/all of the following attributes: x, y, and z"? No big deal, just some thoughts.
================
Comment at: llvm/trunk/test/DebugInfo/X86/rnglists-nobase.ll:11-19
+; class A
+; {
+; public:
+; A();
+; };
+;
+; static A glob;
----------------
This example might obscure what's really going on by having implicit functions from the global static initializer - it may be more illustrative to provide a more explicit example using section attributes to show which functions are in which sections, etc.
The existence of 3 functions wouldn't itself create ranges (if they were all in a single .text section they'd be covered by a single high/low pc), it's the fact that the static initializer for 'glob' creates functions in a .text.startup section. And that those two static initializer functions aren't emitted together - one, then 'foo', then the other (splitting them up - otherwise there'd be only two ranges, instead of 3)
__attribute__((section(".text.foo"))) void f1() {}
__attribute__((section(".text.bar"))) void f2() {}
__attribute__((section(".text.foo"))) void f3() {}
and I'd expect the optimal range list (in the absence of a debug_addr section) for this source to be:
DW_RLE_base_address (.text.foo+0)
DW_RLE_offset_pair (f1 begin/end relative to .text.foo+0)
DW_RLE_offset_pair (f3 begin/end relative to .text.foo+0)
DW_RLE_start_length (f2 begin, f2 length)
(aside: shorter encodings might be achievable (though maybe not sufficiently worthwhile/only a small improvement) if RLE_start_length implicitly set the base address, and if offset_pair was offset_length instead - but at least there's room to improve this over versions (there's versioning in the header, there's space in the RLE_* enum space, etc))
I think the reason this is missing the base_address+offset_pair opportunity is because the code is predicated on UseDwarfRangesBaseAddressSpecifier - which defaults to off, because I found some DWARF consumers couldn't handle it, I think. But that property isn't relevant to DWARFv5 - so the implementation should be modified to do the same thing no matter the value of 'UseDwarfRangesBaseAddressSpecifier'.
Repository:
rL LLVM
https://reviews.llvm.org/D49214
More information about the llvm-commits
mailing list