[PATCH] D49670: dwarfgen: Add support for generating the debug_str_offsets section

[Pavel Labath via Phabricator via llvm-commits]

labath marked an inline comment as done.
labath added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfStringPool.h:40
+  void emitStringOffsetsTableHeader(AsmPrinter &Asm, MCSection *OffsetSection,
+                                    MCSymbol *StartSym);
+
----------------
dblaikie wrote:
> Is a forward decl of MCSymbol required up with MCSection/AsmPrinter then?
It wasn't necessary in my build, but it's a good idea nonetheless.


================
Comment at: unittests/DebugInfo/DWARF/DwarfGenerator.cpp:473-476
+  StringPool->emitStringOffsetsTableHeader(*Asm, MOFI->getDwarfStrOffSection(),
+                                           StringOffsetsStartSym);
+  StringPool->emit(*Asm, MOFI->getDwarfStrSection(),
+                   MOFI->getDwarfStrOffSection());
----------------
dblaikie wrote:
> Why split this into two functions rather than having emit do the table header itself/internally?
The functions were already split, I did not split them. I looked at how things would work if this was done by a single function, but things started to get a bit messy. The thing is we don't always need to emit the header -- we only do it (in the main generator) if `DwarfDebug::useSegmentedStringOffsetsTable()` is true. So I would have to pass that flag into `emit` in addition to the `StringOffsetsStartSym` symbol. That wouldn't look good as the emit function already has two arguments with default values. It might be possible to clean this up, but this would need a larger refactor.


================
Comment at: unittests/DebugInfo/DWARF/DwarfGenerator.h:253
 
+  MCSymbol *StringOffsetsStartSym;
+
----------------
clayborg wrote:
> Does this need to be initialized to nullptr somewhere?
It's initialized in `Generator::init`. The only way we can get a valid `Generator` object is if that functions succeeds, so this variable will always be initialized (and not null).


Repository:
  rL LLVM

https://reviews.llvm.org/D49670