[Lldb-commits] [lldb] [lldb] Unify Platform::ResolveExecutable (PR #96256)
via lldb-commits
lldb-commits at lists.llvm.org
Thu Jun 20 17:24:19 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Jonas Devlieghere (JDevlieghere)
<details>
<summary>Changes</summary>
The Platform class currently has two functions to resolve an executable: `ResolveExecutable` and `ResolveRemoteExecutable`. The former strictly deals with local files while the latter can handle potentially remote files. I couldn't figure out why the distinction matters, at the latter is a super-set of the former.
To make things even more confusion, we had a similar but not identical implementation in RemoteAwarePlatform where its implementation of `ResolveExecutable` could handle remote files. To top it all off, we had copy-pasted implementation, dead code included in `PlatformAppleSimulator` and `PlatformRemoteDarwinDevice`.
I went ahead and unified all the different implementation on the original `ResolveRemoteExecutable` implementation. As far as I can tell, it should work for every other platform, and the test suite (on macOS) seems to agree with me, except for a small wording change.
---
Patch is 23.46 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/96256.diff
11 Files Affected:
- (modified) lldb/include/lldb/Target/Platform.h (+4-5)
- (modified) lldb/include/lldb/Target/RemoteAwarePlatform.h (-4)
- (modified) lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp (-75)
- (modified) lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h (-4)
- (modified) lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp (-65)
- (modified) lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h (-4)
- (modified) lldb/source/Target/Platform.cpp (+5-43)
- (modified) lldb/source/Target/RemoteAwarePlatform.cpp (-139)
- (modified) lldb/test/API/assert_messages_test/TestAssertMessages.py (+1-1)
- (modified) lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py (+1-1)
- (modified) lldb/unittests/Target/RemoteAwarePlatformTest.cpp (+7-6)
``````````diff
diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h
index e05c79cb501bf..75c37be6c2af2 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -138,9 +138,11 @@ class Platform : public PluginInterface {
/// \return
/// Returns \b true if this Platform plug-in was able to find
/// a suitable executable, \b false otherwise.
+ /// @{
virtual Status ResolveExecutable(const ModuleSpec &module_spec,
- lldb::ModuleSP &module_sp,
+ lldb::ModuleSP &exe_module_sp,
const FileSpecList *module_search_paths_ptr);
+ /// @}
/// Find a symbol file given a symbol file module specification.
///
@@ -1004,10 +1006,7 @@ class Platform : public PluginInterface {
virtual const char *GetCacheHostname();
- virtual Status
- ResolveRemoteExecutable(const ModuleSpec &module_spec,
- lldb::ModuleSP &exe_module_sp,
- const FileSpecList *module_search_paths_ptr);
+ /// The method is virtual for mocking in the unit tests.
private:
typedef std::function<Status(const ModuleSpec &)> ModuleResolver;
diff --git a/lldb/include/lldb/Target/RemoteAwarePlatform.h b/lldb/include/lldb/Target/RemoteAwarePlatform.h
index 0b9d79f9ff038..6fbeec7888a98 100644
--- a/lldb/include/lldb/Target/RemoteAwarePlatform.h
+++ b/lldb/include/lldb/Target/RemoteAwarePlatform.h
@@ -23,10 +23,6 @@ class RemoteAwarePlatform : public Platform {
bool GetModuleSpec(const FileSpec &module_file_spec, const ArchSpec &arch,
ModuleSpec &module_spec) override;
- Status
- ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr) override;
-
lldb::user_id_t OpenFile(const FileSpec &file_spec, File::OpenOptions flags,
uint32_t mode, Status &error) override;
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
index 0c4b566b7d535..c27a34b7201a2 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -383,81 +383,6 @@ PlatformSP PlatformAppleSimulator::CreateInstance(
return PlatformSP();
}
-Status PlatformAppleSimulator::ResolveExecutable(
- const ModuleSpec &module_spec, lldb::ModuleSP &exe_module_sp,
- const FileSpecList *module_search_paths_ptr) {
- Status error;
- // Nothing special to do here, just use the actual file and architecture
-
- ModuleSpec resolved_module_spec(module_spec);
-
- // If we have "ls" as the exe_file, resolve the executable loation based on
- // the current path variables
- // TODO: resolve bare executables in the Platform SDK
- // if (!resolved_exe_file.Exists())
- // resolved_exe_file.ResolveExecutableLocation ();
-
- // Resolve any executable within a bundle on MacOSX
- // TODO: verify that this handles shallow bundles, if not then implement one
- // ourselves
- Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
-
- if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec())) {
- if (resolved_module_spec.GetArchitecture().IsValid()) {
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- NULL, NULL, NULL);
-
- if (exe_module_sp && exe_module_sp->GetObjectFile())
- return error;
- exe_module_sp.reset();
- }
- // No valid architecture was specified or the exact ARM slice wasn't found
- // so ask the platform for the architectures that we should be using (in
- // the correct order) and see if we can find a match that way
- StreamString arch_names;
- llvm::ListSeparator LS;
- ArchSpec platform_arch;
- for (const ArchSpec &arch : GetSupportedArchitectures({})) {
- resolved_module_spec.GetArchitecture() = arch;
-
- // Only match x86 with x86 and x86_64 with x86_64...
- if (!module_spec.GetArchitecture().IsValid() ||
- module_spec.GetArchitecture().GetCore() ==
- resolved_module_spec.GetArchitecture().GetCore()) {
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- NULL, NULL, NULL);
- // Did we find an executable using one of the
- if (error.Success()) {
- if (exe_module_sp && exe_module_sp->GetObjectFile())
- break;
- else
- error.SetErrorToGenericError();
- }
-
- arch_names << LS << platform_arch.GetArchitectureName();
- }
- }
-
- if (error.Fail() || !exe_module_sp) {
- if (FileSystem::Instance().Readable(resolved_module_spec.GetFileSpec())) {
- error.SetErrorStringWithFormatv(
- "'{0}' doesn't contain any '{1}' platform architectures: {2}",
- resolved_module_spec.GetFileSpec(), GetPluginName(),
- arch_names.GetString());
- } else {
- error.SetErrorStringWithFormat(
- "'%s' is not readable",
- resolved_module_spec.GetFileSpec().GetPath().c_str());
- }
- }
- } else {
- error.SetErrorStringWithFormat("'%s' does not exist",
- module_spec.GetFileSpec().GetPath().c_str());
- }
-
- return error;
-}
-
Status PlatformAppleSimulator::GetSymbolFile(const FileSpec &platform_file,
const UUID *uuid_ptr,
FileSpec &local_file) {
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
index e17e7b6992ee5..7fcf2c502ca6a 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
@@ -87,10 +87,6 @@ class PlatformAppleSimulator : public PlatformDarwin {
std::vector<ArchSpec>
GetSupportedArchitectures(const ArchSpec &process_host_arch) override;
- Status
- ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr) override;
-
Status GetSharedModule(const ModuleSpec &module_spec, Process *process,
lldb::ModuleSP &module_sp,
const FileSpecList *module_search_paths_ptr,
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp
index a244ee35976b6..9323b66181c7c 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp
@@ -63,71 +63,6 @@ void PlatformRemoteDarwinDevice::GetStatus(Stream &strm) {
}
}
-Status PlatformRemoteDarwinDevice::ResolveExecutable(
- const ModuleSpec &ms, lldb::ModuleSP &exe_module_sp,
- const FileSpecList *module_search_paths_ptr) {
- Status error;
- // Nothing special to do here, just use the actual file and architecture
-
- ModuleSpec resolved_module_spec(ms);
-
- // Resolve any executable within a bundle on MacOSX
- // TODO: verify that this handles shallow bundles, if not then implement one
- // ourselves
- Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
-
- if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec())) {
- if (resolved_module_spec.GetArchitecture().IsValid() ||
- resolved_module_spec.GetUUID().IsValid()) {
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- nullptr, nullptr, nullptr);
-
- if (exe_module_sp && exe_module_sp->GetObjectFile())
- return error;
- exe_module_sp.reset();
- }
- // No valid architecture was specified or the exact ARM slice wasn't found
- // so ask the platform for the architectures that we should be using (in
- // the correct order) and see if we can find a match that way
- StreamString arch_names;
- llvm::ListSeparator LS;
- ArchSpec process_host_arch;
- for (const ArchSpec &arch : GetSupportedArchitectures(process_host_arch)) {
- resolved_module_spec.GetArchitecture() = arch;
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- nullptr, nullptr, nullptr);
- // Did we find an executable using one of the
- if (error.Success()) {
- if (exe_module_sp && exe_module_sp->GetObjectFile())
- break;
- else
- error.SetErrorToGenericError();
- }
-
- arch_names << LS << arch.GetArchitectureName();
- }
-
- if (error.Fail() || !exe_module_sp) {
- if (FileSystem::Instance().Readable(resolved_module_spec.GetFileSpec())) {
- error.SetErrorStringWithFormatv(
- "'{0}' doesn't contain any '{1}' platform architectures: {2}",
- resolved_module_spec.GetFileSpec(), GetPluginName(),
- arch_names.GetData());
- } else {
- error.SetErrorStringWithFormat(
- "'%s' is not readable",
- resolved_module_spec.GetFileSpec().GetPath().c_str());
- }
- }
- } else {
- error.SetErrorStringWithFormat(
- "'%s' does not exist",
- resolved_module_spec.GetFileSpec().GetPath().c_str());
- }
-
- return error;
-}
-
bool PlatformRemoteDarwinDevice::GetFileInSDK(const char *platform_file_path,
uint32_t sdk_idx,
lldb_private::FileSpec &local_file) {
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h
index c604866040995..557f4876e91ab 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h
@@ -40,10 +40,6 @@ class PlatformRemoteDarwinDevice : public PlatformDarwinDevice {
~PlatformRemoteDarwinDevice() override;
// Platform functions
- Status
- ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr) override;
-
void GetStatus(Stream &strm) override;
virtual Status GetSymbolFile(const FileSpec &platform_file,
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index ee1f92470e162..b3116545b91d1 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -768,41 +768,6 @@ Platform::ResolveExecutable(const ModuleSpec &module_spec,
const FileSpecList *module_search_paths_ptr) {
Status error;
- if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
- if (module_spec.GetArchitecture().IsValid()) {
- error = ModuleList::GetSharedModule(module_spec, exe_module_sp,
- module_search_paths_ptr, nullptr,
- nullptr);
- } else {
- // No valid architecture was specified, ask the platform for the
- // architectures that we should be using (in the correct order) and see
- // if we can find a match that way
- ModuleSpec arch_module_spec(module_spec);
- ArchSpec process_host_arch;
- for (const ArchSpec &arch :
- GetSupportedArchitectures(process_host_arch)) {
- arch_module_spec.GetArchitecture() = arch;
- error = ModuleList::GetSharedModule(arch_module_spec, exe_module_sp,
- module_search_paths_ptr, nullptr,
- nullptr);
- // Did we find an executable using one of the
- if (error.Success() && exe_module_sp)
- break;
- }
- }
- } else {
- error.SetErrorStringWithFormat(
- "'%s' does not exist", module_spec.GetFileSpec().GetPath().c_str());
- }
- return error;
-}
-
-Status
-Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
- lldb::ModuleSP &exe_module_sp,
- const FileSpecList *module_search_paths_ptr) {
- Status error;
-
// We may connect to a process and use the provided executable (Don't use
// local $PATH).
ModuleSpec resolved_module_spec(module_spec);
@@ -822,9 +787,9 @@ Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
return error;
exe_module_sp.reset();
}
- // No valid architecture was specified or the exact arch wasn't found so
- // ask the platform for the architectures that we should be using (in the
- // correct order) and see if we can find a match that way
+ // No valid architecture was specified or the exact arch wasn't found.
+ // Ask the platform for the architectures that we should be using (in the
+ // correct order) and see if we can find a match that way.
StreamString arch_names;
llvm::ListSeparator LS;
ArchSpec process_host_arch;
@@ -833,12 +798,10 @@ Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
module_search_paths_ptr, nullptr,
nullptr);
- // Did we find an executable using one of the
if (error.Success()) {
if (exe_module_sp && exe_module_sp->GetObjectFile())
break;
- else
- error.SetErrorToGenericError();
+ error.SetErrorToGenericError();
}
arch_names << LS << arch.GetArchitectureName();
@@ -1516,8 +1479,7 @@ Platform::GetCachedExecutable(ModuleSpec &module_spec,
Status error = GetRemoteSharedModule(
module_spec, nullptr, module_sp,
[&](const ModuleSpec &spec) {
- return ResolveRemoteExecutable(spec, module_sp,
- module_search_paths_ptr);
+ return ResolveExecutable(spec, module_sp, module_search_paths_ptr);
},
nullptr);
if (error.Success()) {
diff --git a/lldb/source/Target/RemoteAwarePlatform.cpp b/lldb/source/Target/RemoteAwarePlatform.cpp
index 9a41a423cadd3..63243d6e71307 100644
--- a/lldb/source/Target/RemoteAwarePlatform.cpp
+++ b/lldb/source/Target/RemoteAwarePlatform.cpp
@@ -29,145 +29,6 @@ bool RemoteAwarePlatform::GetModuleSpec(const FileSpec &module_file_spec,
return false;
}
-Status RemoteAwarePlatform::ResolveExecutable(
- const ModuleSpec &module_spec, ModuleSP &exe_module_sp,
- const FileSpecList *module_search_paths_ptr) {
- Status error;
- // Nothing special to do here, just use the actual file and architecture
-
- char exe_path[PATH_MAX];
- ModuleSpec resolved_module_spec(module_spec);
-
- if (IsHost()) {
- // If we have "ls" as the exe_file, resolve the executable location based
- // on the current path variables
- if (!FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec())) {
- resolved_module_spec.GetFileSpec().GetPath(exe_path, sizeof(exe_path));
- resolved_module_spec.GetFileSpec().SetFile(exe_path,
- FileSpec::Style::native);
- FileSystem::Instance().Resolve(resolved_module_spec.GetFileSpec());
- }
-
- if (!FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
- FileSystem::Instance().ResolveExecutableLocation(
- resolved_module_spec.GetFileSpec());
-
- // Resolve any executable within a bundle on MacOSX
- Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
-
- if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
- error.Clear();
- else {
- const uint32_t permissions = FileSystem::Instance().GetPermissions(
- resolved_module_spec.GetFileSpec());
- if (permissions && (permissions & eFilePermissionsEveryoneR) == 0)
- error.SetErrorStringWithFormat(
- "executable '%s' is not readable",
- resolved_module_spec.GetFileSpec().GetPath().c_str());
- else
- error.SetErrorStringWithFormat(
- "unable to find executable for '%s'",
- resolved_module_spec.GetFileSpec().GetPath().c_str());
- }
- } else {
- if (m_remote_platform_sp) {
- return GetCachedExecutable(resolved_module_spec, exe_module_sp,
- module_search_paths_ptr);
- }
-
- // We may connect to a process and use the provided executable (Don't use
- // local $PATH).
-
- // Resolve any executable within a bundle on MacOSX
- Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
-
- if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
- error.Clear();
- else
- error.SetErrorStringWithFormat("the platform is not currently "
- "connected, and '%s' doesn't exist in "
- "the system root.",
- exe_path);
- }
-
- if (error.Success()) {
- if (resolved_module_spec.GetArchitecture().IsValid()) {
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- module_search_paths_ptr, nullptr, nullptr);
- if (error.Fail()) {
- // If we failed, it may be because the vendor and os aren't known. If
- // that is the case, try setting them to the host architecture and give
- // it another try.
- llvm::Triple &module_triple =
- resolved_module_spec.GetArchitecture().GetTriple();
- bool is_vendor_specified =
- (module_triple.getVendor() != llvm::Triple::UnknownVendor);
- bool is_os_specified =
- (module_triple.getOS() != llvm::Triple::UnknownOS);
- if (!is_vendor_specified || !is_os_specified) {
- const llvm::Triple &host_triple =
- HostInfo::GetArchitecture(HostInfo::eArchKindDefault).GetTriple();
-
- if (!is_vendor_specified)
- module_triple.setVendorName(host_triple.getVendorName());
- if (!is_os_specified)
- module_triple.setOSName(host_triple.getOSName());
-
- error = ModuleList::GetSharedModule(resolved_module_spec,
- exe_module_sp, module_search_paths_ptr, nullptr, nullptr);
- }
- }
-
- // TODO find out why exe_module_sp might be NULL
- if (error.Fail() || !exe_module_sp || !exe_module_sp->GetObjectFile()) {
- exe_module_sp.reset();
- error.SetErrorStringWithFormat(
- "'%s' doesn't contain the architecture %s",
- resolved_module_spec.GetFileSpec().GetPath().c_str(),
- resolved_module_spec.GetArchitecture().GetArchitectureName());
- }
- } else {
- // No valid architecture was specified, ask the platform for the
- // architectures that we should be using (in the correct order) and see
- // if we can find a match that way
- StreamString arch_names;
- llvm::ListSeparator LS;
- ArchSpec process_host_arch;
- for (const ArchSpec &arch :
- GetSupportedArchitectures(process_host_arch)) {
- resolved_module_spec.GetArchitecture() = arch;
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- module_search_paths_ptr, nullptr, nullptr);
- // Did we find an executable using one of the
- if (error.Success()) {
- if (exe_module_sp && exe_module_sp->GetObjectFile())
- break;
- else
- error.SetErrorToGenericError();
- }
-
- arch_names << LS << arch.GetArchitectureName();
- }
-
- if (error.Fail() || !exe_module_sp) {
- if (FileSystem::Instance().Readable(
- resolved_module_spec.GetFileSpec())) {
- error.SetErrorStringWithFormatv(
- "'{0}' doesn't contain any '{1}' platform architectures: {2}",
- resolved_module_spec.GetFileSpec(), GetPluginName(),
- arch_names.GetData());
- } else {
- error.SetErrorStringWithFormat(
- "'%s' is not readable",
- r...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/96256
More information about the lldb-commits
mailing list