[PATCH] D34359: [DWARF] Verification of the validity of the hash data offset and hash data DIEs in the the .apple_names section.
Frederic Riss via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 23 13:46:16 PDT 2017
friss added a comment.
Here's a first round of comments.
================
Comment at: include/llvm/BinaryFormat/Dwarf.h:65-66
+// TODO: It has to match the offset type (uint32_t or uint64_t)
+#define DW_INVALID_OFFSET UINT32_MAX
+
----------------
Why is this not a con unit32_t? I feel like the comment should explain what it is too.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:279-280
+ bool containsUnitOffset(uint32_t Offset);
+
private:
----------------
This needs a comment, and I'm not fond of the name. It's not really descriptive.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:96-98
+ uint64_t getAsReference(uint32_t RefBase) const;
Optional<uint64_t> getAsUnsignedConstant() const;
+ uint64_t getAsUnsigned() const;
----------------
I can see the usefulness of the getAsReference helper, but it should really return an Optional<uint64_t>. I think you should just use getAsUnsignedConstant() instead of adding an unchecked version.
================
Comment at: include/llvm/DebugInfo/DWARF/DWARFUnit.h:301-310
+ bool containsOffset(uint32_t Offset) {
+ extractDIEsIfNeeded(false);
+ if (!DieArray.empty()) {
+ for (auto it = DieArray.begin(); it != DieArray.end(); ++it) {
+ if (it->getOffset() == Offset)
+ return true;
+ }
----------------
DIEs are sorted by offset, so this should use some kind of binary search. Also, I think the API is overly constrained. What about "DWARFDie getDIEAtOffset(nuint32_t offset)" ? It would return an invalid DwarfDIE if that offset is not valid.
================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:359-360
case DW_FORM_data1:
+ Value.uval = Data.getU8(OffsetPtr);
+ break;
case DW_FORM_ref1:
----------------
All those added cases don't change the behavior, just leave this file as it is.
================
Comment at: lib/Support/DataExtractor.cpp:131-137
+const char *DataExtractor::peekCStr(uint32_t Offset) const {
+ StringRef::size_type pos = Data.find('\0', Offset);
+ if (pos != StringRef::npos) {
+ return Data.data() + Offset;
+ }
+ return nullptr;
+}
----------------
This could be implemented in terms of getCStr()
https://reviews.llvm.org/D34359
More information about the llvm-commits
mailing list