[PATCH] D39854: [DWARFv5] Support FORM_strp in .debug_line.dwo

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:213-217
+static void
+buildLineToUnitMap(DWARFContext::cu_iterator_range CUs,
+                   DWARFContext::tu_section_iterator_range TUSections,
+                   LineToUnitMap &LineToUnit) {
+  LineToUnit.clear();
----------------
dblaikie wrote:
> Might make more sense for this function to return the map by value, rather than take it by reference, clear it, and populate it?
Agreed.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:336
   uint8_t savedAddressByteSize = 0;
+  // FIXME: This seems sketchy.
+  for (const auto &CU : compile_units()) {
----------------
dblaikie wrote:
> Probably be more specific/descriptive.
> 
> This also appears to be a regression - before your patch this code did the right thing (used the corresponding CU's address byte size for each CU) for non-DWO CUs, though it was probably wrong for DWO CUs by using this saved thing. But with your patch it now does the same sketchy thing for DWO and non-DWO CUs?
I had tried to fix this properly (allowing a v5 line table to be dumped with no Unit) but tripped over a different issue, and decided to defer it to the next patch.  I think you are right that I could leave this set from the Unit whenever there is one, and pre-v5 basically there always has to be one.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:337-340
+  for (const auto &CU : compile_units()) {
+    savedAddressByteSize = CU->getAddressByteSize();
+    break;
+  }
----------------
dblaikie wrote:
> A loop that only runs once seems a bit odd, though I realize it's pretty compact.
Copied from the dwo case.  Technically it could run zero times.
But if I set the size from the Unit, inside the dump loop, this goes away anyhow.


https://reviews.llvm.org/D39854