[lldb] Use the selected and host platform to disambiguate between fat binary architectures

[JDevlieghere]

Summary

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 works a follows:

1. Returns the selected platform if it matches any of the architectures.
2. Returns the host platform if it matches any of the architectures.
3. Returns the platform that matches all the architectures.

If none of the above apply, this function returns a default platform. The candidates output argument differentiates between either no platforms supporting the given architecture or multiple platforms supporting the given architecture.

I've added a bunch of unit tests to codify this new behavior.

rdar://90360204

[clayborg]

So this will fix LLDB complaining when a binary contains arm64 and x86_64 when debugging on either platform? Great, we ran into this quite a bit lately!

lldb/source/Target/Platform.cpp

1277–1279

Shouldn't we just check if the selected platform matches one architecture here and return the selected platform? Why do all architectures need to match. If I select a platform, and then do "file a.out" and "a.out" contained any number of architectures, it should just use the selected platform, no?

1281–1283

Again, why would we not just return the host platform if one arch matches? same kind of thing as above?

1285–1286

Is this comment out of date? We are iterating through more than one platform here.

1287–1294

What is this doing? Comparing the first entry in candidates to each of the entries and if the name matches, returns the first entry? When would this ever not return true?

1296

1300

[aprantl]

lldb/include/lldb/Target/Platform.h

102

Would you mind adding a doxygen comment for this? I'm asking because the semantics are not clear to me just from looking at the function signature.

lldb/source/Target/Platform.cpp

1228

would std::unique be more idiomatic?

lldb/unittests/Platform/PlatformTest.cpp

2

super-nit: too few -s :-)

[JDevlieghere]

lldb/source/Target/Platform.cpp

1277–1279

That's a good question. I did this to mimic what we're doing today, i.e. preferring the platform that matches all architectures in the fat binary.

