[Lldb-commits] [lldb] c25938d - Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Mon Feb 3 16:18:03 PST 2020
This is causing a test failure on the macos cmake bot in TestAddDsymCommand.py where we load a binary with uuid A, then try to add-dsym a dSYM uuid B and the test is looking for the error text,
error: symbol file 'FILENAME' does not match any existing module
but we're now getting,
error: the object file could not be loaded
and the test doesn't expect that.
I'm looking into this, to see if I can get the more specific error message back in here again. I may xfail the test if I don't have a patch done tonight. I'd rather not test for this new generic error message if it's avoidable.
J
> On Feb 3, 2020, at 2:22 PM, Adrian McCarthy via lldb-commits <lldb-commits at lists.llvm.org> wrote:
>
>
> Author: Adrian McCarthy
> Date: 2020-02-03T14:22:05-08:00
> New Revision: c25938d57b1cf9534887313405bc409e570a9b69
>
> URL: https://github.com/llvm/llvm-project/commit/c25938d57b1cf9534887313405bc409e570a9b69
> DIFF: https://github.com/llvm/llvm-project/commit/c25938d57b1cf9534887313405bc409e570a9b69.diff
>
> LOG: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols
>
> * [NFC] Renamed local `matching_module_list` to `matching_modules` for
> conciseness.
>
> * [NFC] Eliminated redundant local variable `num_matches` to reduce the risk
> that changes get it out of sync with `matching_modules.GetSize()`.
>
> * Used an early return from case where the symbol file specified matches
> multiple modules. This is a slight behavior change, but it's an improvement:
> It didn't make sense to tell the user that the symbol file simultaneously
> matched multiple modules and no modules.
>
> * [NFC] Used an early return from the case where no matches are found, to
> better align with LLVM coding style.
>
> * [NFC] Simplified call of `AppendWarningWithFormat("%s", stuff)` to
> `AppendWarning(stuff)`. I don't think this adds any copies. It does
> construct a StringRef, but it was going to have to scan the string for the
> length anyway.
>
> * [NFC] Removed unnecessary comments and reworded others for clarity.
>
> * Used an early return if the symbol file could not be loaded. This is a
> behavior change because previously it could fail silently.
>
> * Used an early return if the object file could not be retrieved from the
> symbol file. Again, this is a change because now there's an error message.
>
> * [NFC] Eliminated a namespace alias that wasn't particularly helpful.
>
> Differential Revision: https://reviews.llvm.org/D73594
>
> Added:
>
>
> Modified:
> lldb/source/Commands/CommandObjectTarget.cpp
>
> Removed:
>
>
>
> ################################################################################
> diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
> index 1b51fbeb71d3..b08a29d081a0 100644
> --- a/lldb/source/Commands/CommandObjectTarget.cpp
> +++ b/lldb/source/Commands/CommandObjectTarget.cpp
> @@ -4053,12 +4053,10 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
> module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();
> }
>
> - // We now have a module that represents a symbol file that can be used
> - // for a module that might exist in the current target, so we need to
> - // find that module in the target
> - ModuleList matching_module_list;
> + // Now module_spec represents a symbol file for a module that might exist
> + // in the current target. Let's find possible matches.
> + ModuleList matching_modules;
>
> - size_t num_matches = 0;
> // First extract all module specs from the symbol file
> lldb_private::ModuleSpecList symfile_module_specs;
> if (ObjectFile::GetModuleSpecifications(module_spec.GetSymbolFileSpec(),
> @@ -4069,34 +4067,30 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
> target_arch_module_spec.GetArchitecture() = target->GetArchitecture();
> if (symfile_module_specs.FindMatchingModuleSpec(target_arch_module_spec,
> symfile_module_spec)) {
> - // See if it has a UUID?
> if (symfile_module_spec.GetUUID().IsValid()) {
> // It has a UUID, look for this UUID in the target modules
> ModuleSpec symfile_uuid_module_spec;
> symfile_uuid_module_spec.GetUUID() = symfile_module_spec.GetUUID();
> target->GetImages().FindModules(symfile_uuid_module_spec,
> - matching_module_list);
> - num_matches = matching_module_list.GetSize();
> + matching_modules);
> }
> }
>
> - if (num_matches == 0) {
> - // No matches yet, iterate through the module specs to find a UUID
> - // value that we can match up to an image in our target
> - const size_t num_symfile_module_specs =
> - symfile_module_specs.GetSize();
> - for (size_t i = 0; i < num_symfile_module_specs && num_matches == 0;
> - ++i) {
> + if (matching_modules.IsEmpty()) {
> + // No matches yet. Iterate through the module specs to find a UUID
> + // value that we can match up to an image in our target.
> + const size_t num_symfile_module_specs = symfile_module_specs.GetSize();
> + for (size_t i = 0;
> + i < num_symfile_module_specs && matching_modules.IsEmpty(); ++i) {
> if (symfile_module_specs.GetModuleSpecAtIndex(
> i, symfile_module_spec)) {
> if (symfile_module_spec.GetUUID().IsValid()) {
> - // It has a UUID, look for this UUID in the target modules
> + // It has a UUID. Look for this UUID in the target modules.
> ModuleSpec symfile_uuid_module_spec;
> symfile_uuid_module_spec.GetUUID() =
> symfile_module_spec.GetUUID();
> target->GetImages().FindModules(symfile_uuid_module_spec,
> - matching_module_list);
> - num_matches = matching_module_list.GetSize();
> + matching_modules);
> }
> }
> }
> @@ -4104,13 +4098,11 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
> }
>
> // Just try to match up the file by basename if we have no matches at
> - // this point
> - if (num_matches == 0) {
> - target->GetImages().FindModules(module_spec, matching_module_list);
> - num_matches = matching_module_list.GetSize();
> - }
> + // this point. For example, module foo might have symbols in foo.debug.
> + if (matching_modules.IsEmpty())
> + target->GetImages().FindModules(module_spec, matching_modules);
>
> - while (num_matches == 0) {
> + while (matching_modules.IsEmpty()) {
> ConstString filename_no_extension(
> module_spec.GetFileSpec().GetFileNameStrippingExtension());
> // Empty string returned, let's bail
> @@ -4123,82 +4115,93 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {
>
> // Replace basename with one fewer extension
> module_spec.GetFileSpec().GetFilename() = filename_no_extension;
> - target->GetImages().FindModules(module_spec, matching_module_list);
> - num_matches = matching_module_list.GetSize();
> + target->GetImages().FindModules(module_spec, matching_modules);
> + }
> +
> + if (matching_modules.IsEmpty()) {
> + StreamString ss_symfile_uuid;
> + if (module_spec.GetUUID().IsValid()) {
> + ss_symfile_uuid << " (";
> + module_spec.GetUUID().Dump(&ss_symfile_uuid);
> + ss_symfile_uuid << ')';
> + }
> + result.AppendErrorWithFormat(
> + "symbol file '%s'%s does not match any existing module%s\n",
> + symfile_path, ss_symfile_uuid.GetData(),
> + !llvm::sys::fs::is_regular_file(symbol_fspec.GetPath())
> + ? "\n please specify the full path to the symbol file"
> + : "");
> + result.SetStatus(eReturnStatusFailed);
> + return false;
> }
>
> - if (num_matches > 1) {
> + if (matching_modules.GetSize() > 1) {
> result.AppendErrorWithFormat("multiple modules match symbol file '%s', "
> "use the --uuid option to resolve the "
> "ambiguity.\n",
> symfile_path);
> - } else if (num_matches == 1) {
> - ModuleSP module_sp(matching_module_list.GetModuleAtIndex(0));
> -
> - // The module has not yet created its symbol vendor, we can just give
> - // the existing target module the symfile path to use for when it
> - // decides to create it!
> - module_sp->SetSymbolFileFileSpec(symbol_fspec);
> -
> - SymbolFile *symbol_file =
> - module_sp->GetSymbolFile(true, &result.GetErrorStream());
> - if (symbol_file) {
> - ObjectFile *object_file = symbol_file->GetObjectFile();
> -
> - if (object_file && object_file->GetFileSpec() == symbol_fspec) {
> - // Provide feedback that the symfile has been successfully added.
> - const FileSpec &module_fs = module_sp->GetFileSpec();
> - result.AppendMessageWithFormat(
> - "symbol file '%s' has been added to '%s'\n", symfile_path,
> - module_fs.GetPath().c_str());
> -
> - // Let clients know something changed in the module if it is
> - // currently loaded
> - ModuleList module_list;
> - module_list.Append(module_sp);
> - target->SymbolsDidLoad(module_list);
> -
> - // Make sure we load any scripting resources that may be embedded
> - // in the debug info files in case the platform supports that.
> - Status error;
> - StreamString feedback_stream;
> - module_sp->LoadScriptingResourceInTarget(target, error,
> - &feedback_stream);
> - if (error.Fail() && error.AsCString())
> - result.AppendWarningWithFormat(
> - "unable to load scripting data for module %s - error "
> - "reported was %s",
> - module_sp->GetFileSpec()
> - .GetFileNameStrippingExtension()
> - .GetCString(),
> - error.AsCString());
> - else if (feedback_stream.GetSize())
> - result.AppendWarningWithFormat("%s", feedback_stream.GetData());
> -
> - flush = true;
> - result.SetStatus(eReturnStatusSuccessFinishResult);
> - return true;
> - }
> - }
> - // Clear the symbol file spec if anything went wrong
> + result.SetStatus(eReturnStatusFailed);
> + return false;
> + }
> +
> + assert(matching_modules.GetSize() == 1);
> + ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));
> +
> + // The module has not yet created its symbol vendor, we can just give
> + // the existing target module the symfile path to use for when it
> + // decides to create it!
> + module_sp->SetSymbolFileFileSpec(symbol_fspec);
> +
> + SymbolFile *symbol_file =
> + module_sp->GetSymbolFile(true, &result.GetErrorStream());
> + if (!symbol_file) {
> + result.AppendErrorWithFormat("symbol file '%s' could not be loaded\n",
> + symfile_path);
> + result.SetStatus(eReturnStatusFailed);
> module_sp->SetSymbolFileFileSpec(FileSpec());
> + return false;
> }
>
> - namespace fs = llvm::sys::fs;
> - StreamString ss_symfile_uuid;
> - if (module_spec.GetUUID().IsValid()) {
> - ss_symfile_uuid << " (";
> - module_spec.GetUUID().Dump(&ss_symfile_uuid);
> - ss_symfile_uuid << ')';
> + ObjectFile *object_file = symbol_file->GetObjectFile();
> + if (!object_file || object_file->GetFileSpec() != symbol_fspec) {
> + result.AppendError("the object file could not be loaded\n");
> + result.SetStatus(eReturnStatusFailed);
> + module_sp->SetSymbolFileFileSpec(FileSpec());
> + return false;
> }
> - result.AppendErrorWithFormat(
> - "symbol file '%s'%s does not match any existing module%s\n",
> - symfile_path, ss_symfile_uuid.GetData(),
> - !fs::is_regular_file(symbol_fspec.GetPath())
> - ? "\n please specify the full path to the symbol file"
> - : "");
> - result.SetStatus(eReturnStatusFailed);
> - return false;
> +
> + // Provide feedback that the symfile has been successfully added.
> + const FileSpec &module_fs = module_sp->GetFileSpec();
> + result.AppendMessageWithFormat(
> + "symbol file '%s' has been added to '%s'\n", symfile_path,
> + module_fs.GetPath().c_str());
> +
> + // Let clients know something changed in the module if it is
> + // currently loaded
> + ModuleList module_list;
> + module_list.Append(module_sp);
> + target->SymbolsDidLoad(module_list);
> +
> + // Make sure we load any scripting resources that may be embedded
> + // in the debug info files in case the platform supports that.
> + Status error;
> + StreamString feedback_stream;
> + module_sp->LoadScriptingResourceInTarget(target, error,
> + &feedback_stream);
> + if (error.Fail() && error.AsCString())
> + result.AppendWarningWithFormat(
> + "unable to load scripting data for module %s - error "
> + "reported was %s",
> + module_sp->GetFileSpec()
> + .GetFileNameStrippingExtension()
> + .GetCString(),
> + error.AsCString());
> + else if (feedback_stream.GetSize())
> + result.AppendWarning(feedback_stream.GetData());
> +
> + flush = true;
> + result.SetStatus(eReturnStatusSuccessFinishResult);
> + return true;
> }
>
> bool DoExecute(Args &args, CommandReturnObject &result) override {
>
>
>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
More information about the lldb-commits
mailing list