[PATCH] D49214: [DWARF v5] emit DWARF v5 range lists (no support for fission yet)

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
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);
----------------
wolfgangp wrote:
> dblaikie wrote:
> > 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?)
> My understanding is that DW_AT_ranges has to either use FORM_sec_offset (which needs to be relocated) or FORM_rnglistx, which is an index into the offset table. So with the offset table and FORM_rnglistx we'd be saving the relocations in .debug_info.
Right, this certainly saves relocations - but it's not the only way that could've saved relocations.

Rather than introducing FORM_rnglistx, it could've been specified that a DW_AT_ranges value with a data (not section-relative) form is considered as a byte offset relative to the DW_AT_ranges_base value. That way there would be no need for relocations, and no need for the extra indirection table I think... unless I'm missing something.

(@probinson - any ideas why this is the way it is, with the indirection table?)


================
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);
----------------
probinson wrote:
> dblaikie wrote:
> > wolfgangp wrote:
> > > dblaikie wrote:
> > > > 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?)
> > > My understanding is that DW_AT_ranges has to either use FORM_sec_offset (which needs to be relocated) or FORM_rnglistx, which is an index into the offset table. So with the offset table and FORM_rnglistx we'd be saving the relocations in .debug_info.
> > Right, this certainly saves relocations - but it's not the only way that could've saved relocations.
> > 
> > Rather than introducing FORM_rnglistx, it could've been specified that a DW_AT_ranges value with a data (not section-relative) form is considered as a byte offset relative to the DW_AT_ranges_base value. That way there would be no need for relocations, and no need for the extra indirection table I think... unless I'm missing something.
> > 
> > (@probinson - any ideas why this is the way it is, with the indirection table?)
> Right, the benefit is eliminating one relocation per DW_AT_ranges (all collapsed into a single relocation per CU for DW_AT_rnglists_base).  Overall it would be a slight size increase in the linked file, as each reference would cost an extra byte or two total.  Reducing the linker's effort (and what, 24 bytes in the .o per relocation?) seemed like the right tradeoff.
Right - I get the desire to remove a reloc per range list reference. But I don't understand why that solution created the indirection table in debug_rnglist to go from range list indexes to range list offsets? What problem did that indirection solve compared to having range list offsets (relative to DW_AT_ranges_base or whatnot) directly in the DW_AT_ranges attributes?


================
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.");
----------------
wolfgangp wrote:
> dblaikie wrote:
> > 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?
> I thought the check here was only used in case the -no-dwarf-ranges-section option was on,  so we still need the check for the normal case.
Ah, right - good point.

Might still be nice to refactor to have this somewhat complicated expression in one place rather than two. Could skip the old-school pre-v5 ranges emission (even though it's a no-op) when the list is empty? Dunno - no big deal either way, just a thought.


Repository:
  rL LLVM

https://reviews.llvm.org/D49214