I was trying to imagine what that could look like. One possible scenario is a fat binary with triples arm64-apple-macosx and arm64-apple-ios. In this case, both the host platform supports arm64-apple-macosx and arm64-apple-ios (https://support.apple.com/guide/app-store/iphone-ipad-apps-mac-apple-silicon-fird2c7092da/mac). And remote-ios supports arm64-apple-ios. If the binary is fat like that, it's more than likely meant to run on macosx, so with this algorithm, we'd do the right thing.

The second reason is that the order of the architectures is pretty arbitrary. Let's consider the arm64-apple-macosx and arm64-apple-ios example again. If we say that we'll pick whichever matches first, then we'll pick the host platform. But if the order was arm64-apple-ios and arm64-apple-macosx then we'll pick remote-ios.

Anyway I'm not married to this approach. I personally think that picking the selected platform is one host matches makes sense. I'm less sure about the host platform.

1281–1283

See my reply above.

1285–1286

Yup, I was uniquing the platforms originally.

1287–1294

This checks that all the platforms are identical. If they're all identical then that means that we have one platform that works for all architectures. This covers case (3) from the summary.

[aprantl]

lldb/source/Target/Platform.cpp

1228

Not sure why I didn't see this.

[JDevlieghere]

In D122684#3415303, @clayborg wrote:

So this will fix LLDB complaining when a binary contains arm64 and x86_64 when debugging on either platform? Great, we ran into this quite a bit lately!

Yes, exactly. The issue is was introduced by https://reviews.llvm.org/D113159. Previously, IsHost() would be false for remote-macosx and the OSNAME would expand to ios returning absolutely bogus triples. But my change fixed that, causing the remote-macosx platform to be a real contender, and resulting in an error message:

error: more than one platform supports this executable (host, remote-macosx), specify an architecture to disambiguate

[clayborg]

lldb/include/lldb/Target/Platform.h

103

lldb/source/Target/Platform.cpp

1277–1279

I agree, so maybe lets switch to say if the selected platform matches at least one, then it is all good. Will the selected platform ever be the host platform?

1287–1294

Sounds good, just update the comment then and all good.

[JDevlieghere]

lldb/source/Target/Platform.cpp

1277–1279

I believe the selected platform is the host platform if you didn't specify it explicitly.

[JDevlieghere]

Change the algorithm to pick the selected/host platform if it matches at least one architecture.

[clayborg]

Looks good. One nit that you can choose to fix if desired

lldb/source/Target/Platform.cpp

1229–1230

Move this above line 1227?

[labath]

Just this morning, I was (again) contemplating the idea of introducing some sort of numeric platform priorities. (We currently have a sort of a ternary system -- exact match, compatible match, no match). That would allow us to express pretty much arbitrary relationships ("I know this binary looks like it could be run on the host, but I _really_ want to run it through my super fancy platform").

Nevertheless, the current implementation and the priorities set within it look reasonable to me.

[jingham]

lldb/source/Target/Platform.cpp

1252

Why are you passing process_host_arch here? This is the "selected_platform" so you have no way of knowing a priori that this is the host platform or has the same architecture as the host system. In the old version, this selected platform part of the processing passed {} instead of the process_host_arch, which seems more correct.

[jingham]

lldb/source/Target/Platform.cpp

1252

Note, the old code made what seems like the opposite mistake, and DIDN'T pass process_host_arch in the Host Platform section of the code.

[JDevlieghere]

lldb/source/Target/Platform.cpp

1252

The process_host_arch argument was added to Platform::IsCompatibleArchitecture for https://reviews.llvm.org/rGc22c7a61b6d9c90d5d4292205c63cd576f4fd05b. You're correct that we don't have a process host arch from where this function is being called. So I could have omitted it, but I decided not to for consistency with Platform::GetPlatformForArchitecture just above.

[JDevlieghere]

lldb/source/Target/Platform.cpp

1252

The new code in TargetList.cpp:179 still passes {} to GetPlatformForArchitectures as the process host arch.

lldb/include/lldb/Target/Platform.h

public:
   /// attaching to processes unless another platform is specified.
   static lldb::PlatformSP GetHostPlatform();
 
   static lldb::PlatformSP
   GetPlatformForArchitecture(const ArchSpec &arch,
                              const ArchSpec &process_host_arch = {},
                              ArchSpec *platform_arch_ptr = nullptr);
 
+  /// Get the platform for the given list of architectures.
+  ///
+  /// The algorithm works a follows:
+  ///
+  /// 1. Returns the selected platform if it matches any of the architectures.
+  /// 2. Returns the host platform if it matches any of the architectures.
+  /// 3. Returns the platform that matches all the architectures.
+  ///
+  /// If none of the above apply, this function returns a default platform. The
+  /// candidates output argument differentiates between either no platforms
+  /// supporting the given architecture or multiple platforms supporting the
+  /// given architecture.
+  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);
 
   // Find an existing platform plug-in by name
   static lldb::PlatformSP Find(ConstString name);
 
   static lldb::PlatformSP Create(ConstString name, Status &error);
public:
   virtual llvm::Expected<StructuredData::DictionarySP>
   FetchExtendedCrashInformation(lldb_private::Process &process) {
     return nullptr;
   }
 
   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>
   CreateArchList(llvm::ArrayRef<llvm::Triple::ArchType> archs,
                  llvm::Triple::OSType os);
 
   /// Private implementation of connecting to a process. If the stream is set
   /// we connect synchronously.

lldb/source/Target/Platform.cpp

 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/LLDBLog.h"
 #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"
 
 // Define these constants from POSIX mman.h rather than include the file so
 // that they will be correct even when compiled on Linux.
 #define MAP_PRIVATE 2
 #define MAP_ANON 0x1000
 
void Platform::Terminate() {
   if (g_initialize_count > 0) {
     if (--g_initialize_count == 0) {
       std::lock_guard<std::recursive_mutex> guard(GetPlatformListMutex());
       GetPlatformList().clear();
     }
   }
 }
 
