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

This looks much better!



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:57-59
+  uint32_t getNumAtoms();
+  uint16_t getAtomType(int i);
+  dwarf::Form getAtomForm(int i);
----------------
Can this be changed into just one 
ArrayRef<std::pair<AtomType, Form>> getAtomsDesc()
?

I find it cleaner that the piecewise interface.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h:60
+  dwarf::Form getAtomForm(int i);
+  uint32_t readAtoms(uint32_t &HashDataOffset);
   void dump(raw_ostream &OS) const;
----------------
It's not obvious what the function does, what the argument is and what it returns. Please add a doxygen comment.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:331-337
+    uint32_t NumAtoms = AppleNames.getNumAtoms();
+    if (NumAtoms == 0) {
+      OS << format("error: failed to read HashData at 0x%08x\n",
+                   HashDataOffset + 8);
+      HashDataOffset = dwarf::DW_INVALID_OFFSET;
+      ++NumAppleNamesErrors;
+    }
----------------
The error doesn't seem to reflect the test. Also, unless I'm missing something, you should be able to do this test once outside of the loop.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:346-349
+        if (HashDataOffset == dwarf::DW_INVALID_OFFSET) {
+          OS << format("error: failed to read HashData at 0x%08x\n",
+                       HashDataOffsetStart);
+          ++NumAppleNamesErrors;
----------------
I would return here. I think this is a fatal error. There's no point in continuing to try to decode the bitstream when you failed to read a piece of it.


================
Comment at: test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets_hashdataoffset.s:107
+  .byte 0                       ## End Of Macro List Mark
+  .section  __DWARF,__apple_names,regular,debug
+Lnames_begin:
----------------
Do you need all the other sections? Wouldn't it work just with the __apple_names section?


https://reviews.llvm.org/D34359