[Lldb-commits] [PATCH] D157659: 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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

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



================
Comment at: lldb/source/API/SBTarget.cpp:1517
+      if (Symbols::DownloadObjectAndSymbolFile(*module_spec.m_opaque_up, error,
+                                               true)) {
+        if (FileSystem::Instance().Exists(
----------------
nit: add a comment next to `true` to indicate that it's for `copy_executable`


================
Comment at: lldb/source/API/SBTarget.cpp:1528-1529
+  // binary's architecture.
+  if (sb_module.IsValid() && !target_sp->GetArchitecture().IsValid() &&
+      sb_module.GetSP() && sb_module.GetSP()->GetArchitecture().IsValid())
+    target_sp->SetArchitecture(sb_module.GetSP()->GetArchitecture());
----------------
nit: It's redundant to check `sb_module.GetSP()` because `sb_module.IsValid()` already does that.


================
Comment at: lldb/test/API/python_api/target-arch-from-module/TestTargetArchFromModule.py:39
+        dwarfdump_cmd_output = subprocess.check_output(
+            ('/usr/bin/dwarfdump --uuid "%s"' % aout_exe), shell=True
+        ).decode("utf-8")
----------------
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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157659/new/

https://reviews.llvm.org/D157659