[Lldb-commits] [lldb] c25938d - Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols
Adrian McCarthy via lldb-commits
lldb-commits at lists.llvm.org
Tue Feb 4 11:18:05 PST 2020
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
>> >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200204/eeea18d3/attachment-0001.html>
More information about the lldb-commits
mailing list