[lldb] Unify Platform::ResolveExecutable #96256
Merged
+15
−348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThe Platform class currently has two functions to resolve an executable: To make things even more confusion, we had a similar but not identical implementation in RemoteAwarePlatform where its implementation of I went ahead and unified all the different implementation on the original 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:
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]
|
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.
JDevlieghere
force-pushed
the
platform-resolve-executable
branch
from
b94b23f
to
41f7b78
Compare
jasonmolenda
approved these changes
Jun 21, 2024
labath
approved these changes
Jun 21, 2024
lldb/include/lldb/Target/Platform.h
Outdated
| 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. |
There was a problem hiding this comment.
Which function is this comment referring to (if I'm reading the diff right, it's just floating there)?
There was a problem hiding this comment.
This comment is outdated. It was for ResolveRemoteExecutable which was virtual but only overridden in the unit test. I'll remove it.
JDevlieghere
added a commit
that referenced
this pull request
Jun 22, 2024
When unifying the ResolveExecutable implementations in #96256, I missed that RemoteAwarePlatform was able to resolve executables more aggressively. The host platform can rely on the current working directory to make relative paths absolute and resolve things like home directories. This should fix command-target-create-resolve-exe.test.
|
It seems this patch breaks the following tests in the cross build: Note the behavior is the same for Windows and Linux hosts with Linux AArch64 target. For example here is the log for lldb-api :: commands/process/attach/TestProcessAttach.py |
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
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.
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
When unifying the ResolveExecutable implementations in llvm#96256, I missed that RemoteAwarePlatform was able to resolve executables more aggressively. The host platform can rely on the current working directory to make relative paths absolute and resolve things like home directories. This should fix command-target-create-resolve-exe.test.
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
change the expected error msg.
splhack
added a commit
to splhack/llvm-project
that referenced
this pull request
Jul 12, 2024
This reverts commit bf3e328.
|
We think this broke Android debugging on our end. We are investigating. |
|
This did indeed break Android debugging. We reverted this in our repo and functionality was restored |
dzhidzhoev
added a commit
to dzhidzhoev/llvm-project
that referenced
this pull request
Jul 12, 2024
Seemingly, llvm#96256 removed the only call to Platform::GetCachedExecutable, which broke the resolution of executable modules in the remote debugging mode (llvm#97410). This commit fixes that.
dzhidzhoev
added a commit
that referenced
this pull request
Jul 12, 2024
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
Seemingly, llvm#96256 removed the only call to Platform::GetCachedExecutable, which broke the resolution of executable modules in the remote debugging mode (llvm#97410). This commit fixes that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Platform class currently has two functions to resolve an executable:
ResolveExecutableandResolveRemoteExecutable. 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
ResolveExecutablecould handle remote files. To top it all off, we had copy-pasted implementation, dead code included inPlatformAppleSimulatorandPlatformRemoteDarwinDevice.I went ahead and unified all the different implementation on the original
ResolveRemoteExecutableimplementation. 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.