Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[amccarth]

Summary

• [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.

• If we successfully load a symbol file that seems to match, but then detect a file name mismatch, now we only issue a warning. We've already determined they match, so it seems pointless to change our minds.

• [NFC] Eliminated a namespace alias that wasn't particularly helpful.

[labath]

lldb/source/Commands/CommandObjectTarget.cpp

4102

This is not unreasonable in the non-pdb world. You can have a stripped version of a file somewhere prepared for deployment to some device (or downloaded from the device), and then you'll also have an unstripped version of that file (with the same name) in some build folder.

4147

This should be a regular assert according to https://lldb.llvm.org/resources/contributing.html#error-handling-and-use-of-assertions-in-lldb.

4173–4176

This part is not good. Everywhere else except pdbs this means that we were in fact unable to load the symbol file. What happens is that if we cannot load the object file representing the symbol file (at SymbolVendor.cpp:48), we fall back to creating a SymbolFile using the object file of the original module (line 52).

The effect of that is that the symbol file creation will always succeed, the previous checks are more-or-less useless, and the only way to check if we are really using the symbols from the file the user specified is to compare the file specs.

Now, PDB symbol files are abusing this fallback, and reading the symbols from the pdb files even though they were in fact asked to read them from the executable file. This is why this may sound like a "discrepancy" coming from the pdb world, but everywhere else this just means that the symbol file was not actually loaded.

[clayborg]

lldb/source/Commands/CommandObjectTarget.cpp

4147

This assert will never trigger with the above code already checking for empty on line 4123 and then checking if size > 1 on line 4140. Remove it?

4173

This could also fail if the symbol file spec was a bundle which got resolved when the module was loaded. If the user specified "/path/to/foo.dSYM" and the underlying code was able to resolve the symbol file in the dSYM bundle to "/path/to/foo.dSYM/Contents/Resources/DWARF/foo".

[amccarth]

Thanks for the feedback. Obviously I'm confused about how LLDB handles split debug info, so I need more clarification about how to proceed.

lldb/source/Commands/CommandObjectTarget.cpp

4102

Sure, if you've got two files with the same name in two directories that's fine.

But the loop tries peeling the extension off of the supplied file name and comparing it to the target's file name. I guess it's trying to match something like foo.unstripped to foo. Is that a typical thing?

4173–4176

Interesting. I did not expect that fallback behavior from the API.

PDB symbol files are abusing this fallback

I'm not sure there's abuse going on here. I'm not understanding something about how lldb models this situation. Consider the case where the user explicitly asks lldb to load symbols from a PDB:

(lldb) target add symbols D:\my_symbols\foo.pdb

Then symbol_file is the PDB and object_file is the executable or DLL. I would expect those file specs to match only in the case where the symbols are in the binary (in other words, when symbol file and object file are the same file). Even ignoring the PDB case, it seems this wouldn't even work in the case you mentioned above where the object file is stripped and the symbol file is an unstripped copy of the object file (possibly with a different name). You can also get here if the symbols were matched by UUID rather than file spec. Or when the symbols were matched by the basename minus some extension.

Given that dsyms work, I must be misunderstanding something here. Can you help me understand?

What's the right thing to do here? If I just undo this refactor, then we're back to silent failures when the symbols exist in a different file than the object.

[amccarth]

lldb/source/Commands/CommandObjectTarget.cpp

4147

It's true that this assert won't happen given the early returns above, so I will remove it if you like.

But I did it intentionally: (1) to document the requirement and (2) to protect against future changes accidentally breaking the assumptions. If the function were much shorter, then it might be obvious enough as is. But early returns, especially halfway down a long function, can be easy to miss.

[clayborg]

lldb/source/Commands/CommandObjectTarget.cpp

4147

Gotcha. It is fine to leave it in, just switch to assert() as Pavel mentioned.

[amccarth]

lldb/source/Commands/CommandObjectTarget.cpp

4173

In your example, the file specs do not match.

1. The old code would therefore reject the symbol file and return an error.
2. The new code would therefore emit a warning but proceed with the selected symbol file and return success.

Do you want either of these behaviors or something else?

[clayborg]

lldb/source/Commands/CommandObjectTarget.cpp

4173

all I know is I can and have typed:

(lldb) target symbols add /path/to/foo.dSYM

And it would successfully associate it with the binary, probably because the code on 4071 would see a UUID and accept it before it gets to this point.

What is the point of the warning? When would this happen?

[amccarth]

lldb/source/Commands/CommandObjectTarget.cpp

4173

There are examples in the thread with Pavel just below this thread. On Windows, your symbols may come from a file called foo.pdb for an executable called foo.exe. The old code would successfully load the PDB file, then decide it was no good because the file specs don't match. (Heck, at this point, we've just gone through a bunch of gyrations to allow matching of differing file specs.) So you'd get a mostly silent failure for code that actually worked.

My intent was to simply warn about such cases without actually forcing a failure, and to do so with some explicit feedback so that the user understands.

