Skip to content

Commit 41f7b78

Browse files
JDevlieghereJDevlieghere
committed
[lldb] Unify Platform::ResolveExecutable duplication
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.
1 parent f77ade0 commit 41f7b78

File tree

11 files changed

+16
-347
lines changed

11 files changed

+16
-347
lines changed

‎lldb/include/lldb/Target/Platform.h

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ class Platform : public PluginInterface {
139139
/// Returns \b true if this Platform plug-in was able to find
140140
/// a suitable executable, \b false otherwise.
141141
virtual Status ResolveExecutable(const ModuleSpec &module_spec,
142-
lldb::ModuleSP &module_sp,
142+
lldb::ModuleSP &exe_module_sp,
143143
const FileSpecList *module_search_paths_ptr);
144144

145145
/// Find a symbol file given a symbol file module specification.
@@ -1004,10 +1004,7 @@ class Platform : public PluginInterface {
10041004

10051005
virtual const char *GetCacheHostname();
10061006

1007-
virtual Status
1008-
ResolveRemoteExecutable(const ModuleSpec &module_spec,
1009-
lldb::ModuleSP &exe_module_sp,
1010-
const FileSpecList *module_search_paths_ptr);
1007+
/// The method is virtual for mocking in the unit tests.
10111008

10121009
private:
10131010
typedef std::function<Status(const ModuleSpec &)> ModuleResolver;

‎lldb/include/lldb/Target/RemoteAwarePlatform.h

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,6 @@ class RemoteAwarePlatform : public Platform {
2323
bool GetModuleSpec(const FileSpec &module_file_spec, const ArchSpec &arch,
2424
ModuleSpec &module_spec) override;
2525

26-
Status
27-
ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
28-
const FileSpecList *module_search_paths_ptr) override;
29-
3026
lldb::user_id_t OpenFile(const FileSpec &file_spec, File::OpenOptions flags,
3127
uint32_t mode, Status &error) override;
3228

‎lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp

Lines changed: 0 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -383,81 +383,6 @@ PlatformSP PlatformAppleSimulator::CreateInstance(
383383
return PlatformSP();
384384
}
385385

386-
Status PlatformAppleSimulator::ResolveExecutable(
387-
const ModuleSpec &module_spec, lldb::ModuleSP &exe_module_sp,
388-
const FileSpecList *module_search_paths_ptr) {
389-
Status error;
390-
// Nothing special to do here, just use the actual file and architecture
391-
392-
ModuleSpec resolved_module_spec(module_spec);
393-
394-
// If we have "ls" as the exe_file, resolve the executable loation based on
395-
// the current path variables
396-
// TODO: resolve bare executables in the Platform SDK
397-
// if (!resolved_exe_file.Exists())
398-
// resolved_exe_file.ResolveExecutableLocation ();
399-
400-
// Resolve any executable within a bundle on MacOSX
401-
// TODO: verify that this handles shallow bundles, if not then implement one
402-
// ourselves
403-
Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
404-
405-
if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec())) {
406-
if (resolved_module_spec.GetArchitecture().IsValid()) {
407-
error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
408-
NULL, NULL, NULL);
409-
410-
if (exe_module_sp && exe_module_sp->GetObjectFile())
411-
return error;
412-
exe_module_sp.reset();
413-
}
414-
// No valid architecture was specified or the exact ARM slice wasn't found
415-
// so ask the platform for the architectures that we should be using (in
416-
// the correct order) and see if we can find a match that way
417-
StreamString arch_names;
418-
llvm::ListSeparator LS;
419-
ArchSpec platform_arch;
420-
for (const ArchSpec &arch : GetSupportedArchitectures({})) {
421-
resolved_module_spec.GetArchitecture() = arch;
422-
423-
// Only match x86 with x86 and x86_64 with x86_64...
424-
if (!module_spec.GetArchitecture().IsValid() ||
425-
module_spec.GetArchitecture().GetCore() ==
426-
resolved_module_spec.GetArchitecture().GetCore()) {
427-
error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
428-
NULL, NULL, NULL);
429-
// Did we find an executable using one of the
430-
if (error.Success()) {
431-
if (exe_module_sp && exe_module_sp->GetObjectFile())
432-
break;
433-
else
434-
error.SetErrorToGenericError();
435-
}
436-
437-
arch_names << LS << platform_arch.GetArchitectureName();
438-
}
439-
}
440-
441-
if (error.Fail() || !exe_module_sp) {
442-
if (FileSystem::Instance().Readable(resolved_module_spec.GetFileSpec())) {
443-
error.SetErrorStringWithFormatv(
444-
"'{0}' doesn't contain any '{1}' platform architectures: {2}",
445-
resolved_module_spec.GetFileSpec(), GetPluginName(),
446-
arch_names.GetString());
447-
} else {
448-
error.SetErrorStringWithFormat(
449-
"'%s' is not readable",
450-
resolved_module_spec.GetFileSpec().GetPath().c_str());
451-
}
452-
}
453-
} else {
454-
error.SetErrorStringWithFormat("'%s' does not exist",
455-
module_spec.GetFileSpec().GetPath().c_str());
456-
}
457-
458-
return error;
459-
}
460-
461386
Status PlatformAppleSimulator::GetSymbolFile(const FileSpec &platform_file,
462387
const UUID *uuid_ptr,
463388
FileSpec &local_file) {

‎lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,6 @@ class PlatformAppleSimulator : public PlatformDarwin {
8787
std::vector<ArchSpec>
8888
GetSupportedArchitectures(const ArchSpec &process_host_arch) override;
8989

90-
Status
91-
ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
92-
const FileSpecList *module_search_paths_ptr) override;
93-
9490
Status GetSharedModule(const ModuleSpec &module_spec, Process *process,
9591
lldb::ModuleSP &module_sp,
9692
const FileSpecList *module_search_paths_ptr,

‎lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp

Lines changed: 0 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -63,71 +63,6 @@ void PlatformRemoteDarwinDevice::GetStatus(Stream &strm) {
6363
}
6464
}
6565

66-
Status PlatformRemoteDarwinDevice::ResolveExecutable(
67-
const ModuleSpec &ms, lldb::ModuleSP &exe_module_sp,
68-
const FileSpecList *module_search_paths_ptr) {
69-
Status error;
70-
// Nothing special to do here, just use the actual file and architecture
71-
72-
ModuleSpec resolved_module_spec(ms);
73-
74-
// Resolve any executable within a bundle on MacOSX
75-
// TODO: verify that this handles shallow bundles, if not then implement one
76-
// ourselves
77-
Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
78-
79-
if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec())) {
80-
if (resolved_module_spec.GetArchitecture().IsValid() ||
81-
resolved_module_spec.GetUUID().IsValid()) {
82-
error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
83-
nullptr, nullptr, nullptr);
84-
85-
if (exe_module_sp && exe_module_sp->GetObjectFile())
86-
return error;
87-
exe_module_sp.reset();
88-
}
89-
// No valid architecture was specified or the exact ARM slice wasn't found
90-
// so ask the platform for the architectures that we should be using (in
91-
// the correct order) and see if we can find a match that way
92-
StreamString arch_names;
93-
llvm::ListSeparator LS;
94-
ArchSpec process_host_arch;
95-
for (const ArchSpec &arch : GetSupportedArchitectures(process_host_arch)) {
96-
resolved_module_spec.GetArchitecture() = arch;
97-
error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
98-
nullptr, nullptr, nullptr);
99-
// Did we find an executable using one of the
100-
if (error.Success()) {
101-
if (exe_module_sp && exe_module_sp->GetObjectFile())
102-
break;
103-
else
104-
error.SetErrorToGenericError();
105-
}
106-
107-
arch_names << LS << arch.GetArchitectureName();
108-
}
109-
110-
if (error.Fail() || !exe_module_sp) {
111-
if (FileSystem::Instance().Readable(resolved_module_spec.GetFileSpec())) {
112-
error.SetErrorStringWithFormatv(
113-
"'{0}' doesn't contain any '{1}' platform architectures: {2}",
114-
resolved_module_spec.GetFileSpec(), GetPluginName(),
115-
arch_names.GetData());
116-
} else {
117-
error.SetErrorStringWithFormat(
118-
"'%s' is not readable",
119-
resolved_module_spec.GetFileSpec().GetPath().c_str());
120-
}
121-
}
122-
} else {
123-
error.SetErrorStringWithFormat(
124-
"'%s' does not exist",
125-
resolved_module_spec.GetFileSpec().GetPath().c_str());
126-
}
127-
128-
return error;
129-
}
130-
13166
bool PlatformRemoteDarwinDevice::GetFileInSDK(const char *platform_file_path,
13267
uint32_t sdk_idx,
13368
lldb_private::FileSpec &local_file) {

‎lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,6 @@ class PlatformRemoteDarwinDevice : public PlatformDarwinDevice {
4040
~PlatformRemoteDarwinDevice() override;
4141

4242
// Platform functions
43-
Status
44-
ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
45-
const FileSpecList *module_search_paths_ptr) override;
46-
4743
void GetStatus(Stream &strm) override;
4844

4945
virtual Status GetSymbolFile(const FileSpec &platform_file,

‎lldb/source/Target/Platform.cpp

Lines changed: 5 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -768,41 +768,6 @@ Platform::ResolveExecutable(const ModuleSpec &module_spec,
768768
const FileSpecList *module_search_paths_ptr) {
769769
Status error;
770770

771-
if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
772-
if (module_spec.GetArchitecture().IsValid()) {
773-
error = ModuleList::GetSharedModule(module_spec, exe_module_sp,
774-
module_search_paths_ptr, nullptr,
775-
nullptr);
776-
} else {
777-
// No valid architecture was specified, ask the platform for the
778-
// architectures that we should be using (in the correct order) and see
779-
// if we can find a match that way
780-
ModuleSpec arch_module_spec(module_spec);
781-
ArchSpec process_host_arch;
782-
for (const ArchSpec &arch :
783-
GetSupportedArchitectures(process_host_arch)) {
784-
arch_module_spec.GetArchitecture() = arch;
785-
error = ModuleList::GetSharedModule(arch_module_spec, exe_module_sp,
786-
module_search_paths_ptr, nullptr,
787-
nullptr);
788-
// Did we find an executable using one of the
789-
if (error.Success() && exe_module_sp)
790-
break;
791-
}
792-
}
793-
} else {
794-
error.SetErrorStringWithFormat(
795-
"'%s' does not exist", module_spec.GetFileSpec().GetPath().c_str());
796-
}
797-
return error;
798-
}
799-
800-
Status
801-
Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
802-
lldb::ModuleSP &exe_module_sp,
803-
const FileSpecList *module_search_paths_ptr) {
804-
Status error;
805-
806771
// We may connect to a process and use the provided executable (Don't use
807772
// local $PATH).
808773
ModuleSpec resolved_module_spec(module_spec);
@@ -822,9 +787,9 @@ Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
822787
return error;
823788
exe_module_sp.reset();
824789
}
825-
// No valid architecture was specified or the exact arch wasn't found so
826-
// ask the platform for the architectures that we should be using (in the
827-
// correct order) and see if we can find a match that way
790+
// No valid architecture was specified or the exact arch wasn't found.
791+
// Ask the platform for the architectures that we should be using (in the
792+
// correct order) and see if we can find a match that way.
828793
StreamString arch_names;
829794
llvm::ListSeparator LS;
830795
ArchSpec process_host_arch;
@@ -833,12 +798,10 @@ Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
833798
error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
834799
module_search_paths_ptr, nullptr,
835800
nullptr);
836-
// Did we find an executable using one of the
837801
if (error.Success()) {
838802
if (exe_module_sp && exe_module_sp->GetObjectFile())
839803
break;
840-
else
841-
error.SetErrorToGenericError();
804+
error.SetErrorToGenericError();
842805
}
843806

844807
arch_names << LS << arch.GetArchitectureName();
@@ -1516,8 +1479,7 @@ Platform::GetCachedExecutable(ModuleSpec &module_spec,
15161479
Status error = GetRemoteSharedModule(
15171480
module_spec, nullptr, module_sp,
15181481
[&](const ModuleSpec &spec) {
1519-
return ResolveRemoteExecutable(spec, module_sp,
1520-
module_search_paths_ptr);
1482+
return ResolveExecutable(spec, module_sp, module_search_paths_ptr);
15211483
},
15221484
nullptr);
15231485
if (error.Success()) {

0 commit comments

Comments
 (0)