[PATCH] D42707: [DWARFv5] Emit .debug_line_str (in a non-DWO file)

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:78
+  /// Indicate whether we're emitting a .debug_line_str section.
+  operator bool() { return LineStrLabel != nullptr; }
+};
----------------
probinson wrote:
> dblaikie wrote:
> > Any op bool should be marked explicit - otherwise there's all manner of implicit conversions that can get weird.
> Got it.
> Would you rather have it be a normal predicate method?  I went with operator bool mainly for compactness.
Nah - op bool is OK, if it's expliict - and it'll still be usable implicitly in bool contexts like "if (x)".

Though, alternatively, I wonder if it'd be better to not give this object the extra state - and pass through a null pointer instead of a 'false' MCDwarfLineStr object? It looks like it's only constructed in one place & so the check for DWARF version could go there rather than inside this class & the other caller could pass null?


================
Comment at: llvm/lib/MC/MCDwarf.cpp:482
   if (LineTableVersion >= 5)
-    emitV5FileDirTables(MCOS);
+    emitV5FileDirTables(MCOS, LineStr);
   else
----------------
probinson wrote:
> dblaikie wrote:
> > I'm not quite sure - is there a reason that emitV5FileDirTables handle the whole MCDwarfLineStr functionality internally without having to pass in an MCDwarfLineStr object? (emitV5FileDirTables could create the MCDwarfLineStr locally, potentially & emit it immediately?)?
> When there are multiple CUs (e.g. LTO) we want the MCDwarfLineStr instance to span emitting all of them.  Otherwise we can get multiple instances of the same path in the same .debug_line_str section, in a single .o file.
> I could have put MCDwarfLineStr in the MCContext, but this way we have better data hiding.
> Also we need a parameter of some kind anyway, to distinguish the DWO and non-DWO cases.
Ah, right, the line table's one of the things that's not shared between CUs (unlike the abbrev table and string pool, etc - and the line string pool).


================
Comment at: llvm/test/MC/ELF/debug-md5.s:20-25
+# CHECK: .debug_line_str contents:
+# CHECK-NEXT: 0x00000000: ""
+# CHECK-NEXT: 0x00000001: "dir1"
+# CHECK-NEXT: 0x00000006: "dir2"
+# CHECK-NEXT: 0x0000000b: "foo"
+# CHECK-NEXT: 0x0000000f: "bar"
----------------
probinson wrote:
> probinson wrote:
> > dblaikie wrote:
> > > Any way to test that the line table is actually using the indexes into the debug_line_str section?
> > Maybe, when printing the directory/file names we could print the section+index when they're indirect.  I'll look into that.
> So, this would be a missing feature in llvm-dwarfdump, which doesn't provide a way to dump the section/index pointed to by an indirect reference in the line table (like it does in .debug_info).  In fact by the time we're dumping the line table header, the relevant information is long gone.
> That should be its own patch.  Follow-up, or put this on hold to wait for it?
(replied to this on-list too, but: Whichever order you like)


Repository:
  rL LLVM

https://reviews.llvm.org/D42707