From: Raphael Isemann Date: Tue, 1 Sep 2020 10:21:44 +0000 (+0200) Subject: Revert "[lldb] Add reproducer verifier" X-Git-Tag: llvmorg-13-init~13218 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=7c80f2da812e45bbdfa3c8f9ab24440f8ef3362a;p=platform%2Fupstream%2Fllvm.git Revert "[lldb] Add reproducer verifier" This reverts commit 297f69afac58fc9dc13897857a5e70131c5adc85. It broke the Fedora 33 x86-64 bot. See the review for more info. --- diff --git a/lldb/include/lldb/API/SBReproducer.h b/lldb/include/lldb/API/SBReproducer.h index 5578162..78044e9 100644 --- a/lldb/include/lldb/API/SBReproducer.h +++ b/lldb/include/lldb/API/SBReproducer.h @@ -11,32 +11,8 @@ #include "lldb/API/SBDefines.h" -namespace lldb_private { -namespace repro { -struct ReplayOptions; -} -} // namespace lldb_private - namespace lldb { -class LLDB_API SBReplayOptions { -public: - SBReplayOptions(); - SBReplayOptions(const SBReplayOptions &rhs); - ~SBReplayOptions(); - - SBReplayOptions &operator=(const SBReplayOptions &rhs); - - void SetVerify(bool verify); - bool GetVerify() const; - - void SetCheckVersion(bool check); - bool GetCheckVersion() const; - -private: - std::unique_ptr m_opaque_up; -}; - /// The SBReproducer class is special because it bootstraps the capture and /// replay of SB API calls. As a result we cannot rely on any other SB objects /// in the interface or implementation of this class. @@ -46,7 +22,6 @@ public: static const char *Capture(const char *path); static const char *Replay(const char *path); static const char *Replay(const char *path, bool skip_version_check); - static const char *Replay(const char *path, const SBReplayOptions &options); static const char *PassiveReplay(const char *path); static const char *GetPath(); static bool SetAutoGenerate(bool b); diff --git a/lldb/include/lldb/Utility/Reproducer.h b/lldb/include/lldb/Utility/Reproducer.h index 7e55914..d6cde44 100644 --- a/lldb/include/lldb/Utility/Reproducer.h +++ b/lldb/include/lldb/Utility/Reproducer.h @@ -227,22 +227,6 @@ private: mutable std::mutex m_mutex; }; -class Verifier { -public: - Verifier(Loader *loader) : m_loader(loader) {} - void Verify(llvm::function_ref error_callback, - llvm::function_ref warning_callback, - llvm::function_ref note_callback) const; - -private: - Loader *m_loader; -}; - -struct ReplayOptions { - bool verify = true; - bool check_version = true; -}; - } // namespace repro } // namespace lldb_private diff --git a/lldb/source/API/SBReproducer.cpp b/lldb/source/API/SBReproducer.cpp index 233e555..7d08a88 100644 --- a/lldb/source/API/SBReproducer.cpp +++ b/lldb/source/API/SBReproducer.cpp @@ -30,33 +30,6 @@ using namespace lldb; using namespace lldb_private; using namespace lldb_private::repro; -SBReplayOptions::SBReplayOptions() - : m_opaque_up(std::make_unique()){}; - -SBReplayOptions::SBReplayOptions(const SBReplayOptions &rhs) - : m_opaque_up(std::make_unique(*rhs.m_opaque_up)) {} - -SBReplayOptions::~SBReplayOptions() = default; - -SBReplayOptions &SBReplayOptions::operator=(const SBReplayOptions &rhs) { - if (this == &rhs) - return *this; - *m_opaque_up = *rhs.m_opaque_up; - return *this; -} - -void SBReplayOptions::SetVerify(bool verify) { m_opaque_up->verify = verify; } - -bool SBReplayOptions::GetVerify() const { return m_opaque_up->verify; } - -void SBReplayOptions::SetCheckVersion(bool check) { - m_opaque_up->check_version = check; -} - -bool SBReplayOptions::GetCheckVersion() const { - return m_opaque_up->check_version; -} - SBRegistry::SBRegistry() { Registry &R = *this; @@ -190,18 +163,10 @@ const char *SBReproducer::PassiveReplay(const char *path) { } const char *SBReproducer::Replay(const char *path) { - SBReplayOptions options; - return SBReproducer::Replay(path, options); + return SBReproducer::Replay(path, false); } const char *SBReproducer::Replay(const char *path, bool skip_version_check) { - SBReplayOptions options; - options.SetCheckVersion(!skip_version_check); - return SBReproducer::Replay(path, options); -} - -const char *SBReproducer::Replay(const char *path, - const SBReplayOptions &options) { static std::string error; if (auto e = Reproducer::Initialize(ReproducerMode::Replay, FileSpec(path))) { error = llvm::toString(std::move(e)); @@ -214,7 +179,7 @@ const char *SBReproducer::Replay(const char *path, return error.c_str(); } - if (options.GetCheckVersion()) { + if (!skip_version_check) { llvm::Expected version = loader->LoadBuffer(); if (!version) { error = llvm::toString(version.takeError()); @@ -230,30 +195,6 @@ const char *SBReproducer::Replay(const char *path, } } - if (options.GetVerify()) { - bool verification_failed = false; - llvm::raw_string_ostream os(error); - auto error_callback = [&](llvm::StringRef error) { - verification_failed = true; - os << "\nerror: " << error; - }; - - auto warning_callback = [&](llvm::StringRef warning) { - verification_failed = true; - os << "\nwarning: " << warning; - }; - - auto note_callback = [&](llvm::StringRef warning) {}; - - Verifier verifier(loader); - verifier.Verify(error_callback, warning_callback, note_callback); - - if (verification_failed) { - os.flush(); - return error.c_str(); - } - } - FileSpec file = loader->GetFile(); if (!file) { error = "unable to get replay data from reproducer."; diff --git a/lldb/source/Commands/CommandObjectReproducer.cpp b/lldb/source/Commands/CommandObjectReproducer.cpp index ae48940..da2d9ca 100644 --- a/lldb/source/Commands/CommandObjectReproducer.cpp +++ b/lldb/source/Commands/CommandObjectReproducer.cpp @@ -116,9 +116,6 @@ static constexpr OptionEnumValues ReproducerSignalType() { #define LLDB_OPTIONS_reproducer_xcrash #include "CommandOptions.inc" -#define LLDB_OPTIONS_reproducer_verify -#include "CommandOptions.inc" - template llvm::Expected static ReadFromYAML(StringRef filename) { auto error_or_file = MemoryBuffer::getFile(filename); @@ -137,38 +134,6 @@ llvm::Expected static ReadFromYAML(StringRef filename) { return t; } -static void SetError(CommandReturnObject &result, Error err) { - result.GetErrorStream().Printf("error: %s\n", - toString(std::move(err)).c_str()); - result.SetStatus(eReturnStatusFailed); -} - -/// Create a loader from the given path if specified. Otherwise use the current -/// loader used for replay. -static Loader * -GetLoaderFromPathOrCurrent(llvm::Optional &loader_storage, - CommandReturnObject &result, - FileSpec reproducer_path) { - if (reproducer_path) { - loader_storage.emplace(reproducer_path); - Loader *loader = &(*loader_storage); - if (Error err = loader->LoadIndex()) { - // This is a hard error and will set the result to eReturnStatusFailed. - SetError(result, std::move(err)); - return nullptr; - } - return loader; - } - - if (Loader *loader = Reproducer::Instance().GetLoader()) - return loader; - - // This is a soft error because this is expected to fail during capture. - result.SetError("Not specifying a reproducer is only support during replay."); - result.SetStatus(eReturnStatusSuccessFinishNoResult); - return nullptr; -} - class CommandObjectReproducerGenerate : public CommandObjectParsed { public: CommandObjectReproducerGenerate(CommandInterpreter &interpreter) @@ -347,6 +312,12 @@ protected: } }; +static void SetError(CommandReturnObject &result, Error err) { + result.GetErrorStream().Printf("error: %s\n", + toString(std::move(err)).c_str()); + result.SetStatus(eReturnStatusFailed); +} + class CommandObjectReproducerDump : public CommandObjectParsed { public: CommandObjectReproducerDump(CommandInterpreter &interpreter) @@ -411,11 +382,29 @@ protected: return false; } + // If no reproducer path is specified, use the loader currently used for + // replay. Otherwise create a new loader just for dumping. llvm::Optional loader_storage; - Loader *loader = - GetLoaderFromPathOrCurrent(loader_storage, result, m_options.file); - if (!loader) - return false; + Loader *loader = nullptr; + if (!m_options.file) { + loader = Reproducer::Instance().GetLoader(); + if (loader == nullptr) { + result.SetError( + "Not specifying a reproducer is only support during replay."); + result.SetStatus(eReturnStatusSuccessFinishNoResult); + return false; + } + } else { + loader_storage.emplace(m_options.file); + loader = &(*loader_storage); + if (Error err = loader->LoadIndex()) { + SetError(result, std::move(err)); + return false; + } + } + + // If we get here we should have a valid loader. + assert(loader); switch (m_options.provider) { case eReproducerProviderFiles: { @@ -594,101 +583,6 @@ private: CommandOptions m_options; }; -class CommandObjectReproducerVerify : public CommandObjectParsed { -public: - CommandObjectReproducerVerify(CommandInterpreter &interpreter) - : CommandObjectParsed(interpreter, "reproducer verify", - "Verify the contents of a reproducer. " - "If no reproducer is specified during replay, it " - "verifies the content of the current reproducer.", - nullptr) {} - - ~CommandObjectReproducerVerify() override = default; - - Options *GetOptions() override { return &m_options; } - - class CommandOptions : public Options { - public: - CommandOptions() : Options(), file() {} - - ~CommandOptions() override = default; - - Status SetOptionValue(uint32_t option_idx, StringRef option_arg, - ExecutionContext *execution_context) override { - Status error; - const int short_option = m_getopt_table[option_idx].val; - - switch (short_option) { - case 'f': - file.SetFile(option_arg, FileSpec::Style::native); - FileSystem::Instance().Resolve(file); - break; - default: - llvm_unreachable("Unimplemented option"); - } - - return error; - } - - void OptionParsingStarting(ExecutionContext *execution_context) override { - file.Clear(); - } - - ArrayRef GetDefinitions() override { - return makeArrayRef(g_reproducer_verify_options); - } - - FileSpec file; - }; - -protected: - bool DoExecute(Args &command, CommandReturnObject &result) override { - if (!command.empty()) { - result.AppendErrorWithFormat("'%s' takes no arguments", - m_cmd_name.c_str()); - return false; - } - - llvm::Optional loader_storage; - Loader *loader = - GetLoaderFromPathOrCurrent(loader_storage, result, m_options.file); - if (!loader) - return false; - - bool errors = false; - auto error_callback = [&](llvm::StringRef error) { - errors = true; - result.AppendError(error); - }; - - bool warnings = false; - auto warning_callback = [&](llvm::StringRef warning) { - warnings = true; - result.AppendWarning(warning); - }; - - auto note_callback = [&](llvm::StringRef warning) { - result.AppendMessage(warning); - }; - - Verifier verifier(loader); - verifier.Verify(error_callback, warning_callback, note_callback); - - if (warnings || errors) { - result.AppendMessage("reproducer verification failed"); - result.SetStatus(eReturnStatusFailed); - } else { - result.AppendMessage("reproducer verification succeeded"); - result.SetStatus(eReturnStatusSuccessFinishResult); - } - - return result.Succeeded(); - } - -private: - CommandOptions m_options; -}; - CommandObjectReproducer::CommandObjectReproducer( CommandInterpreter &interpreter) : CommandObjectMultiword( @@ -711,8 +605,6 @@ CommandObjectReproducer::CommandObjectReproducer( new CommandObjectReproducerStatus(interpreter))); LoadSubCommand("dump", CommandObjectSP(new CommandObjectReproducerDump(interpreter))); - LoadSubCommand("verify", CommandObjectSP( - new CommandObjectReproducerVerify(interpreter))); LoadSubCommand("xcrash", CommandObjectSP( new CommandObjectReproducerXCrash(interpreter))); } diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td index eaabf06..fbb6495 100644 --- a/lldb/source/Commands/Options.td +++ b/lldb/source/Commands/Options.td @@ -451,12 +451,6 @@ let Command = "reproducer dump" in { "provided, that reproducer is dumped.">; } -let Command = "reproducer verify" in { - def reproducer_verify_file : Option<"file", "f">, Group<1>, Arg<"Filename">, - Desc<"The reproducer path. If a reproducer is replayed and no path is " - "provided, that reproducer is dumped.">; -} - let Command = "reproducer xcrash" in { def reproducer_signal : Option<"signal", "s">, Group<1>, EnumArg<"None", "ReproducerSignalType()">, diff --git a/lldb/source/Utility/Reproducer.cpp b/lldb/source/Utility/Reproducer.cpp index 1f9ab8d..68c6419 100644 --- a/lldb/source/Utility/Reproducer.cpp +++ b/lldb/source/Utility/Reproducer.cpp @@ -268,94 +268,3 @@ bool Loader::HasFile(StringRef file) { auto it = std::lower_bound(m_files.begin(), m_files.end(), file.str()); return (it != m_files.end()) && (*it == file); } - -void Verifier::Verify( - llvm::function_ref error_callback, - llvm::function_ref warning_callback, - llvm::function_ref note_callack) const { - if (!m_loader) { - error_callback("invalid loader"); - return; - } - - FileSpec vfs_mapping = m_loader->GetFile(); - ErrorOr> buffer = - vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath()); - if (!buffer) { - error_callback("unable to read files: " + buffer.getError().message()); - return; - } - - IntrusiveRefCntPtr vfs = vfs::getVFSFromYAML( - std::move(buffer.get()), nullptr, vfs_mapping.GetPath()); - if (!vfs) { - error_callback("unable to initialize the virtual file system"); - return; - } - - auto &redirecting_vfs = static_cast(*vfs); - redirecting_vfs.setFallthrough(false); - - { - llvm::Expected working_dir = - GetDirectoryFrom(m_loader); - if (working_dir) { - if (!vfs->exists(*working_dir)) - warning_callback("working directory '" + *working_dir + "' not in VFS"); - vfs->setCurrentWorkingDirectory(*working_dir); - } else { - warning_callback("no working directory in reproducer: " + - toString(working_dir.takeError())); - } - } - - { - llvm::Expected home_dir = - GetDirectoryFrom(m_loader); - if (home_dir) { - if (!vfs->exists(*home_dir)) - warning_callback("home directory '" + *home_dir + "' not in VFS"); - } else { - warning_callback("no home directory in reproducer: " + - toString(home_dir.takeError())); - } - } - - { - Expected symbol_files = - m_loader->LoadBuffer(); - if (symbol_files) { - std::vector entries; - llvm::yaml::Input yin(*symbol_files); - yin >> entries; - for (const auto &entry : entries) { - if (!entry.module_path.empty() && !vfs->exists(entry.module_path)) { - warning_callback("'" + entry.module_path + "': module path for " + - entry.uuid + " not in VFS"); - } - if (!entry.symbol_path.empty() && !vfs->exists(entry.symbol_path)) { - warning_callback("'" + entry.symbol_path + "': symbol path for " + - entry.uuid + " not in VFS"); - } - } - } else { - llvm::consumeError(symbol_files.takeError()); - } - } - - // Missing files in the VFS are notes rather than warnings. Because the VFS - // is a snapshot, temporary files could have been removed between when they - // were recorded and when the reproducer was generated. - std::vector roots = redirecting_vfs.getRoots(); - for (llvm::StringRef root : roots) { - std::error_code ec; - vfs::recursive_directory_iterator iter(*vfs, root, ec); - vfs::recursive_directory_iterator end; - for (; iter != end && !ec; iter.increment(ec)) { - ErrorOr status = vfs->status(iter->path()); - if (!status) - note_callack("'" + iter->path().str() + - "': " + status.getError().message()); - } - } -} diff --git a/lldb/source/Utility/ReproducerProvider.cpp b/lldb/source/Utility/ReproducerProvider.cpp index d67c886..f555665 100644 --- a/lldb/source/Utility/ReproducerProvider.cpp +++ b/lldb/source/Utility/ReproducerProvider.cpp @@ -9,7 +9,6 @@ #include "lldb/Utility/ReproducerProvider.h" #include "lldb/Utility/ProcessInfo.h" #include "llvm/Support/FileSystem.h" -#include "llvm/Support/WithColor.h" #include "llvm/Support/raw_ostream.h" using namespace lldb_private; diff --git a/lldb/test/Shell/Reproducer/TestDebugSymbols.test b/lldb/test/Shell/Reproducer/TestDebugSymbols.test index 986452e..6a3cc12 100644 --- a/lldb/test/Shell/Reproducer/TestDebugSymbols.test +++ b/lldb/test/Shell/Reproducer/TestDebugSymbols.test @@ -12,7 +12,3 @@ # DUMP: uuid: AD52358C-94F8-3796-ADD6-B20FFAC00E5C # DUMP-NEXT: module path: /path/to/unstripped/executable # DUMP-NEXT: symbol path: /path/to/foo.dSYM/Contents/Resources/DWARF/foo - -# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %s --check-prefix VERIFY -# VERIFY: warning: '/path/to/unstripped/executable': module path for AD52358C-94F8-3796-ADD6-B20FFAC00E5C not in VFS -# VERIFY: warning: '/path/to/foo.dSYM/Contents/Resources/DWARF/foo': symbol path for AD52358C-94F8-3796-ADD6-B20FFAC00E5C not in VFS diff --git a/lldb/test/Shell/Reproducer/TestVerify.test b/lldb/test/Shell/Reproducer/TestVerify.test deleted file mode 100644 index 0b34e62..0000000 --- a/lldb/test/Shell/Reproducer/TestVerify.test +++ /dev/null @@ -1,27 +0,0 @@ -# RUN: rm -rf %t.repro -# RUN: rm -rf %t.repro2 -# RUN: %clang_host %S/Inputs/simple.c -g -o %t.out -# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture --capture-path %t.repro %t.out -# RUN: %lldb --replay %t.repro - -# RUN: echo "/bogus/home/dir" > %t.repro/home.txt -# RUN: echo "/bogus/current/working/dir" > %t.repro/cwd.txt - -# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %s -# CHECK: working directory '/bogus/current/working/dir' not in VFS -# CHECK: home directory '/bogus/home/dir' not in VFS - -# RUN: rm %t.repro/root/%S/Inputs/GDBRemoteCapture.in -# RUN: echo "CHECK: '%S/Inputs/GDBRemoteCapture.in': No such file or directory" > %t.check -# RUN: not %lldb -b -o 'reproducer verify -f %t.repro' 2>&1 | FileCheck %t.check - -# RUN: not %lldb --replay %t.repro 2>&1 | FileCheck %s - -# At this point the reproducer is too broken to ignore the verification issues. -# Capture a new reproducer and only change the home directory, which is -# recoverable as far as this test goes. - -# RUN: %lldb -x -b -s %S/Inputs/GDBRemoteCapture.in --capture --capture-path %t.repro2 %t.out -# RUN: echo "/bogus/home/dir" > %t.repro2/home.txt -# RUN: %lldb --replay %t.repro2 --reproducer-no-verify 2>&1 | FileCheck %s --check-prefix NO-VERIFY -# NO-VERIFY-NOT: home directory '/bogus/home/dir' not in VFS diff --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp index c66923f..13cb9be 100644 --- a/lldb/tools/driver/Driver.cpp +++ b/lldb/tools/driver/Driver.cpp @@ -800,11 +800,9 @@ EXAMPLES: llvm::Optional InitializeReproducer(llvm::StringRef argv0, opt::InputArgList &input_args) { if (auto *replay_path = input_args.getLastArg(OPT_replay)) { - SBReplayOptions replay_options; - replay_options.SetCheckVersion(!input_args.hasArg(OPT_no_version_check)); - replay_options.SetVerify(!input_args.hasArg(OPT_no_verification)); + const bool no_version_check = input_args.hasArg(OPT_no_version_check); if (const char *error = - SBReproducer::Replay(replay_path->getValue(), replay_options)) { + SBReproducer::Replay(replay_path->getValue(), no_version_check)) { WithColor::error() << "reproducer replay failed: " << error << '\n'; return 1; } diff --git a/lldb/tools/driver/Options.td b/lldb/tools/driver/Options.td index b3ffc2d..96f696e 100644 --- a/lldb/tools/driver/Options.td +++ b/lldb/tools/driver/Options.td @@ -234,8 +234,6 @@ def replay: Separate<["--", "-"], "replay">, HelpText<"Tells the debugger to replay a reproducer from .">; def no_version_check: F<"reproducer-no-version-check">, HelpText<"Disable the reproducer version check.">; -def no_verification: F<"reproducer-no-verify">, - HelpText<"Disable the reproducer verification.">; def no_generate_on_signal: F<"reproducer-no-generate-on-signal">, HelpText<"Don't generate reproducer when a signal is received.">; def generate_on_exit: F<"reproducer-generate-on-exit">, diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h index 055c0e5..af09c21 100644 --- a/llvm/include/llvm/Support/VirtualFileSystem.h +++ b/llvm/include/llvm/Support/VirtualFileSystem.h @@ -749,10 +749,6 @@ public: StringRef getExternalContentsPrefixDir() const; - void setFallthrough(bool Fallthrough); - - std::vector getRoots() const; - void dump(raw_ostream &OS) const; void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const; #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp index bbde44c..5b757c9 100644 --- a/llvm/lib/Support/VirtualFileSystem.cpp +++ b/llvm/lib/Support/VirtualFileSystem.cpp @@ -1159,17 +1159,6 @@ StringRef RedirectingFileSystem::getExternalContentsPrefixDir() const { return ExternalContentsPrefixDir; } -void RedirectingFileSystem::setFallthrough(bool Fallthrough) { - IsFallthrough = Fallthrough; -} - -std::vector RedirectingFileSystem::getRoots() const { - std::vector R; - for (const auto &Root : Roots) - R.push_back(Root->getName()); - return R; -} - void RedirectingFileSystem::dump(raw_ostream &OS) const { for (const auto &Root : Roots) dumpEntry(OS, Root.get());