I'll wait to hear what Pavel says. If necessary, I'll just roll back this part of the change (so that this remains just a refactor), and I'll figure out a way around the problem in a later patch.

[labath]

lldb/source/Commands/CommandObjectTarget.cpp

4102

Ah, sorry, I missed that part.

Yes, this is also thing that's used sometimes (probably even more often than the first thing). One of the conventions for automatically finding separate debug files involves appending .debug to your file and then placing it in a special folder. I guess this was added to enable one to do "target symbols add /whereever/foo.debug" and have it match "foo".

That said, stripping the extension in a loop does seem like overkill.

4173–4176

Greg's use case will work because the bundle path will be resolved to the actual file (in PlatformDarwin::ResolveSymbolFile) before we get to this place.

Overall, I think changing this to a warning would be acceptable. Thinking about this more, I am not sure if the scenario I described below could happen (easily -- it might for some very weird elf files where we were able to extract the uuid, but cannot create an actual ObjectFileELF instance). So calling this a discrepancy would be fine.

And I do think that the current pdb behavior is a big discrepancy, which should be fixed. Either we should go the breakpad way, and introduce an "ObjectFilePDB" so that the pdb symbol file plugin(s) can behave like the dwarf code does, or we should change the SymbolFile class so that it does not require an object file instance in order to read symbols from a separate file (and then we can delete ObjectFileBreakpad).

Having a more explicit method of checking whether the symbol file creation was successful (instead of checking file specs) would not hurt either...

[amccarth]

I backed out the TODO and the warn-and-continue behavior to expedite this patch which is just supposed to be a refactor for readability.

We can revisit the issue of debug symbol file names not matching the object file names in the future (if necessary).

lldb/source/Commands/CommandObjectTarget.cpp

4102

OK, I'm removing the TODO in favor of a comment with the foo and foo.debug you gave. Whether it should be a loop or not can be addressed later. Whether we should do something that would also work for Windows (e.g., foo.exe matching foo.pdb) can also be considered later.

This patch is just supposed to be a refactor for readability/debuggability.

4173–4176

Thanks for all the clarification.

I am planning to introduce an ObjectFilePDB in a future patch to solve other problems.

To keep this patch a behavior-preserving refactor, I'll back out the warning message. Once PDBs can be loaded reliably, I can circle back here to see if there's a better way to detect failure and/or to communicate matching problems to the user.

[amccarth]

• Backed out the warn-and-continue behavior change. It should be addressed later.
• Changed lldb_assert to assert.
• Improved comment in place of the proposed TODO.

[clayborg]

Looks reasonable to me. Any objections from anyone else? We do have tests for this right?

[labath]

In D73594#1851044, @clayborg wrote:

Looks reasonable to me. Any objections from anyone else? We do have tests for this right?

LGTM too. There are tests for this command already, but of course, we could always use a bunch more, particularly for the "error" cases...

lldb/source/Commands/CommandObjectTarget.cpp

bool AddModuleSymbols(Target *target, ModuleSpec &module_spec, bool &flush,
     char symfile_path[PATH_MAX];
     symbol_fspec.GetPath(symfile_path, sizeof(symfile_path));
 
     if (!module_spec.GetUUID().IsValid()) {
       if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec())
         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(),
                                             0, 0, symfile_module_specs)) {
       // Now extract the module spec that matches the target architecture
       ModuleSpec target_arch_module_spec;
       ModuleSpec symfile_module_spec;
       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);
             }
           }
         }
       }
     }
 
     // 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
       if (!filename_no_extension)
         break;
 
       // Check if there was no extension to strip and the basename is the same
       if (filename_no_extension == module_spec.GetFileSpec().GetFilename())
         break;
 
       // 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 (num_matches > 1) {
+    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 (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));
+      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) {
-        ObjectFile *object_file = symbol_file->GetObjectFile();
+    if (!symbol_file) {
+      result.AppendErrorWithFormat("symbol file '%s' could not be loaded\n",
+                                   symfile_path);
+      result.SetStatus(eReturnStatusFailed);
+      module_sp->SetSymbolFileFileSpec(FileSpec());
+      return false;
+    }
+
+    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;
+    }
     
-        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());
+      result.AppendWarning(feedback_stream.GetData());
 
-          flush = true;
-          result.SetStatus(eReturnStatusSuccessFinishResult);
-          return true;
-        }
-      }
-      // Clear the symbol file spec if anything went wrong
-      module_sp->SetSymbolFileFileSpec(FileSpec());
-    }
-
-    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 << ')';
-    }
-    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;
-  }
 
   bool DoExecute(Args &args, CommandReturnObject &result) override {
     Target *target = m_exe_ctx.GetTargetPtr();
     result.SetStatus(eReturnStatusFailed);
     bool flush = false;
     ModuleSpec module_spec;
     const bool uuid_option_set =
         m_uuid_option_group.GetOptionValue().OptionWasSet();