[PATCH] D147203: [dsymutil] Add a new verification mode

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:247
     messageHandler;
+typedef std::function<void(const DWARFFile &File)> verificationHandler;
 typedef std::function<ErrorOr<DWARFFile &>(StringRef ContainerName,
----------------
onInputVerificationFailure looks to be a bit more precise name.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:351
+  /// errors.
+  void setVerificationHandler(verificationHandler Handler) {
+    Options.VerificationHandler = Handler;
----------------



================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:825
+  /// Whether any errors were found during verification.
+  bool HasVerificationErrors = false;
+
----------------
It looks like this variable is never set.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2897
   if (!File.Dwarf->verify(os, DumpOpts.noImplicitRecursion())) {
+    Options.VerificationHandler(File);
     reportWarning("input verification failed", File);
----------------
if VerificationHandler is not set then we need to have a check for null here, or provide already existing default implementation.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2898
+    Options.VerificationHandler(File);
     reportWarning("input verification failed", File);
   }
----------------
as we have a call to the handler here it is probably better to move warning reporting code into this handler.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:276
+        std::unique_ptr<DWARFContext>(DWARFContext::create(
+            *ErrorOrObj, DWARFContext::ProcessDebugRelocations::Process)));
     AddressMapForLinking.push_back(
----------------
default state for the second parameter is DWARFContext::ProcessDebugRelocations::Process. Is it necessary to explicitly specify it here?


================
Comment at: llvm/tools/dsymutil/dsymutil.cpp:703
       WithColor::warning()
           << "skipping output verification because --no-output was passed\n";
     }
----------------
probably move this check and warning  into the verifyOutput() function? to have a single place where Options.LinkOpts.NoOutput is checked.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147203/new/

https://reviews.llvm.org/D147203