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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
