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

[Paul Robinson via Phabricator via llvm-commits]

probinson 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, 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.


================
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() {
----------------
dblaikie wrote:
> 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?
Maybe just "Emit address ranges into the .debug_ranges or .debug_rnglists section."  Someday there will be a DWARF 6 and mostly likely it will use the same layout as v5.


================
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) {
----------------
wolfgangp wrote:
> dblaikie wrote:
> > 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.
> Seems like this comment needs to be reworked for DWARF 5 anyway with some information on  what is DWARF 5 specific and what's not. It also should go into the header, no? Let me take a stab at that in another review.
Replace all the commas with semicolons, then it reads correctly (the 'or' here is an exclusive or).


Repository:
  rL LLVM

https://reviews.llvm.org/D49214