<div dir="ltr">Given that the UUIDs in the test don't actually match, I think it's reasonable for the error message to say it's not a match. I'm not sure detecting the problem in a different step of the process makes that much difference to the user that it warrants a different message. (I know it sounds like I'm waffling.)<div><br></div><div>I'm happy to let others (e.g., Jason) make the call. These are Darwin-only tests, so I don't have a handy way to work through them.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 4, 2020 at 12:07 PM Jim Ingham <<a href="mailto:jingham@apple.com">jingham@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Seems like your change is more informative. Could we just fix the tests?<br>
<br>
Jim<br>
<br>
<br>
> On Feb 4, 2020, at 11:18 AM, Adrian McCarthy via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br>
> <br>
> I see why the behavior is different, but I'm not sure whether the new behavior is wrong.<br>
> <br>
> The old code nested a bunch of if-success statements so that any failure would fall through to a default error message (the one the tests is expecting).<br>
> <br>
> The new code is a linear set of if-failure-return statements so that failures at different steps get different messages. (So, arguably, the new error message is more specific to the failure, even though it's worded rather vaguely.)<br>
> <br>
> I can send a patch to use the original message for each of the three error types that are now distinct.<br>
> <br>
> On Tue, Feb 4, 2020 at 8:49 AM Adrian McCarthy <<a href="mailto:amccarth@google.com" target="_blank">amccarth@google.com</a>> wrote:<br>
> Sorry. I can check this out this morning.<br>
> <br>
> I didn't have any test failures locally, but I'm guessing that test doesn't run on Windows.<br>
> <br>
> On Mon, Feb 3, 2020 at 6:43 PM Jason Molenda <<a href="mailto:jmolenda@apple.com" target="_blank">jmolenda@apple.com</a>> wrote:<br>
> I'm going to xfail it for tonight; we end up copying the base filename into the ModuleSpec that we use for searching,<br>
> <br>
> if (!module_spec.GetUUID().IsValid()) {<br>
> if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())<br>
> module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();<br>
> }<br>
> <br>
> 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.<br>
> <br>
> I looked over his patch and I'm not sure where the logic went wrong here; I'll step through the old version tomorrow.<br>
> <br>
> J<br>
> <br>
> <br>
> > On Feb 3, 2020, at 4:18 PM, Jason Molenda <<a href="mailto:jmolenda@apple.com" target="_blank">jmolenda@apple.com</a>> wrote:<br>
> > <br>
> > 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,<br>
> > <br>
> > error: symbol file 'FILENAME' does not match any existing module<br>
> > <br>
> > but we're now getting,<br>
> > <br>
> > error: the object file could not be loaded<br>
> > <br>
> > and the test doesn't expect that.<br>
> > <br>
> > <br>
> > 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.<br>
> > <br>
> > <br>
> > <br>
> > <br>
> > J<br>
> > <br>
> > <br>
> >> On Feb 3, 2020, at 2:22 PM, Adrian McCarthy via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br>
> >> <br>
> >> <br>
> >> Author: Adrian McCarthy<br>
> >> Date: 2020-02-03T14:22:05-08:00<br>
> >> New Revision: c25938d57b1cf9534887313405bc409e570a9b69<br>
> >> <br>
> >> URL: <a href="https://github.com/llvm/llvm-project/commit/c25938d57b1cf9534887313405bc409e570a9b69" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/c25938d57b1cf9534887313405bc409e570a9b69</a><br>
> >> DIFF: <a href="https://github.com/llvm/llvm-project/commit/c25938d57b1cf9534887313405bc409e570a9b69.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/c25938d57b1cf9534887313405bc409e570a9b69.diff</a><br>
> >> <br>
> >> LOG: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols<br>
> >> <br>
> >> * [NFC] Renamed local `matching_module_list` to `matching_modules` for<br>
> >> conciseness.<br>
> >> <br>
> >> * [NFC] Eliminated redundant local variable `num_matches` to reduce the risk<br>
> >> that changes get it out of sync with `matching_modules.GetSize()`.<br>
> >> <br>
> >> * Used an early return from case where the symbol file specified matches<br>
> >> multiple modules. This is a slight behavior change, but it's an improvement:<br>
> >> It didn't make sense to tell the user that the symbol file simultaneously<br>
> >> matched multiple modules and no modules.<br>
> >> <br>
> >> * [NFC] Used an early return from the case where no matches are found, to<br>
> >> better align with LLVM coding style.<br>
> >> <br>
> >> * [NFC] Simplified call of `AppendWarningWithFormat("%s", stuff)` to<br>
> >> `AppendWarning(stuff)`. I don't think this adds any copies. It does<br>
> >> construct a StringRef, but it was going to have to scan the string for the<br>
> >> length anyway.<br>
> >> <br>
> >> * [NFC] Removed unnecessary comments and reworded others for clarity.<br>
> >> <br>
> >> * Used an early return if the symbol file could not be loaded. This is a<br>
> >> behavior change because previously it could fail silently.<br>
> >> <br>
> >> * Used an early return if the object file could not be retrieved from the<br>
> >> symbol file. Again, this is a change because now there's an error message.<br>
> >> <br>
> >> * [NFC] Eliminated a namespace alias that wasn't particularly helpful.<br>
> >> <br>
> >> Differential Revision: <a href="https://reviews.llvm.org/D73594" rel="noreferrer" target="_blank">https://reviews.llvm.org/D73594</a><br>
> >> <br>
> >> Added: <br>
> >> <br>
> >> <br>
> >> Modified: <br>
> >> lldb/source/Commands/CommandObjectTarget.cpp<br>
> >> <br>
> >> Removed: <br>
> >> <br>
> >> <br>
> >> <br>
> >> ################################################################################<br>
> >> diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp<br>
> >> index 1b51fbeb71d3..b08a29d081a0 100644<br>
> >> --- a/lldb/source/Commands/CommandObjectTarget.cpp<br>
> >> +++ b/lldb/source/Commands/CommandObjectTarget.cpp<br>
> >> @@ -4053,12 +4053,10 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {<br>
> >> module_spec.GetFileSpec().GetFilename() = symbol_fspec.GetFilename();<br>
> >> }<br>
> >> <br>
> >> - // We now have a module that represents a symbol file that can be used<br>
> >> - // for a module that might exist in the current target, so we need to<br>
> >> - // find that module in the target<br>
> >> - ModuleList matching_module_list;<br>
> >> + // Now module_spec represents a symbol file for a module that might exist<br>
> >> + // in the current target. Let's find possible matches.<br>
> >> + ModuleList matching_modules;<br>
> >> <br>
> >> - size_t num_matches = 0;<br>
> >> // First extract all module specs from the symbol file<br>
> >> lldb_private::ModuleSpecList symfile_module_specs;<br>
> >> if (ObjectFile::GetModuleSpecifications(module_spec.GetSymbolFileSpec(),<br>
> >> @@ -4069,34 +4067,30 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {<br>
> >> target_arch_module_spec.GetArchitecture() = target->GetArchitecture();<br>
> >> if (symfile_module_specs.FindMatchingModuleSpec(target_arch_module_spec,<br>
> >> symfile_module_spec)) {<br>
> >> - // See if it has a UUID?<br>
> >> if (symfile_module_spec.GetUUID().IsValid()) {<br>
> >> // It has a UUID, look for this UUID in the target modules<br>
> >> ModuleSpec symfile_uuid_module_spec;<br>
> >> symfile_uuid_module_spec.GetUUID() = symfile_module_spec.GetUUID();<br>
> >> target->GetImages().FindModules(symfile_uuid_module_spec,<br>
> >> - matching_module_list);<br>
> >> - num_matches = matching_module_list.GetSize();<br>
> >> + matching_modules);<br>
> >> }<br>
> >> }<br>
> >> <br>
> >> - if (num_matches == 0) {<br>
> >> - // No matches yet, iterate through the module specs to find a UUID<br>
> >> - // value that we can match up to an image in our target<br>
> >> - const size_t num_symfile_module_specs =<br>
> >> - symfile_module_specs.GetSize();<br>
> >> - for (size_t i = 0; i < num_symfile_module_specs && num_matches == 0;<br>
> >> - ++i) {<br>
> >> + if (matching_modules.IsEmpty()) {<br>
> >> + // No matches yet. Iterate through the module specs to find a UUID<br>
> >> + // value that we can match up to an image in our target.<br>
> >> + const size_t num_symfile_module_specs = symfile_module_specs.GetSize();<br>
> >> + for (size_t i = 0;<br>
> >> + i < num_symfile_module_specs && matching_modules.IsEmpty(); ++i) {<br>
> >> if (symfile_module_specs.GetModuleSpecAtIndex(<br>
> >> i, symfile_module_spec)) {<br>
> >> if (symfile_module_spec.GetUUID().IsValid()) {<br>
> >> - // It has a UUID, look for this UUID in the target modules<br>
> >> + // It has a UUID. Look for this UUID in the target modules.<br>
> >> ModuleSpec symfile_uuid_module_spec;<br>
> >> symfile_uuid_module_spec.GetUUID() =<br>
> >> symfile_module_spec.GetUUID();<br>
> >> target->GetImages().FindModules(symfile_uuid_module_spec,<br>
> >> - matching_module_list);<br>
> >> - num_matches = matching_module_list.GetSize();<br>
> >> + matching_modules);<br>
> >> }<br>
> >> }<br>
> >> }<br>
> >> @@ -4104,13 +4098,11 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {<br>
> >> }<br>
> >> <br>
> >> // Just try to match up the file by basename if we have no matches at<br>
> >> - // this point<br>
> >> - if (num_matches == 0) {<br>
> >> - target->GetImages().FindModules(module_spec, matching_module_list);<br>
> >> - num_matches = matching_module_list.GetSize();<br>
> >> - }<br>
> >> + // this point. For example, module foo might have symbols in foo.debug.<br>
> >> + if (matching_modules.IsEmpty())<br>
> >> + target->GetImages().FindModules(module_spec, matching_modules);<br>
> >> <br>
> >> - while (num_matches == 0) {<br>
> >> + while (matching_modules.IsEmpty()) {<br>
> >> ConstString filename_no_extension(<br>
> >> module_spec.GetFileSpec().GetFileNameStrippingExtension());<br>
> >> // Empty string returned, let's bail<br>
> >> @@ -4123,82 +4115,93 @@ class CommandObjectTargetSymbolsAdd : public CommandObjectParsed {<br>
> >> <br>
> >> // Replace basename with one fewer extension<br>
> >> module_spec.GetFileSpec().GetFilename() = filename_no_extension;<br>
> >> - target->GetImages().FindModules(module_spec, matching_module_list);<br>
> >> - num_matches = matching_module_list.GetSize();<br>
> >> + target->GetImages().FindModules(module_spec, matching_modules);<br>
> >> + }<br>
> >> +<br>
> >> + if (matching_modules.IsEmpty()) {<br>
> >> + StreamString ss_symfile_uuid;<br>
> >> + if (module_spec.GetUUID().IsValid()) {<br>
> >> + ss_symfile_uuid << " (";<br>
> >> + module_spec.GetUUID().Dump(&ss_symfile_uuid);<br>
> >> + ss_symfile_uuid << ')';<br>
> >> + }<br>
> >> + result.AppendErrorWithFormat(<br>
> >> + "symbol file '%s'%s does not match any existing module%s\n",<br>
> >> + symfile_path, ss_symfile_uuid.GetData(),<br>
> >> + !llvm::sys::fs::is_regular_file(symbol_fspec.GetPath())<br>
> >> + ? "\n please specify the full path to the symbol file"<br>
> >> + : "");<br>
> >> + result.SetStatus(eReturnStatusFailed);<br>
> >> + return false;<br>
> >> }<br>
> >> <br>
> >> - if (num_matches > 1) {<br>
> >> + if (matching_modules.GetSize() > 1) {<br>
> >> result.AppendErrorWithFormat("multiple modules match symbol file '%s', "<br>
> >> "use the --uuid option to resolve the "<br>
> >> "ambiguity.\n",<br>
> >> symfile_path);<br>
> >> - } else if (num_matches == 1) {<br>
> >> - ModuleSP module_sp(matching_module_list.GetModuleAtIndex(0));<br>
> >> -<br>
> >> - // The module has not yet created its symbol vendor, we can just give<br>
> >> - // the existing target module the symfile path to use for when it<br>
> >> - // decides to create it!<br>
> >> - module_sp->SetSymbolFileFileSpec(symbol_fspec);<br>
> >> -<br>
> >> - SymbolFile *symbol_file =<br>
> >> - module_sp->GetSymbolFile(true, &result.GetErrorStream());<br>
> >> - if (symbol_file) {<br>
> >> - ObjectFile *object_file = symbol_file->GetObjectFile();<br>
> >> -<br>
> >> - if (object_file && object_file->GetFileSpec() == symbol_fspec) {<br>
> >> - // Provide feedback that the symfile has been successfully added.<br>
> >> - const FileSpec &module_fs = module_sp->GetFileSpec();<br>
> >> - result.AppendMessageWithFormat(<br>
> >> - "symbol file '%s' has been added to '%s'\n", symfile_path,<br>
> >> - module_fs.GetPath().c_str());<br>
> >> -<br>
> >> - // Let clients know something changed in the module if it is<br>
> >> - // currently loaded<br>
> >> - ModuleList module_list;<br>
> >> - module_list.Append(module_sp);<br>
> >> - target->SymbolsDidLoad(module_list);<br>
> >> -<br>
> >> - // Make sure we load any scripting resources that may be embedded<br>
> >> - // in the debug info files in case the platform supports that.<br>
> >> - Status error;<br>
> >> - StreamString feedback_stream;<br>
> >> - module_sp->LoadScriptingResourceInTarget(target, error,<br>
> >> - &feedback_stream);<br>
> >> - if (error.Fail() && error.AsCString())<br>
> >> - result.AppendWarningWithFormat(<br>
> >> - "unable to load scripting data for module %s - error "<br>
> >> - "reported was %s",<br>
> >> - module_sp->GetFileSpec()<br>
> >> - .GetFileNameStrippingExtension()<br>
> >> - .GetCString(),<br>
> >> - error.AsCString());<br>
> >> - else if (feedback_stream.GetSize())<br>
> >> - result.AppendWarningWithFormat("%s", feedback_stream.GetData());<br>
> >> -<br>
> >> - flush = true;<br>
> >> - result.SetStatus(eReturnStatusSuccessFinishResult);<br>
> >> - return true;<br>
> >> - }<br>
> >> - }<br>
> >> - // Clear the symbol file spec if anything went wrong<br>
> >> + result.SetStatus(eReturnStatusFailed);<br>
> >> + return false;<br>
> >> + }<br>
> >> + <br>
> >> + assert(matching_modules.GetSize() == 1);<br>
> >> + ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));<br>
> >> +<br>
> >> + // The module has not yet created its symbol vendor, we can just give<br>
> >> + // the existing target module the symfile path to use for when it<br>
> >> + // decides to create it!<br>
> >> + module_sp->SetSymbolFileFileSpec(symbol_fspec);<br>
> >> +<br>
> >> + SymbolFile *symbol_file =<br>
> >> + module_sp->GetSymbolFile(true, &result.GetErrorStream());<br>
> >> + if (!symbol_file) {<br>
> >> + result.AppendErrorWithFormat("symbol file '%s' could not be loaded\n",<br>
> >> + symfile_path);<br>
> >> + result.SetStatus(eReturnStatusFailed);<br>
> >> module_sp->SetSymbolFileFileSpec(FileSpec());<br>
> >> + return false;<br>
> >> }<br>
> >> <br>
> >> - namespace fs = llvm::sys::fs;<br>
> >> - StreamString ss_symfile_uuid;<br>
> >> - if (module_spec.GetUUID().IsValid()) {<br>
> >> - ss_symfile_uuid << " (";<br>
> >> - module_spec.GetUUID().Dump(&ss_symfile_uuid);<br>
> >> - ss_symfile_uuid << ')';<br>
> >> + ObjectFile *object_file = symbol_file->GetObjectFile();<br>
> >> + if (!object_file || object_file->GetFileSpec() != symbol_fspec) {<br>
> >> + result.AppendError("the object file could not be loaded\n");<br>
> >> + result.SetStatus(eReturnStatusFailed);<br>
> >> + module_sp->SetSymbolFileFileSpec(FileSpec());<br>
> >> + return false;<br>
> >> }<br>
> >> - result.AppendErrorWithFormat(<br>
> >> - "symbol file '%s'%s does not match any existing module%s\n",<br>
> >> - symfile_path, ss_symfile_uuid.GetData(),<br>
> >> - !fs::is_regular_file(symbol_fspec.GetPath())<br>
> >> - ? "\n please specify the full path to the symbol file"<br>
> >> - : "");<br>
> >> - result.SetStatus(eReturnStatusFailed);<br>
> >> - return false;<br>
> >> + <br>
> >> + // Provide feedback that the symfile has been successfully added.<br>
> >> + const FileSpec &module_fs = module_sp->GetFileSpec();<br>
> >> + result.AppendMessageWithFormat(<br>
> >> + "symbol file '%s' has been added to '%s'\n", symfile_path,<br>
> >> + module_fs.GetPath().c_str());<br>
> >> +<br>
> >> + // Let clients know something changed in the module if it is<br>
> >> + // currently loaded<br>
> >> + ModuleList module_list;<br>
> >> + module_list.Append(module_sp);<br>
> >> + target->SymbolsDidLoad(module_list);<br>
> >> +<br>
> >> + // Make sure we load any scripting resources that may be embedded<br>
> >> + // in the debug info files in case the platform supports that.<br>
> >> + Status error;<br>
> >> + StreamString feedback_stream;<br>
> >> + module_sp->LoadScriptingResourceInTarget(target, error,<br>
> >> + &feedback_stream);<br>
> >> + if (error.Fail() && error.AsCString())<br>
> >> + result.AppendWarningWithFormat(<br>
> >> + "unable to load scripting data for module %s - error "<br>
> >> + "reported was %s",<br>
> >> + module_sp->GetFileSpec()<br>
> >> + .GetFileNameStrippingExtension()<br>
> >> + .GetCString(),<br>
> >> + error.AsCString());<br>
> >> + else if (feedback_stream.GetSize())<br>
> >> + result.AppendWarning(feedback_stream.GetData());<br>
> >> +<br>
> >> + flush = true;<br>
> >> + result.SetStatus(eReturnStatusSuccessFinishResult);<br>
> >> + return true;<br>
> >> }<br>
> >> <br>
> >> bool DoExecute(Args &args, CommandReturnObject &result) override {<br>
> >> <br>
> >> <br>
> >> <br>
> >> _______________________________________________<br>
> >> lldb-commits mailing list<br>
> >> <a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
> >> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
> > <br>
> <br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
<br>
</blockquote></div>