[PATCH] D34177: [DWARF] Partial verification for .apple_names accelerator table in llvm-dwarfdump output.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFVerifier.h:81
+      : OS(S), DCtx(D), NumDebugInfoErrors(0), NumDebugLineErrors(0),
+        NumAppleNamesErrors(0) {}
   /// Verify the information in the .debug_info section.
----------------
Maybe use a non-static data member initializer here (& move the other members over to using that syntax too (in a separate pre-commit, doesn't need review))


================
Comment at: llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp:290-293
+  if (!AppleNames.extract()) {
+    OS << "error: cannot extract .apple_names accelerator table\n";
+    return false;
+  }
----------------
Is this error produced when the section is not present? Is that the right behavior/does this need an error in that case?


================
Comment at: llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp:310
+  }
+  return NumAppleNamesErrors == 0;
+}
----------------
Probably use a boolean flag if that's the only thing that matters? ("HasErrors"?)


================
Comment at: llvm/trunk/test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets.s:1-10
+# RUN: llvm-mc %s -filetype obj -triple x86_64-apple-darwin -o - \
+# RUN: | not llvm-dwarfdump -verify - \
+# RUN: | FileCheck %s
+
+# CHECK: Verifying .apple_names...
+# CHECK-NEXT: error:
+
----------------
Provide some detail about what this test case was built from (source code, command line, etc)?


================
Comment at: llvm/trunk/test/tools/llvm-dwarfdump/X86/apple_names_verify_buckets.s:5-6
+
+# CHECK: Verifying .apple_names...
+# CHECK-NEXT: error:
+
----------------
This seems pretty vague/non-specific for a test case. The error message should be tested.

Also, add a test case with an object with no apple names, to demonstrate what happens there. (be it an error, or silence, depending on the resolution of my other comment).


Repository:
  rL LLVM

https://reviews.llvm.org/D34177