[dsymutil] Add a new automatic verification mode

This patch a new verification mode called "auto" that runs the DWARF
verifier on the input and if the input is valid, also runs the DWARF
verifier on the output. The goal is to catch cases where dsymutil turns
valid DWARF into invalid DWARF. This patch makes this verification mode
the default when assertions or expensive checks are enabled.

Differential revision: https://reviews.llvm.org/D147203

diff --git a/llvm/include/llvm/DWARFLinker/DWARFLinker.h b/llvm/include/llvm/DWARFLinker/DWARFLinker.h
index f58bb3f..06a0696 100644 (file)
--- a/llvm/include/llvm/DWARFLinker/DWARFLinker.h
+++ b/llvm/include/llvm/DWARFLinker/DWARFLinker.h
@@ -244,6 +244,7 @@ public:
 typedef std::function<void(const Twine &Warning, StringRef Context,
                            const DWARFDie *DIE)>
     messageHandler;
+typedef std::function<void(const DWARFFile &File)> inputVerificationHandler;
 typedef std::function<ErrorOr<DWARFFile &>(StringRef ContainerName,
                                            StringRef Path)>
     objFileLoader;
@@ -345,6 +346,12 @@ public:
     Options.ErrorHandler = Handler;
   }
 
+  /// Set verification handler which would be used to report verification
+  /// errors.
+  void setInputVerificationHandler(inputVerificationHandler Handler) {
+    Options.InputVerificationHandler = Handler;
+  }
+
   /// Set map for Swift interfaces.
   void setSwiftInterfacesMap(swiftInterfacesMap *Map) {
     Options.ParseableSwiftInterfaces = Map;
@@ -424,7 +431,7 @@ private:
   };
 
   /// Verify the given DWARF file.
-  bool verify(const DWARFFile &File);
+  void verifyInput(const DWARFFile &File);
 
   /// returns true if we need to translate strings.
   bool needToTranslateStrings() { return StringsTranslator != nullptr; }
@@ -856,6 +863,9 @@ private:
     // error handler
     messageHandler ErrorHandler = nullptr;
 
+    // input verification handler
+    inputVerificationHandler InputVerificationHandler = nullptr;
+
     /// A list of all .swiftinterface files referenced by the debug
     /// info, mapping Module name to path on disk. The entries need to
     /// be uniqued and sorted and there are only few entries expected

diff --git a/llvm/lib/DWARFLinker/DWARFLinker.cpp b/llvm/lib/DWARFLinker/DWARFLinker.cpp
index 1a61506..94a711a 100644 (file)
--- a/llvm/lib/DWARFLinker/DWARFLinker.cpp
+++ b/llvm/lib/DWARFLinker/DWARFLinker.cpp
@@ -2577,7 +2577,7 @@ Error DWARFLinker::link() {
       continue;
 
     if (Options.VerifyInputDWARF)
-      verify(OptContext.File);
+      verifyInput(OptContext.File);
 
     // Look for relocations that correspond to address map entries.
 
@@ -2888,16 +2888,15 @@ Error DWARFLinker::cloneModuleUnit(LinkContext &Context, RefModuleUnit &Unit,
   return Error::success();
 }
 
-bool DWARFLinker::verify(const DWARFFile &File) {
+void DWARFLinker::verifyInput(const DWARFFile &File) {
   assert(File.Dwarf);
 
   raw_ostream &os = Options.Verbose ? errs() : nulls();
   DIDumpOptions DumpOpts;
   if (!File.Dwarf->verify(os, DumpOpts.noImplicitRecursion())) {
-    reportWarning("input verification failed", File);
-    return false;
+    if (Options.InputVerificationHandler)
+      Options.InputVerificationHandler(File);
   }
-  return true;
 }
 
 } // namespace llvm

diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
index c5e1bf1..cf3c87b 100644 (file)
--- a/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
+++ b/llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
@@ -589,6 +589,10 @@ bool DwarfLinkerForBinary::link(const DebugMap &Map) {
       [&](const Twine &Error, StringRef Context, const DWARFDie *) {
         error(Error, Context);
       });
+  GeneralLinker.setInputVerificationHandler([&](const DWARFFile &File) {
+    reportWarning("input verification failed", File.FileName);
+    HasVerificationErrors = true;
+  });
   objFileLoader Loader = [&DebugMap, &RL,
                           this](StringRef ContainerName,
                                 StringRef Path) -> ErrorOr<DWARFFile &> {
@@ -654,7 +658,7 @@ bool DwarfLinkerForBinary::link(const DebugMap &Map) {
   if (!Options.NoOutput && !ReflectionSectionsPresentInBinary) {
     auto SectionToOffsetInDwarf =
         calculateStartOfStrippableReflectionSections(Map);
-    for (const auto &Obj : Map.objects()) 
+    for (const auto &Obj : Map.objects())
       copySwiftReflectionMetadata(Obj.get(), Streamer.get(),
                                   SectionToOffsetInDwarf, RelocationsToApply);
   }
@@ -1070,11 +1074,5 @@ bool DwarfLinkerForBinary::AddressManager::applyValidRelocs(
   return Relocs.size() > 0;
 }
 
-bool linkDwarf(raw_fd_ostream &OutFile, BinaryHolder &BinHolder,
-               const DebugMap &DM, LinkOptions Options) {
-  DwarfLinkerForBinary Linker(OutFile, BinHolder, std::move(Options));
-  return Linker.link(DM);
-}
-
 } // namespace dsymutil
 } // namespace llvm

diff --git a/llvm/tools/dsymutil/DwarfLinkerForBinary.h b/llvm/tools/dsymutil/DwarfLinkerForBinary.h
index 1f7422f..7fd15a1 100644 (file)
--- a/llvm/tools/dsymutil/DwarfLinkerForBinary.h
+++ b/llvm/tools/dsymutil/DwarfLinkerForBinary.h
@@ -44,6 +44,10 @@ public:
   void reportWarning(const Twine &Warning, StringRef Context,
                      const DWARFDie *DIE = nullptr) const;
 
+  /// Returns true if input verification is enabled and verification errors were
+  /// found.
+  bool InputVerificationFailed() const { return HasVerificationErrors; }
+
   /// Flags passed to DwarfLinker::lookForDIEsToKeep
   enum TraversalFlags {
     TF_Keep = 1 << 0,            ///< Mark the traversed DIEs as kept.
@@ -230,6 +234,7 @@ private:
 
   bool ModuleCacheHintDisplayed = false;
   bool ArchiveHintDisplayed = false;
+  bool HasVerificationErrors = false;
 };
 
 } // end namespace dsymutil

