Have SBTarget::AddModule force a possibly-slow search for the binary, and if the Target has no arch, initialize it with the binary's arch

[jasonmolenda]

Summary

This patch addresses two issues I found while looking into a problem yesterday. First, SBTarget::AddModule calls Target::GetOrCreate which will find the module in the global module list, the current target, and will call the DebugSymbols framework on macOS and look in specified directories, and do a Spotlight search of the local computer. But it doesn't call Symbols::DownloadObjectAndSymbolFile() with a "force expensive lookup" which we should assume the program is trying to do if they gave us a UUID to look up. This matches the behavior of target modules add -u .... today in the lldb command line interface.

Also, if the SB API user has created a Target which has no architecture, we normally will set the Target's arch as soon as we find a hint about the arch -- e.g. when we connect to a gdb remote serial protocol stub, or when we get a corefile. But in this case, they are creating a Target with no arch, adding a module to it. We should use the same scheme where a Target inherits the first likely ArchSpec we see when it has none of its own.

Add a test for these two things, using a fake dsym-for-uuid.sh shell (which Jonas has pointed out in the past, I really need to create a utility function that creates these given an array of uuid/binary/dsyms, which I agree with but haven't done yet. :/)

[jasonmolenda]

One more bit of explanation about the changes: There is an SBTarget::AddModule which takes individual parts of a module spec, and an SBTarget::AddModule which takes an SBModuleSpec. I was going to need to duplicate my code to force the expensive search & update the Target's arch if it didn't have one in both, so I had the first method create an SBModuleSpec with its information and call the second.

[bulbazord]

The idea seems fine to me. A few nits and comments, otherwise LGTM.

lldb/source/API/SBTarget.cpp

1517

nit: add a comment next to true to indicate that it's for copy_executable

1528–1529

nit: It's redundant to check sb_module.GetSP() because sb_module.IsValid() already does that.

lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py

40

I know this is already done in a few places but we should really use llvm-dwarfdump so this can be a more portable test...

[jasonmolenda]

lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py

40

Yeah Jonas made that point last week about another test I was getting the uuid from a file. I don't know how to refer to something in the build products directory in an API test like this, is that what you meant? It's not installed on a standard macOS system in any path.

[jasonmolenda]

Incorporate Alex's suggestions, not sure about using llvm-dwarfdump yet tho.

[bulbazord]

lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py

40

Yeah I was referring to the build product llvm-dwarfdump. I did a quick search through the test suite but it doesn't look like we do this anywhere right now, so we'll need to figure out how to do it first.

[bulbazord]

LGTM

lldb/source/API/SBTarget.cpp

 //===-- SBTarget.cpp ------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/API/SBTarget.h"
+#include "lldb/Symbol/LocateSymbolFile.h"
 #include "lldb/Utility/Instrumentation.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/lldb-public.h"
 
 #include "lldb/API/SBBreakpoint.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
lldb::SBModule SBTarget::AddModule(const char *path, const char *triple,
 
   return AddModule(path, triple, uuid_cstr, nullptr);
 }
 
 lldb::SBModule SBTarget::AddModule(const char *path, const char *triple,
                                    const char *uuid_cstr, const char *symfile) {
   LLDB_INSTRUMENT_VA(this, path, triple, uuid_cstr, symfile);
 
-  lldb::SBModule sb_module;
   TargetSP target_sp(GetSP());
-  if (target_sp) {
-    ModuleSpec module_spec;
-    if (path)
-      module_spec.GetFileSpec().SetFile(path, FileSpec::Style::native);
+  if (!target_sp)
+    return {};
+
 
-    if (uuid_cstr)
-      module_spec.GetUUID().SetFromStringRef(uuid_cstr);
 
-    if (triple)
-      module_spec.GetArchitecture() = Platform::GetAugmentedArchSpec(
-          target_sp->GetPlatform().get(), triple);
-    else
-      module_spec.GetArchitecture() = target_sp->GetArchitecture();
+    module_spec.GetArchitecture() =
+        Platform::GetAugmentedArchSpec(target_sp->GetPlatform().get(), triple);
 
-    if (symfile)
-      module_spec.GetSymbolFileSpec().SetFile(symfile, FileSpec::Style::native);
 
-    sb_module.SetSP(target_sp->GetOrCreateModule(module_spec, true /* notify */));
-  }
-  return sb_module;
+  SBModuleSpec sb_modulespec(module_spec);
+
+  return AddModule(sb_modulespec);
 }
 
 lldb::SBModule SBTarget::AddModule(const SBModuleSpec &module_spec) {
   LLDB_INSTRUMENT_VA(this, module_spec);
 
   lldb::SBModule sb_module;
   TargetSP target_sp(GetSP());
-  if (target_sp)
+  if (target_sp) {
+    sb_module.SetSP(target_sp->GetOrCreateModule(*module_spec.m_opaque_up,
+                                                 true /* notify */));
+    if (!sb_module.IsValid() && module_spec.m_opaque_up->GetUUID().IsValid()) {
+      Status error;
+      if (Symbols::DownloadObjectAndSymbolFile(*module_spec.m_opaque_up, error,
+                                               /* force_lookup */ true)) {
+        if (FileSystem::Instance().Exists(
+                module_spec.m_opaque_up->GetFileSpec())) {
           sb_module.SetSP(target_sp->GetOrCreateModule(*module_spec.m_opaque_up,
                                                        true /* notify */));
+        }
+      }
+    }
+  }
+  // If the target hasn't initialized any architecture yet, use the
+  // binary's architecture.
+  if (sb_module.IsValid() && !target_sp->GetArchitecture().IsValid() &&
+      sb_module.GetSP()->GetArchitecture().IsValid())
+    target_sp->SetArchitecture(sb_module.GetSP()->GetArchitecture());
   return sb_module;
 }
 
 bool SBTarget::AddModule(lldb::SBModule &module) {
   LLDB_INSTRUMENT_VA(this, module);
 
   TargetSP target_sp(GetSP());
   if (target_sp) {

lldb/test/API/python_api/target-arch-from-module/Makefile

+C_SOURCES := main.c
+
+include Makefile.rules

lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py

+"""
+An SBTarget with no arch, call AddModule, SBTarget's arch should be set.
+"""
+
+import os
+import subprocess
+import re
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TargetArchFromModule(TestBase):
+    @skipIf(
+        debug_info=no_match(["dsym"]),
+        bugnumber="This test is looking explicitly for a dSYM",
+    )
+    @skipUnlessDarwin
+    @skipIfRemote
+    def test_target_arch_init(self):
+        self.build()
+        aout_exe = self.getBuildArtifact("a.out")
+        aout_dsym = self.getBuildArtifact("a.out.dSYM")
+        hidden_dir = self.getBuildArtifact("hide.noindex")
+        hidden_aout_exe = self.getBuildArtifact("hide.noindex/a.out")
+        hidden_aout_dsym = self.getBuildArtifact("hide.noindex/a.out.dSYM")
+        dsym_for_uuid = self.getBuildArtifact("dsym-for-uuid.sh")
+
+        # We can hook in our dsym-for-uuid shell script to lldb with
+        # this env var instead of requiring a defaults write.
+        os.environ["LLDB_APPLE_DSYMFORUUID_EXECUTABLE"] = dsym_for_uuid
+        self.addTearDownHook(
+            lambda: os.environ.pop("LLDB_APPLE_DSYMFORUUID_EXECUTABLE", None)
+        )
+
+        dwarfdump_uuid_regex = re.compile("UUID: ([-0-9a-fA-F]+) \(([^\(]+)\) .*")
+        dwarfdump_cmd_output = subprocess.check_output(
+            ('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True
+        ).decode("utf-8")
+        aout_uuid = None
+        for line in dwarfdump_cmd_output.splitlines():
+            match = dwarfdump_uuid_regex.search(line)
+            if match:
+                aout_uuid = match.group(1)
+        self.assertNotEqual(aout_uuid, None, "Could not get uuid of built a.out")
+
+        ###  Create our dsym-for-uuid shell script which returns self.hidden_aout_exe.
+        shell_cmds = [
+            "#! /bin/sh",
+            "# the last argument is the uuid",
+            "while [ $# -gt 1 ]",
+            "do",
+            "  shift",
+            "done",
+            "ret=0",
+            'echo "<?xml version=\\"1.0\\" encoding=\\"UTF-8\\"?>"',
+            'echo "<!DOCTYPE plist PUBLIC \\"-//Apple//DTD PLIST 1.0//EN\\" \\"http://www.apple.com/DTDs/PropertyList-1.0.dtd\\">"',
+            'echo "<plist version=\\"1.0\\">"',
+            "",
+            'if [ "$1" = "%s" ]' % aout_uuid,
+            "then",
+            "  uuid=%s" % aout_uuid,
+            "  bin=%s" % hidden_aout_exe,
+            "  dsym=%s.dSYM/Contents/Resources/DWARF/%s"
+            % (hidden_aout_exe, os.path.basename(hidden_aout_exe)),
+            "fi",
+            'echo "  <dict>"',
+            'echo "    <key>$1</key>"',
+            'echo "    <dict>"',
+            'if [ -z "$uuid" -o -z "$bin" -o ! -f "$bin" ]',
+            "then",
+            '  echo "      <key>DBGError</key>"',
+            '  echo "      <string>not found by $0</string>"',
+            '  echo "    </dict>"',
+            '  echo "  </dict>"',
+            '  echo "</plist>"',
+            "  exit 0",
+            "fi",
+            "",
+            'echo "<key>DBGDSYMPath</key><string>$dsym</string>"',
+            'echo "<key>DBGSymbolRichExecutable</key><string>$bin</string>"',
+            'echo "</dict></dict></plist>"',
+            "exit $ret",
+        ]
+
+        with open(dsym_for_uuid, "w") as writer:
+            for l in shell_cmds:
+                writer.write(l + "\n")
+
+        os.chmod(dsym_for_uuid, 0o755)
+
+        # Move the main binary and its dSYM into the hide.noindex
+        # directory.  Now the only way lldb can find them is with
+        # the LLDB_APPLE_DSYMFORUUID_EXECUTABLE shell script -
+        # so we're testing that this dSYM discovery method works.
+        lldbutil.mkdir_p(hidden_dir)
+        os.rename(aout_exe, hidden_aout_exe)
+        os.rename(aout_dsym, hidden_aout_dsym)
+
+        target = self.dbg.CreateTarget("")
+        self.assertTrue(target.IsValid())
+        self.expect("target list", matching=False, substrs=["arch="])
+
+        m = target.AddModule(None, None, aout_uuid)
+        self.assertTrue(m.IsValid())
+        self.expect("target list", substrs=["arch="])

lldb/test/API/python_api/target-arch-from-module/main.c

+#include <stdio.h>
+int main() {
+  puts("HI i am a program");
+  return 0;
+}