[Lldb-commits] [lldb] c25938d - Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Tue Feb 4 12:53:22 PST 2020
I think the new error message will be harder for users to understand. When they try to add a dSYM that doesn't match any binary currently in the Target, the error message should make that clear so the user can realize that either (1) they picked the wrong dSYM, or (2) they thought a binary was loaded in the Target, but it isn't (yet). I don't think "error: the object file could not be loadd" is helpful here (or any generic error message we could supply once we have a Module with an ObjectFile and SymbolFile that are not matches).
I'll look at this more this afternoon.
> 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
>
More information about the lldb-commits
mailing list