diff --git a/llvm/tools/dsymutil/Options.td b/llvm/tools/dsymutil/Options.td
index 8af6dbe..abcdc91 100644 (file)
--- a/llvm/tools/dsymutil/Options.td
+++ b/llvm/tools/dsymutil/Options.td
@@ -43,7 +43,9 @@ def verify: F<"verify">,
 
 def verify_dwarf: Separate<["--", "-"], "verify-dwarf">,
   MetaVarName<"<verification mode>">,
-  HelpText<"Run the DWARF verifier on the input and/or output. Valid options are 'input', 'output', 'all' or 'none'.">,
+  HelpText<"Run the DWARF verifier on the input and/or output. "
+           "Valid options are 'none, 'input', 'output', 'all' or 'auto' "
+           "which runs the output verifier only if input verification passed.">,
   Group<grp_general>;
 def: Joined<["--", "-"], "verify-dwarf=">, Alias<verify_dwarf>;
 

diff --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp
index d868459..e6e5d7b 100644 (file)
--- a/llvm/tools/dsymutil/dsymutil.cpp
+++ b/llvm/tools/dsymutil/dsymutil.cpp
@@ -14,6 +14,7 @@
 #include "BinaryHolder.h"
 #include "CFBundle.h"
 #include "DebugMap.h"
+#include "DwarfLinkerForBinary.h"
 #include "LinkUtils.h"
 #include "MachOUtils.h"
 #include "Reproducer.h"
@@ -94,7 +95,14 @@ enum class DWARFVerify : uint8_t {
   None = 0,
   Input = 1 << 0,
   Output = 1 << 1,
+  OutputOnValidInput = 1 << 2,
   All = Input | Output,
+  Auto = Input | OutputOnValidInput,
+#if !defined(NDEBUG) || defined(EXPENSIVE_CHECKS)
+  Default = Auto
+#else
+  Default = None
+#endif
 };
 
 inline bool flagIsSet(DWARFVerify Flags, DWARFVerify SingleFlag) {
@@ -115,7 +123,7 @@ struct DsymutilOptions {
   std::vector<std::string> Archs;
   std::vector<std::string> InputFiles;
   unsigned NumThreads;
-  DWARFVerify Verify = DWARFVerify::None;
+  DWARFVerify Verify = DWARFVerify::Default;
   ReproducerMode ReproMode = ReproducerMode::GenerateOnCrash;
   dsymutil::LinkOptions LinkOpts;
 };
@@ -266,14 +274,16 @@ static Expected<DWARFVerify> getVerifyKind(opt::InputArgList &Args) {
       return DWARFVerify::Output;
     if (S == "all")
       return DWARFVerify::All;
+    if (S == "auto")
+      return DWARFVerify::Auto;
     if (S == "none")
       return DWARFVerify::None;
-    return make_error<StringError>(
-        "invalid verify type specified: '" + S +
-            "'. Supported values are 'input', 'output', 'all' and 'none'.",
-        inconvertibleErrorCode());
+    return make_error<StringError>("invalid verify type specified: '" + S +
+                                       "'. Supported values are 'none', "
+                                       "'input', 'output', 'all' and 'auto'.",
+                                   inconvertibleErrorCode());
   }
-  return DWARFVerify::None;
+  return DWARFVerify::Default;
 }
 
 /// Parses the command line options into the LinkOptions struct and performs
