[PATCH] D33155: [DWARFv5] Support FORM_strp in the line table header

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:57
   ValueType Value;              // Contains all data for the form.
-  const DWARFUnit *U = nullptr; // Remember the DWARFUnit at extract time.
+  std::shared_ptr<const DWARFUnitProxy> U; // Info useful during extraction.
 
----------------
My best guess is that sizeof(DWARFFormValue) isn't so important this needs to be indirected. It looks like most/all DWARFFormValues are transient, when some code is visiting the current attribute or form - I don't see code manifesting in a container "all forms/attributes for a DIE" for example (nor all attributes/forms in an entire file, etc).

So maybe having this as a value member, rather than a shared_ptr would reduce complexity?

Also - while I understand it's convenient to wrap DWARFUnit in the proxy so existing code doesn't need to be touched, I'm not sure it's that much of an improvement in readability - but I could be wrong - compared to having a few members and using those directly? (indeed most code still wouldn't change because it could use the DWARFUnit *U that was there before/would remain - and a few places would use the few extra new state/variables) 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:77-85
+  /// Extracts a value in \p Data at offset from \p OffsetPtr.
   ///
   /// The passed DWARFUnit is allowed to be nullptr, in which
   /// case no relocation processing will be performed and some
   /// kind of forms that depend on Unit information are disallowed.
+  /// \param Data The DataExtractor to use.
+  /// \param OffsetPtr The offset within DataExtractor where the data starts.
----------------
Feel free to commit changes like this separately, without pre-commit review. Keeps patches small/targeted/easier to review/revert, etc.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:82
   /// kind of forms that depend on Unit information are disallowed.
+  /// \param Data The DataExtractor to use.
+  /// \param OffsetPtr The offset within DataExtractor where the data starts.
----------------
Does the LLVM style guide say to document all parameters? (I guess the clang doxy warnings flag things like this if they're not document?)

It'd be nice if there were a tidier way to just say "there's nothing to add here" - because I don't think "The DataExtractor to use" really adds anything compared to what the generated documentation or function signature already has.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:193-196
+    DWARFUnit *U = nullptr;
+    if (!CUs.empty())
+      U = &**CUs.begin();
+    while (LineTable.Prologue.parse(lineData, &stmtOffset, U)) {
----------------
Guessing this probably isn't right for DWP files? Or potentially for a DWO containing multiple CUs that might have different CU properties that are relevant to the line table?

Though that's perhaps an interesting question - since a DWO line table is just for type units, the CU won't have a reference to any line.dwo section, and multiple DWO TUs might refer to the same line table, I think? (I think LLVM produces only one, but I forget - it's been awhile since I implemented taht) so you'd want to pick one of the DWO TUs that references this line table to use here?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:143-144
                      std::vector<DWARFDebugLine::FileNameEntry> &FileNames) {
+  dwarf::DwarfFormat Format =
+      IsDwarf64 ? dwarf::DwarfFormat::DWARF64 : dwarf::DwarfFormat::DWARF32;
+
----------------
I know DWARF64 support is starting to be considered in a few places - but is this tested?


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:135
+      return U->getStringOffsetSectionItem(Index, Result);
+    llvm_unreachable("Invalid use of DWARFUnitProxy");
+  }
----------------
avoid branch-to-unreachable, instead "assert(U)" at the start of the function (similarly for other cases above/in this patch)


https://reviews.llvm.org/D33155