[PATCH] D26469: Improve DWARF parsing and attribute retrieval speed by improving DWARF abbreviation declarations.

[Greg Clayton via llvm-commits]

clayborg added a comment.

I will make the changes.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h:36
+    // or the value size varies and must be decoded from the debug information
+    // in order to determine its size.
+    Optional<uint8_t> ByteSize;
----------------
aprantl wrote:
> It would be better to use doxygen comments `///` for all these definitions.
ok


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h:86
+    // The fixed byte size for fixed size forms.
+    uint16_t NumBytes;
+    // Number of DW_FORM_address forms in this abbrevation declaration.
----------------
aprantl wrote:
> perhaps write this as:
> ```
>     /// The fixed byte size for fixed size fo
>     uint16_t NumBytes = 0;
>     /// Number of DW_FORM_address forms in this abbrevation declaration.
>     uint8_t NumAddrs = 0;
>     /// Number of DW_FORM_ref_addr forms in this abbrevation declaration.
>     uint8_t NumRefAddrs = 0;
> ```
> and use the default constructor?
I added the ///. Can't use the default constructor as it won't initialize anything.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h:103
+  // Keep a map of attributes to attribute index for quick attribute lookups.
+  std::map<dwarf::Attribute, uint32_t> AttributeMap;
+  // If this abbreviation has a fixed byte size then \a FixedAttributeSize
----------------
aprantl wrote:
> Why are we using a std::map instead of an llvm::DenseMap here?
DenseMap doesn't compile.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:287
     const LoadedObjectInfo *L = nullptr);
+  DWARFContextInMemory(bool IsLittleEndian, uint8_t AddrSize,
+                       StringRef DebugAbbrev, StringRef DebugInfo,
----------------
aprantl wrote:
> The Doxygen comment should be in the header and not in the .cpp file. And the surrounding code is also violating this rule :-(
Are you asking for me to add a Doxygen comment? What is the suggestion here? There is not header doc in the .cpp file.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:54
+                   const DataExtractor &DebugInfoData,
+                   const uint32_t UEndOffset);
 
----------------
aprantl wrote:
> What's the point in making the by-value uint32_t const?
It was so people wouldn't change it accidentally, but I remember with the last review we fixed these so I can remove it.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:105
 
+  int64_t getAttributeValueAsSignedConstant(const DWARFUnit *U,
+                                            dwarf::Attribute Attr,
----------------
aprantl wrote:
> Doxygen comment?
I will skip this for now as I will be moving all of this into the llvm::DWARFDIE class in the next revision. I will add doxygen comments then.


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:58
+      auto FixedFormByteSize = DWARFFormValue::getFixedByteSize(F);
+      if (HasFixedByteSize) {
+        if (FixedFormByteSize.hasValue())
----------------
aprantl wrote:
> This loop is confusing with HasFixedByteSize being carried over into the next iteration...
It has to be this way. We are trying to see if the entire DWARFAbbreviationDeclaration has a fixed byte size. I can fix it by using an Optional around FSI maybe to make it clearer?


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:121
 
 uint32_t
+DWARFAbbreviationDeclaration::findAttributeIndex(dwarf::Attribute Attr) const {
----------------
aprantl wrote:
> This might be a good chance to replace this API with an Optional<uint32_t> in a follow-up commit?
No, this is actually a good idea, I will do it as part of this commit. I am not too fond of the use of the magic -1U that is used elsewhere.


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:161
+    const DWARFUnit *U) const {
+  assert(U);
+  size_t ByteSize = NumBytes;
----------------
aprantl wrote:
> why not pass in  a const DWARFUnit &U?
I thought of this but all DWARF APIs currently pass around pointers to DWARFUnits everywhere, so I would rather not change it and make it inconsistent. 


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:215
+  if (FixedDIESize.hasValue()) {
+    *OffsetPtr += FixedDIESize.getValue();
+    return true;
----------------
aprantl wrote:
> maybe `*FixedDIESize`?
Not sure that makes it clearer, but if that what most LLVM users do, I will change it.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:117
+    // 4 bytes in DWARF32, 8 in DWARF64
+    return 4; // FIXME: This DWARF parser currently only handles DWARF32.
+
----------------
aprantl wrote:
> Perhaps make this a variable?
I added an accessor in DWARFUnit:

```
  case DW_FORM_GNU_ref_alt:
  case DW_FORM_GNU_strp_alt:
  case DW_FORM_line_strp:
  case DW_FORM_sec_offset:
  case DW_FORM_strp_sup:
  case DW_FORM_ref_sup:
    // 4 bytes in DWARF 32, 8 bytes in DWARF 64.
    if (U)
      return U->getDwarfOffsetByteSize();
    return None;
```


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:26
+
+inline bool HostIsLittleEndian() {
+  union {
----------------
aprantl wrote:
> there is sys::IsBigEndianHost in Support/Endian.h
Nice, I'll use that.


Repository:
  rL LLVM

https://reviews.llvm.org/D26469