Fix after c25938d

[amccarth]

Summary

My refactor caused some changes in error reporting that TestAddDsymCommand.py was checking, so this restores some of the changes to preserve the old behavior.

Putting this through review rather than committing directly because it's one of a couple alternatives discussed, and the affected test is currently XFAILed, so we have time to decide the best way forward.

[jasonmolenda]

This looks great to me, thanks! If it's easy to add on, could you remove the xfail from lldb/packages/Python/lldbsuite/test/commands/add-dsym/uuid/TestAddDsymCommand.py in the same commit? I verified that the original test passes with this patch.

lldb/packages/Python/lldbsuite/test/commands/add-dsym/uuid/TestAddDsymCommand.py

class AddDsymCommandCase(TestBase):
 
     def setUp(self):
         TestBase.setUp(self)
         self.template = 'main.cpp.template'
         self.source = 'main.cpp'
         self.teardown_hook_added = False
 
     @no_debug_info_test
-    @expectedFailureDarwin('until AdrianM or I find a fix for his 2020-02-03 CommandObjectTarget change')
     def test_add_dsym_command_with_error(self):
         """Test that the 'add-dsym' command informs the user about failures."""
 
         # Call the program generator to produce main.cpp, version 1.
         self.generate_main_cpp(version=1)
         self.buildDefault(dictionary={'MAKE_DSYM':'YES'})
 
         # Insert some delay and then call the program generator to produce

lldb/source/Commands/CommandObjectTarget.cpp

while (matching_modules.IsEmpty()) {
       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_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 (matching_modules.GetSize() > 1) {
       result.AppendErrorWithFormat("multiple modules match symbol file '%s', "
                                    "use the --uuid option to resolve the "
                                    "ambiguity.\n",
                                    symfile_path);
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
-    
-    assert(matching_modules.GetSize() == 1);
+    if (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;
-    }
-
+      if (symbol_file) {
         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;
-    }
-    
+        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.AppendWarning(feedback_stream.GetData());
 
           flush = true;
           result.SetStatus(eReturnStatusSuccessFinishResult);
           return true;
         }
+      }
+      // Clear the symbol file spec if anything went wrong
+      module_sp->SetSymbolFileFileSpec(FileSpec());
+    }
+
+    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;
+  }
 
   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();