@@ -461,10 +471,18 @@ static Error createBundleDir(StringRef BundleBase) {
   return Error::success();
 }
 
-static bool verifyOutput(StringRef OutputFile, StringRef Arch, bool Verbose) {
+static bool verifyOutput(StringRef OutputFile, StringRef Arch,
+                         DsymutilOptions Options) {
+
   if (OutputFile == "-") {
     WithColor::warning() << "verification skipped for " << Arch
-                         << "because writing to stdout.\n";
+                         << " because writing to stdout.\n";
+    return true;
+  }
+
+  if (Options.LinkOpts.NoOutput) {
+    WithColor::warning() << "verification skipped for " << Arch
+                         << " because --no-output was passed.\n";
     return true;
   }
 
@@ -476,9 +494,14 @@ static bool verifyOutput(StringRef OutputFile, StringRef Arch, bool Verbose) {
 
   Binary &Binary = *BinOrErr.get().getBinary();
   if (auto *Obj = dyn_cast<MachOObjectFile>(&Binary)) {
-    raw_ostream &os = Verbose ? errs() : nulls();
-    os << "Verifying DWARF for architecture: " << Arch << "\n";
     std::unique_ptr<DWARFContext> DICtx = DWARFContext::create(*Obj);
+    if (DICtx->getMaxVersion() >= 5) {
+      WithColor::warning() << "verification skipped for " << Arch
+                           << " because DWARFv5 is not fully supported yet.\n";
+      return true;
+    }
+    raw_ostream &os = Options.LinkOpts.Verbose ? errs() : nulls();
+    os << "Verifying DWARF for architecture: " << Arch << "\n";
     DIDumpOptions DumpOpts;
     bool success = DICtx->verify(os, DumpOpts.noImplicitRecursion());
     if (!success)
@@ -687,12 +710,6 @@ int main(int argc, char **argv) {
     const bool NeedsTempFiles =
         !Options.DumpDebugMap && (Options.OutputFile != "-") &&
         (DebugMapPtrsOrErr->size() != 1 || Options.LinkOpts.Update);
-    bool VerifyOutput = flagIsSet(Options.Verify, DWARFVerify::Output);
-    if (VerifyOutput && Options.LinkOpts.NoOutput) {
-      WithColor::warning()
-          << "skipping output verification because --no-output was passed\n";
-      VerifyOutput = false;
-    }
 
     // Set up a crash recovery context.
     CrashRecoveryContext::Enable();
@@ -751,14 +768,15 @@ int main(int argc, char **argv) {
         }
 
         auto LinkLambda = [&,
-                           OutputFile](std::shared_ptr<raw_fd_ostream> Stream,
-                                       LinkOptions Options) {
-          AllOK.fetch_and(
-              linkDwarf(*Stream, BinHolder, *Map, std::move(Options)));
+                           OutputFile](std::shared_ptr<raw_fd_ostream> Stream) {
+          DwarfLinkerForBinary Linker(*Stream, BinHolder, Options.LinkOpts);
+          AllOK.fetch_and(Linker.link(*Map));
           Stream->flush();
-          if (VerifyOutput) {
+          if (flagIsSet(Options.Verify, DWARFVerify::Output) ||
+              (flagIsSet(Options.Verify, DWARFVerify::OutputOnValidInput) &&
+               !Linker.InputVerificationFailed())) {
             AllOK.fetch_and(verifyOutput(
-                OutputFile, Map->getTriple().getArchName(), Options.Verbose));
+                OutputFile, Map->getTriple().getArchName(), Options));
           }
         };
 
@@ -766,9 +784,9 @@ int main(int argc, char **argv) {
         // out the (significantly smaller) stack when using threads. We don't
         // want this limitation when we only have a single thread.
         if (S.ThreadsRequested == 1)
-          LinkLambda(OS, Options.LinkOpts);
+          LinkLambda(OS);
         else
-          Threads.async(LinkLambda, OS, Options.LinkOpts);
+          Threads.async(LinkLambda, OS);
       }
 
       Threads.wait();

diff --git a/llvm/tools/dsymutil/dsymutil.h b/llvm/tools/dsymutil/dsymutil.h
index f88f57b..e679928 100644 (file)
--- a/llvm/tools/dsymutil/dsymutil.h
+++ b/llvm/tools/dsymutil/dsymutil.h
@@ -45,11 +45,6 @@ bool dumpStab(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
               StringRef InputFile, ArrayRef<std::string> Archs,
               StringRef PrependPath = "");
 
-/// Link the Dwarf debug info as directed by the passed DebugMap \p DM into a
-/// DwarfFile named \p OutputFilename. \returns false if the link failed.
-bool linkDwarf(raw_fd_ostream &OutFile, BinaryHolder &BinHolder,
-               const DebugMap &DM, LinkOptions Options);
-
 } // end namespace dsymutil
 } // end namespace llvm