[Lldb-commits] [lldb] c25938d - Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Mon Feb 3 18:43:10 PST 2020
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
>
More information about the lldb-commits
mailing list