[Lldb-commits] [lldb] c25938d - Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols
Adrian McCarthy via lldb-commits
lldb-commits at lists.llvm.org
Tue Feb 4 13:08:42 PST 2020
Suppose we give the traditional message (making the users and the test
happy) and then augment it with a little extra text to let developers
figure out where it went wrong.
symbol file '%s'%s does not match any existing module%s
symbol file '%s'%s does not match any existing module%s
(the symbol file could not be loaded)
symbol file '%s'%s does not match any existing module%s
(the object file could not be loaded)
Adrian.
On Tue, Feb 4, 2020 at 12:53 PM Jim Ingham <jingham at apple.com> wrote:
> The question isn't so much whether it makes a difference to the user than
> whether when somebody reports getting this error in a project that we can't
> get our hands on so all we have is the screen output (a very not uncommon
> circumstance for debuggers) the different error messages would be useful to
> us...
>
> Jim
>
>
> > On Feb 4, 2020, at 12:47 PM, Adrian McCarthy <amccarth at google.com>
> wrote:
> >
> > 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.)
> >
> > 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.
> >
> > On Tue, Feb 4, 2020 at 12:07 PM Jim Ingham <jingham at apple.com> wrote:
> > Seems like your change is more informative. Could we just fix the tests?
> >
> > Jim
> >
> >
> > > On Feb 4, 2020, at 11:18 AM, Adrian McCarthy via lldb-commits <
> lldb-commits at lists.llvm.org> wrote:
> > >
> > > I see why the behavior is different, but I'm not sure whether the new
> behavior is wrong.
> > >
> > > 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).
> > >
> > > 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.)
> > >
> > > I can send a patch to use the original message for each of the three
> error types that are now distinct.
> > >
> > > On Tue, Feb 4, 2020 at 8:49 AM Adrian McCarthy <amccarth at google.com>
> wrote:
> > > 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 <jmolenda at 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 <jmolenda at 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 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
> > > >
> > >
> > > _______________________________________________
> > > lldb-commits mailing list
> > > lldb-commits at lists.llvm.org
> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200204/3b6f3eda/attachment-0001.html>
More information about the lldb-commits
mailing list