JDevlieghere created this revision. JDevlieghere added reviewers: labath, jingham, clayborg, jasonmolenda. Herald added subscribers: mgrang, mgorny. Herald added a project: All. JDevlieghere requested review of this revision.
Currently we error out if multiple platforms can supports the architectures in a fat binary. For example, on Darwin, almost every architecture supported by the host platform is also supported by the remote-macosx platform. Currently, the only way to disambiguate between the two is to specify the desired architecture. This patch changes the behavior to account for the selected and host platform. The new algorithm to pick a platform works as follows: 1. Prefer the selected platform if it supports all architectures in the binary, 2. Prefer the host platform if it supports all architectures in the binary, 3. Prefer the platform that supports all architectures in the binary, 4. Prefer the selected platform if it supports one architecture in the binary, 5. Prefer the host platform if it supports one architecture in the binary, 6. If none of the above apply, error out saying either no or multiple platforms support the architectures in the binary I've added a bunch of unit tests to codify this new behavior. https://reviews.llvm.org/D122684 Files: lldb/include/lldb/Target/Platform.h lldb/source/Target/Platform.cpp lldb/source/Target/TargetList.cpp lldb/unittests/Platform/CMakeLists.txt lldb/unittests/Platform/PlatformTest.cpp
Index: lldb/unittests/Platform/PlatformTest.cpp =================================================================== --- /dev/null +++ lldb/unittests/Platform/PlatformTest.cpp @@ -0,0 +1,162 @@ +//===-- PlatformTest.cpp ------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "gtest/gtest.h" + +#include "Plugins/Platform/POSIX/PlatformPOSIX.h" +#include "TestingSupport/SubsystemRAII.h" +#include "lldb/Core/PluginManager.h" +#include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Target/Platform.h" + +using namespace lldb; +using namespace lldb_private; + +class TestPlatform : public PlatformPOSIX { +public: + TestPlatform() : PlatformPOSIX(false) {} + using Platform::Clear; +}; + +class PlatformArm : public TestPlatform { +public: + PlatformArm() = default; + + std::vector<ArchSpec> + GetSupportedArchitectures(const ArchSpec &process_host_arch) override { + return {ArchSpec("arm64-apple-ps4")}; + } + + llvm::StringRef GetPluginName() override { return "arm"; } + llvm::StringRef GetDescription() override { return "arm"; } +}; + +class PlatformIntel : public TestPlatform { +public: + PlatformIntel() = default; + + std::vector<ArchSpec> + GetSupportedArchitectures(const ArchSpec &process_host_arch) override { + return {ArchSpec("x86_64-apple-ps4")}; + } + + llvm::StringRef GetPluginName() override { return "intel"; } + llvm::StringRef GetDescription() override { return "intel"; } +}; + +class PlatformThumb : public TestPlatform { +public: + static void Initialize() { + PluginManager::RegisterPlugin("thumb", "thumb", + PlatformThumb::CreateInstance); + } + static void Terminate() { + PluginManager::UnregisterPlugin(PlatformThumb::CreateInstance); + } + + static PlatformSP CreateInstance(bool force, const ArchSpec *arch) { + return std::make_shared<PlatformThumb>(); + } + + std::vector<ArchSpec> + GetSupportedArchitectures(const ArchSpec &process_host_arch) override { + return {ArchSpec("thumbv7-apple-ps4"), ArchSpec("thumbv7f-apple-ps4")}; + } + + llvm::StringRef GetPluginName() override { return "thumb"; } + llvm::StringRef GetDescription() override { return "thumb"; } +}; + +class PlatformTest : public ::testing::Test { + SubsystemRAII<FileSystem, HostInfo> subsystems; + void SetUp() override { TestPlatform::Clear(); } +}; + +TEST_F(PlatformTest, GetPlatformForArchitecturesHost) { + const PlatformSP host_platform_sp = std::make_shared<PlatformArm>(); + Platform::SetHostPlatform(host_platform_sp); + ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp); + + const std::vector<ArchSpec> archs = {ArchSpec("arm64-apple-ps4"), + ArchSpec("arm64e-apple-ps4")}; + std::vector<PlatformSP> candidates; + + // The host platform matches all architectures. + PlatformSP platform_sp = + Platform::GetPlatformForArchitectures(archs, {}, {}, candidates); + ASSERT_TRUE(platform_sp); + EXPECT_EQ(platform_sp, host_platform_sp); +} + +TEST_F(PlatformTest, GetPlatformForArchitecturesSelected) { + const PlatformSP host_platform_sp = std::make_shared<PlatformIntel>(); + Platform::SetHostPlatform(host_platform_sp); + ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp); + + const std::vector<ArchSpec> archs = {ArchSpec("arm64-apple-ps4"), + ArchSpec("arm64e-apple-ps4")}; + std::vector<PlatformSP> candidates; + + // The host platform matches no architectures. No selected platform. + PlatformSP platform_sp = + Platform::GetPlatformForArchitectures(archs, {}, {}, candidates); + ASSERT_FALSE(platform_sp); + + // The selected platform matches all architectures. + const PlatformSP selected_platform_sp = std::make_shared<PlatformArm>(); + platform_sp = Platform::GetPlatformForArchitectures( + archs, {}, selected_platform_sp, candidates); + ASSERT_TRUE(platform_sp); + EXPECT_EQ(platform_sp, selected_platform_sp); +} + +TEST_F(PlatformTest, GetPlatformForArchitecturesSelectedOverHost) { + const PlatformSP host_platform_sp = std::make_shared<PlatformIntel>(); + Platform::SetHostPlatform(host_platform_sp); + ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp); + + const std::vector<ArchSpec> archs = {ArchSpec("arm64-apple-ps4"), + ArchSpec("x86_64-apple-ps4")}; + std::vector<PlatformSP> candidates; + + // The host platform matches one architecture. No selected platform. + PlatformSP platform_sp = + Platform::GetPlatformForArchitectures(archs, {}, {}, candidates); + ASSERT_TRUE(platform_sp); + EXPECT_EQ(platform_sp, host_platform_sp); + + // The selected and host platform each match one architecture. + // The selected platform is preferred. + const PlatformSP selected_platform_sp = std::make_shared<PlatformArm>(); + platform_sp = Platform::GetPlatformForArchitectures( + archs, {}, selected_platform_sp, candidates); + ASSERT_TRUE(platform_sp); + EXPECT_EQ(platform_sp, selected_platform_sp); +} + +TEST_F(PlatformTest, GetPlatformForArchitecturesCandidates) { + PlatformThumb::Initialize(); + + const PlatformSP host_platform_sp = std::make_shared<PlatformIntel>(); + Platform::SetHostPlatform(host_platform_sp); + ASSERT_EQ(Platform::GetHostPlatform(), host_platform_sp); + const PlatformSP selected_platform_sp = std::make_shared<PlatformArm>(); + + const std::vector<ArchSpec> archs = {ArchSpec("thumbv7-apple-ps4"), + ArchSpec("thumbv7f-apple-ps4")}; + std::vector<PlatformSP> candidates; + + // The host platform matches one architecture. No selected platform. + PlatformSP platform_sp = Platform::GetPlatformForArchitectures( + archs, {}, selected_platform_sp, candidates); + ASSERT_TRUE(platform_sp); + EXPECT_EQ(platform_sp->GetName(), "thumb"); + + PlatformThumb::Terminate(); +} Index: lldb/unittests/Platform/CMakeLists.txt =================================================================== --- lldb/unittests/Platform/CMakeLists.txt +++ lldb/unittests/Platform/CMakeLists.txt @@ -3,6 +3,7 @@ PlatformDarwinTest.cpp PlatformMacOSXTest.cpp PlatformSiginfoTest.cpp + PlatformTest.cpp LINK_LIBS lldbPluginPlatformFreeBSD Index: lldb/source/Target/TargetList.cpp =================================================================== --- lldb/source/Target/TargetList.cpp +++ lldb/source/Target/TargetList.cpp @@ -171,80 +171,31 @@ } else { // Fat binary. No architecture specified, check if there is // only one platform for all of the architectures. - PlatformSP host_platform_sp = Platform::GetHostPlatform(); - std::vector<PlatformSP> platforms; - for (size_t i = 0; i < num_specs; ++i) { - ModuleSpec module_spec; - if (module_specs.GetModuleSpecAtIndex(i, module_spec)) { - // First consider the platform specified by the user, if any, and - // the selected platform otherwise. - if (platform_sp) { - if (platform_sp->IsCompatibleArchitecture( - module_spec.GetArchitecture(), {}, false, nullptr)) { - platforms.push_back(platform_sp); - continue; - } - } - - // Now consider the host platform if it is different from the - // specified/selected platform. - if (host_platform_sp && - (!platform_sp || - host_platform_sp->GetName() != platform_sp->GetName())) { - if (host_platform_sp->IsCompatibleArchitecture( - module_spec.GetArchitecture(), {}, false, nullptr)) { - platforms.push_back(host_platform_sp); - continue; - } - } - - // Finally find a platform that matches the architecture in the - // executable file. - PlatformSP fallback_platform_sp( - Platform::GetPlatformForArchitecture( - module_spec.GetArchitecture())); - if (fallback_platform_sp) { - platforms.push_back(fallback_platform_sp); - } - } - } - - Platform *platform_ptr = nullptr; - bool more_than_one_platforms = false; - for (const auto &the_platform_sp : platforms) { - if (platform_ptr) { - if (platform_ptr->GetName() != the_platform_sp->GetName()) { - more_than_one_platforms = true; - platform_ptr = nullptr; - break; - } - } else { - platform_ptr = the_platform_sp.get(); - } - } - - if (platform_ptr) { - // All platforms for all modules in the executable match, so we can - // select this platform. - platform_sp = platforms.front(); - } else if (!more_than_one_platforms) { - // No platforms claim to support this file. + std::vector<PlatformSP> candidates; + std::vector<ArchSpec> archs; + for (const ModuleSpec &spec : module_specs.ModuleSpecs()) + archs.push_back(spec.GetArchitecture()); + if (PlatformSP platform_for_archs_sp = + Platform::GetPlatformForArchitectures(archs, {}, platform_sp, + candidates)) { + platform_sp = platform_for_archs_sp; + } else if (candidates.empty()) { error.SetErrorString("no matching platforms found for this file"); return error; } else { // More than one platform claims to support this file. StreamString error_strm; - std::set<Platform *> platform_set; + std::set<llvm::StringRef> platform_set; error_strm.Printf( "more than one platform supports this executable ("); - for (const auto &the_platform_sp : platforms) { - if (platform_set.find(the_platform_sp.get()) == - platform_set.end()) { - if (!platform_set.empty()) - error_strm.PutCString(", "); - error_strm.PutCString(the_platform_sp->GetName()); - platform_set.insert(the_platform_sp.get()); - } + for (const auto &candidate : candidates) { + llvm::StringRef platform_name = candidate->GetName(); + if (platform_set.count(platform_name)) + continue; + if (!platform_set.empty()) + error_strm.PutCString(", "); + error_strm.PutCString(platform_name); + platform_set.insert(platform_name); } error_strm.Printf("), specify an architecture to disambiguate"); error.SetErrorString(error_strm.GetString()); Index: lldb/source/Target/Platform.cpp =================================================================== --- lldb/source/Target/Platform.cpp +++ lldb/source/Target/Platform.cpp @@ -38,6 +38,7 @@ #include "lldb/Utility/Log.h" #include "lldb/Utility/Status.h" #include "lldb/Utility/StructuredData.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Path.h" @@ -156,6 +157,8 @@ } } +void Platform::Clear() { GetPlatformList().clear(); } + PlatformProperties &Platform::GetGlobalPlatformProperties() { static PlatformProperties g_settings; return g_settings; @@ -1216,6 +1219,100 @@ return platform_sp; } +void remove_duplicates(std::vector<lldb::PlatformSP> &platforms) { + llvm::sort(platforms.begin(), platforms.end(), + [](const PlatformSP &lhs, const PlatformSP &rhs) { + return lhs->GetName() < rhs->GetName(); + }); + platforms.erase(std::unique(platforms.begin(), platforms.end()), + platforms.end()); +} + +lldb::PlatformSP Platform::GetPlatformForArchitectures( + std::vector<ArchSpec> archs, const ArchSpec &process_host_arch, + lldb::PlatformSP selected_platform_sp, + std::vector<lldb::PlatformSP> &candidates) { + candidates.clear(); + candidates.reserve(archs.size()); + + if (archs.empty()) + return nullptr; + + PlatformSP host_platform_sp = Platform::GetHostPlatform(); + + bool host_platform_matches_one_arch = false; + bool selected_platform_matches_one_arch = false; + + bool host_platform_matches_all_archs = static_cast<bool>(host_platform_sp); + bool selected_platform_matches_all_archs = + static_cast<bool>(selected_platform_sp); + + for (const ArchSpec &arch : archs) { + if (selected_platform_sp) { + if (selected_platform_sp->IsCompatibleArchitecture( + arch, process_host_arch, false, nullptr)) { + selected_platform_matches_one_arch = true; + selected_platform_matches_all_archs &= true; + } else { + selected_platform_matches_all_archs = false; + } + } + + if (host_platform_sp) { + if (host_platform_sp->IsCompatibleArchitecture(arch, process_host_arch, + false, nullptr)) { + host_platform_matches_one_arch = true; + host_platform_matches_all_archs &= true; + } else { + host_platform_matches_all_archs = false; + } + } + + if (PlatformSP platform = + Platform::GetPlatformForArchitecture(arch, process_host_arch)) { + candidates.push_back(platform); + } + } + + // Prefer the selected platform if it matches all architectures. + if (selected_platform_matches_all_archs) + return selected_platform_sp; + + // Prefer the host platform if it matches all architectures. + if (host_platform_matches_all_archs) + return host_platform_sp; + + // If there's only one platform left then that means that means that it + // supports all architectures. + if (candidates.size() == archs.size()) { + if (std::all_of(candidates.begin(), candidates.end(), + [&](const PlatformSP &p) -> bool { + return p->GetName() == candidates.front()->GetName(); + })) { + return candidates.front(); + } + } + + // Prefer the selected platform if it matches at least on architecture. + if (selected_platform_matches_one_arch) + return selected_platform_sp; + + // Prefer the host platform if it matches at least on architecture. + if (host_platform_matches_one_arch) + return host_platform_sp; + + // At this point there's nothing left to differentiate the candidates. Add + // the selected platform and the host platform to the list of candidates if + // they matched at least on architecture. + if (selected_platform_matches_one_arch) + candidates.push_back(selected_platform_sp); + + if (host_platform_matches_one_arch) + candidates.push_back(host_platform_sp); + + return nullptr; +} + std::vector<ArchSpec> Platform::CreateArchList(llvm::ArrayRef<llvm::Triple::ArchType> archs, llvm::Triple::OSType os) { Index: lldb/include/lldb/Target/Platform.h =================================================================== --- lldb/include/lldb/Target/Platform.h +++ lldb/include/lldb/Target/Platform.h @@ -99,6 +99,12 @@ const ArchSpec &process_host_arch = {}, ArchSpec *platform_arch_ptr = nullptr); + static lldb::PlatformSP + GetPlatformForArchitectures(std::vector<ArchSpec> archs, + const ArchSpec &process_host_arch, + lldb::PlatformSP selected_platform_sp, + std::vector<lldb::PlatformSP> &candidates); + static const char *GetHostPlatformName(); static void SetHostPlatform(const lldb::PlatformSP &platform_sp); @@ -871,6 +877,9 @@ virtual CompilerType GetSiginfoType(const llvm::Triple &triple); protected: + /// For unit testing purposes only. + static void Clear(); + /// Create a list of ArchSpecs with the given OS and a architectures. The /// vendor field is left as an "unspecified unknown". static std::vector<ArchSpec>
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
