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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits