[PATCH] D26469: Improve DWARF parsing and attribute retrieval speed by improving DWARF abbreviation declarations.
Adrian Prantl via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 9 14:51:27 PST 2016
aprantl added a comment.
This patch is really too long for a thorough review. I made a first pass and added a couple of low-level comments, but it would benefit a lot if you could split this up into smaller units.
================
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;
----------------
It would be better to use doxygen comments `///` for all these definitions.
================
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.
----------------
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?
================
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
----------------
Why are we using a std::map instead of an llvm::DenseMap here?
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:287
const LoadedObjectInfo *L = nullptr);
+ DWARFContextInMemory(bool IsLittleEndian, uint8_t AddrSize,
+ StringRef DebugAbbrev, StringRef DebugInfo,
----------------
The Doxygen comment should be in the header and not in the .cpp file. And the surrounding code is also violating this rule :-(
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:51
bool extractFast(const DWARFUnit *U, uint32_t *OffsetPtr);
+ /// High performance DWARFDebugInfoEntry should use this call
+ bool extractFast(const DWARFUnit *U, uint32_t *OffsetPtr,
----------------
trailing `.`
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:54
+ const DataExtractor &DebugInfoData,
+ const uint32_t UEndOffset);
----------------
What's the point in making the by-value uint32_t const?
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:105
+ int64_t getAttributeValueAsSignedConstant(const DWARFUnit *U,
+ dwarf::Attribute Attr,
----------------
Doxygen comment?
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:88
+ // value will be returned. If the form can vary in size depending on the
+ // DWARFUnit (DWARF version, address byte size, or DWARF32/DWARFF64) and the
+ // DWARFUnit is valid, then an Optional with a valid value is returned. If the
----------------
DWARF 64
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:92
+ // numbers or blocks) or the size depends on a DWARFUnit and the DWARFUnit is
+ // NULL, then an Optional with no value will be returned.
+ static Optional<uint8_t> getFixedByteSize(dwarf::Form form,
----------------
/// NULL, then None will be returned.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:97
+ // using a variable length storage format (ULEB/SLEB numbers or blocks) then
+ // an Optional with no value will be returned.
+ static Optional<uint8_t> getFixedByteSize(dwarf::Form Form, uint16_t Version,
----------------
ditto
================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:58
+ auto FixedFormByteSize = DWARFFormValue::getFixedByteSize(F);
+ if (HasFixedByteSize) {
+ if (FixedFormByteSize.hasValue())
----------------
This loop is confusing with HasFixedByteSize being carried over into the next iteration...
================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:121
uint32_t
+DWARFAbbreviationDeclaration::findAttributeIndex(dwarf::Attribute Attr) const {
----------------
This might be a good chance to replace this API with an Optional<uint32_t> in a follow-up commit?
================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:136
+
+ DataExtractor DebugInfoData = U->getDebugInfoExtractor();
+
----------------
auto, since the type is clear from the context?
================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:139
+ // Add the byte size of ULEB that for the abbrev Code so we can start skipping
+ // the attribute data
+ uint32_t Offset = DIEOffset + CodeByteSize;
----------------
trailing .
================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:144
+ if (MatchAttrIndex == AttrIndex) {
+ // We have arrived at the attribute to extract, extract if from Offset
+ FormValue.setForm(Spec.Form);
----------------
trailing .
================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:161
+ const DWARFUnit *U) const {
+ assert(U);
+ size_t ByteSize = NumBytes;
----------------
why not pass in a const DWARFUnit &U?
================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:170
+
+// Get the fixed byte size of this Form if the Form is fixed in byte size
+Optional<uint8_t> DWARFAbbreviationDeclaration::AttributeSpec::getByteSize(
----------------
trailing .
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:215
+ if (FixedDIESize.hasValue()) {
+ *OffsetPtr += FixedDIESize.getValue();
+ return true;
----------------
maybe `*FixedDIESize`?
================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:218
+ }
// Skip all data in the .debug_info for the attributes
for (const auto &AttrSpec : AbbrevDecl->attributes()) {
----------------
.
================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:73
+
+ case DW_FORM_block: // ULEB128 length L followed by L bytes
+ case DW_FORM_block1: // 1 byte length L followed by L bytes
----------------
trailing .
================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:94
+ return U->getRefAddrByteSize();
+ break;
+
----------------
maybe use returns everywhere?
================
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.
+
----------------
Perhaps make this a variable?
================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:126
+ return 0;
+ break;
+
----------------
redundant break.
================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:143
+
+Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form form,
+ uint16_t Version,
----------------
Same comments as in previous function.
================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:26
+
+inline bool HostIsLittleEndian() {
+ union {
----------------
there is sys::IsBigEndianHost in Support/Endian.h
Repository:
rL LLVM
https://reviews.llvm.org/D26469
More information about the llvm-commits
mailing list