Sorry. I can check this out this morning. I didn't have any test failures locally, but I'm guessing that test doesn't run on Windows.
On Mon, Feb 3, 2020 at 6:43 PM Jason Molenda <jmole...@apple.com> wrote: > I'm going to xfail it for tonight; we end up copying the base filename > into the ModuleSpec that we use for searching, > > if (!module_spec.GetUUID().IsValid()) { > if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec()) > module_spec.GetFileSpec().GetFilename() = > symbol_fspec.GetFilename(); > } > > So when the binary is /dir1/a.out and the (wrong) dSYM is > /dir2/a.out.dSYM/Contents/Resources/DWARF/a.out, and we did 'target symbols > add /dir2/a.out.dSYM', "a.out" gets copied into module_spec and after > Adrian's change, we search through the Target image list for a file called > "a.out", find one (the wrong one), try to construct a ModuleSP with that as > the ObjectFile and the /dir2/a.out.dSYM as the SymbolFile and it gets > caught as inconsistent and we return the new, less helpful, error message. > > I looked over his patch and I'm not sure where the logic went wrong here; > I'll step through the old version tomorrow. > > J > > > > On Feb 3, 2020, at 4:18 PM, Jason Molenda <jmole...@apple.com> wrote: > > > > 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@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@lists.llvm.org > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > > > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits