This should enable the linker to do string-pooling of path names.
Details
Diff Detail
Event Timeline
Looks reasonable.
llvm/lib/MC/MCDwarf.cpp | ||
---|---|---|
313 | Sounds like a std::vector<uint_8t> or something would also be sufficient (but I'm not sure if that's any more efficient). |
llvm/lib/MC/MCDwarf.cpp | ||
---|---|---|
313 | This sequence is cargo-culting StringTableBuilder::write(raw_ostream&). I don't think a std::vector would be any more efficient, and EmitBinaryData wants a StringRef anyway which I'd have to construct manually. |
llvm/lib/MC/MCDwarf.cpp | ||
---|---|---|
78 | Any op bool should be marked explicit - otherwise there's all manner of implicit conversions that can get weird. | |
473 | 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?)? | |
llvm/test/MC/ELF/debug-md5.s | ||
18–23 | Any way to test that the line table is actually using the indexes into the debug_line_str section? |
llvm/lib/MC/MCDwarf.cpp | ||
---|---|---|
78 | Got it. | |
473 | 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. | |
llvm/test/MC/ELF/debug-md5.s | ||
18–23 | Maybe, when printing the directory/file names we could print the section+index when they're indirect. I'll look into that. |
llvm/test/MC/ELF/debug-md5.s | ||
---|---|---|
18–23 | 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. |
llvm/lib/MC/MCDwarf.cpp | ||
---|---|---|
78 | 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? | |
473 | 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). | |
llvm/test/MC/ELF/debug-md5.s | ||
18–23 | (replied to this on-list too, but: Whichever order you like) |
Address review comments:
Pass the MCDwarfLineStr around by pointer, eliminating the operator-bool;
use new dwarfdump feature in the test to show the section for the string references;
put more responsibility on the (only) user of the class.
That last bit is not really my preference, but as this new class is basically an implementation detail within MCDwarf.cpp I think it should be okay.
llvm/lib/MC/MCDwarf.cpp | ||
---|---|---|
245–246 | Maybe using an Optional<MCDwarfLineStr> (you've got the bool and the MCDwarfLineStr there already, this would just wrap it up together & avoid the construction of an MCDwarfLineStr when it's not used) |
Any op bool should be marked explicit - otherwise there's all manner of implicit conversions that can get weird.