+void Platform::Clear() { GetPlatformList().clear(); }
+
 PlatformProperties &Platform::GetGlobalPlatformProperties() {
   static PlatformProperties g_settings;
   return g_settings;
 }
 
 void Platform::SetHostPlatform(const lldb::PlatformSP &platform_sp) {
   // The native platform should use its static void Platform::Initialize()
   // function to register itself as the native platform.
Platform::GetPlatformForArchitecture(const ArchSpec &arch,
   lldb::PlatformSP platform_sp;
   Status error;
   if (arch.IsValid())
     platform_sp =
         Platform::Create(arch, process_host_arch, platform_arch_ptr, error);
   return platform_sp;
 }
 
+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();
+
+  // Prefer the selected platform if it matches at least one architecture.
+  if (selected_platform_sp) {
+    for (const ArchSpec &arch : archs) {
+      if (selected_platform_sp->IsCompatibleArchitecture(
+              arch, process_host_arch, false, nullptr))
+        return selected_platform_sp;
+    }
+  }
+
+  // Prefer the host platform if it matches at least one architecture.
+  if (host_platform_sp) {
+    for (const ArchSpec &arch : archs) {
+      if (host_platform_sp->IsCompatibleArchitecture(arch, process_host_arch,
+                                                     false, nullptr))
+        return host_platform_sp;
+    }
+  }
+
+  // Collect a list of candidate platforms for the architectures.
+  for (const ArchSpec &arch : archs) {
+    if (PlatformSP platform =
+            Platform::GetPlatformForArchitecture(arch, process_host_arch))
+      candidates.push_back(platform);
+  }
+
+  // The selected or host platform didn't match any of the architectures. If
+  // the same platform supports all architectures then that's the obvious next
+  // best thing.
+  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();
+    }
+  }
+
+  // At this point we either have no platforms that match the given
+  // architectures or multiple platforms with no good way to disambiguate
+  // between them.
+  return nullptr;
+}
+
 std::vector<ArchSpec>
 Platform::CreateArchList(llvm::ArrayRef<llvm::Triple::ArchType> archs,
                          llvm::Triple::OSType os) {
   std::vector<ArchSpec> list;
   for(auto arch : archs) {
     llvm::Triple triple;
     triple.setArch(arch);
     triple.setOS(os);
     list.push_back(ArchSpec(triple));
   }
   return list;
 }
 
 /// Lets a platform answer if it is compatible with a given
 /// architecture and the target triple contained within.
 bool Platform::IsCompatibleArchitecture(const ArchSpec &arch,
                                         const ArchSpec &process_host_arch,
                                         bool exact_arch_match,
                                         ArchSpec *compatible_arch_ptr) {
   // If the architecture is invalid, we must answer true...
   if (arch.IsValid()) {
     ArchSpec platform_arch;
     auto match = exact_arch_match ? &ArchSpec::IsExactMatch
                                   : &ArchSpec::IsCompatibleMatch;
     for (const ArchSpec &platform_arch :
          GetSupportedArchitectures(process_host_arch)) {
       if ((arch.*match)(platform_arch)) {
         if (compatible_arch_ptr)
           *compatible_arch_ptr = platform_arch;
         return true;
       }
     }

lldb/source/Target/TargetList.cpp

if (num_specs > 0) {
         // Fat binary. A (valid) architecture was specified.
         module_spec.GetArchitecture() = arch;
         if (module_specs.FindMatchingModuleSpec(module_spec,
                                                 matching_module_spec))
             update_platform_arch(matching_module_spec.GetArchitecture());
       } 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;
+            error_strm.PutCString(platform_name);
+            platform_set.insert(platform_name);
           }
           error_strm.Printf("), specify an architecture to disambiguate");
           error.SetErrorString(error_strm.GetString());
           return error;
         }
       }
     }
   }

lldb/unittests/Platform/CMakeLists.txt

 add_lldb_unittest(LLDBPlatformTests
   PlatformAppleSimulatorTest.cpp
   PlatformDarwinTest.cpp
   PlatformMacOSXTest.cpp
   PlatformSiginfoTest.cpp
+  PlatformTest.cpp
 
   LINK_LIBS
     lldbPluginPlatformFreeBSD
     lldbPluginPlatformLinux
     lldbPluginPlatformMacOSX
     lldbPluginPlatformNetBSD
   LINK_COMPONENTS
     Support
   )
 
 add_subdirectory(Android)

lldb/unittests/Platform/PlatformTest.cpp

+//===-- 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();
+}