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

[Paul Robinson via Phabricator via llvm-commits]

probinson 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.
 
----------------
dblaikie wrote:
> 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) 
You have touched on a couple of different things here.

Using a value member instead of shared_ptr seems fine, it would make the diff a whole lot longer but most of the extra changes would be replacing `U->foo` with `U.foo`.

Yes we could build a helper class that has value members with all the right info.  But then we are paying some nonzero cost to extract/compute those values (e.g., size of a ref-addr) when constructing the class, but most of the time we won't actually need all that.  And remember that with all the new indirection in v5, we are talking about handles on quite a few different sections that we'd need to copy over into the new helper, on the off chance that we'd need them.  Or make the constructor a lot more complicated, smart enough to work out which pieces it needs based on the Form... 

If we really wanted to get radical, DWARFFormValue should be handed the DWARFContext rather than the DWARFUnit, along with the few bits of unit-specific info (32/64 format, version, address size).  Then we query the context as needed, instead of the unit.  That minimizes copying while retaining the unit/line-table flexibility.

I'm open to suggestions.



================
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.
----------------
dblaikie wrote:
> 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.
Seems to me I did get warnings or something when not all parameters were documents.
If you want to propose a "nothing to see here" case in the coding standard, knock yourself out.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:143-144
                      std::vector<DWARFDebugLine::FileNameEntry> &FileNames) {
+  dwarf::DwarfFormat Format =
+      IsDwarf64 ? dwarf::DwarfFormat::DWARF64 : dwarf::DwarfFormat::DWARF32;
+
----------------
dblaikie wrote:
> I know DWARF64 support is starting to be considered in a few places - but is this tested?
Huh.  No, I missed that, I will add a test.
IMHO it would not be the end of the world for the dumpers to be able to handle DWARF64.  The day will likely come when somebody really can't work around the lack.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:135
+      return U->getStringOffsetSectionItem(Index, Result);
+    llvm_unreachable("Invalid use of DWARFUnitProxy");
+  }
----------------
dblaikie wrote:
> avoid branch-to-unreachable, instead "assert(U)" at the start of the function (similarly for other cases above/in this patch)
Hmm so a release build would get a seg fault instead of a more intelligent error?  I can do what you said but an actual diagnostic instead of a seg fault seems pretty cheap.


https://reviews.llvm.org/D33155