[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]

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