[lldb] Simplify specifying of platform supported architectures

[labath]

Summary

The GetSupportedArchitectureAtIndex pattern forces the use of
complicated patterns in both the implementations of the function and in
the various callers.

This patch creates a new method (GetSupportedArchitectures), which
returns a list (vector) of architectures. The
GetSupportedArchitectureAtIndex is kept in order to enable incremental
rollout. Base Platform class contains implementations of both of these
methods, using the other method as the source of truth. Platforms
without infinite stacks should implement at least one of them.

This patch also ports Linux, FreeBSD and NetBSD platforms to the new
API. A new helper function (CreateArchList) is added to simplify the
common task of creating a list of ArchSpecs with the same OS but
different architectures.

[labath]

lldb/include/lldb/Target/Platform.h

332

If all platforms (as I expect them to) implement this by maintaining a vector of supported architectures, this could be changed into an ArrayRef<ArchSpec>, but returning a value type for now simplifies the rollout.

[JDevlieghere]

lldb/include/lldb/Target/Platform.h

330–331

Do you expect any platform to implement both?

lldb/source/Target/Platform.cpp

1148–1149

In PlatformDarwin we set the vendor too. Could we have this take an optional OSType and Vendor maybe that sets the ones that are not None? That would also make migrating PlatformDarwin easier where we currently share the code and don't set the OS. Later we could move this out of PlatformDarwin into the child classes and have the OS set correctly.

[labath]

lldb/include/lldb/Target/Platform.h

330–331

Not really, though it wouldn't necessarily be an error if they did. I'll just remove the "at least" part.

lldb/source/Target/Platform.cpp

1148–1149

Umm.. maybe? I don't think this function is set in stone, but the nice thing about it being a helper function is that it's usage is completely optional. I haven't tried porting the darwin platforms yet, but, from a cursory glance I did see that they have a lot more complex usage patterns:

• they pass subarchitectures as well, so we'd either also need to have an Optional subarch field, or switch to passing architectures as strings (I mean, I suppose I could switch to strings now, but it feels more correct to work with named constants)
• a few of them also pass environments
• and some don't even have matching OS fields (x86_64-apple-macosx and arm64-apple-ios in the same list)

So, I'd leave it to those patches to determine whether it's better to extend this function or create a new one.

[labath]

• remove "at least"
• change CreateArchList argument order to match the triple format. This would be particularly important if new arguments are added later.

[labath]

Fix a rather embarrasing off-by-one in the base implementation of
GetSupportedArchitectureAtIndex, which I guess also means I did not run tests
before uploading this (I have now).

[JDevlieghere]

LGTM

lldb/source/Target/Platform.cpp

1148–1149

Okay, I'm happy to modify the helper when I start using it for the darwin platfomrs. I think at least some of the complexity/inconsistencies is purely historic such as the one where we were specifying an OS in the list of supported architectures in PlatformDarwin.

[labath]

lldb/source/Target/Platform.cpp

1148–1149

Cool. Does that mean I can count on you do convert the darwin platforms? :)

(I was going to try doing it myself, but I have basically no way of checking whether my modifications break anything.)

[JDevlieghere]

lldb/source/Target/Platform.cpp

1148–1149

Yup, although I can't commit to a timeframe :-) For validation you'd need to run the "on device" test suite and there's no real way for non-Apple people to do that, but if you get to it before me I'm more than happy to apply the patch locally and try it out.

lldb/include/lldb/Target/Platform.h

public:
   /// \param[out] arch
   ///     A copy of the architecture at index if the return value is
   ///     \b true.
   ///
   /// \return
   ///     \b true if \a arch was filled in and is valid, \b false
   ///     otherwise.
   virtual bool GetSupportedArchitectureAtIndex(uint32_t idx,
-                                               ArchSpec &arch) = 0;
+                                               ArchSpec &arch);
+
+  /// Get the platform's supported architectures in the order in which they
+  /// should be searched.
+  /// NB: This implementation is mutually recursive with
+  /// GetSupportedArchitectureAtIndex. Subclasses should implement one of them.
+  virtual std::vector<ArchSpec> GetSupportedArchitectures();
 
   virtual size_t GetSoftwareBreakpointTrapOpcode(Target &target,
                                                  BreakpointSite *bp_site);
 
   /// Launch a new process on a platform, not necessarily for debugging, it
   /// could be just for running the process.
   virtual Status LaunchProcess(ProcessLaunchInfo &launch_info);
 
   /// Perform expansion of the command-line for this launch info This can
public:
   ///     entry value. \b nullptr if not implemented or  if the process has no
   ///     crash information entry. \b error if an error occured.
   virtual llvm::Expected<StructuredData::DictionarySP>
   FetchExtendedCrashInformation(lldb_private::Process &process) {
     return nullptr;
   }
 
 protected:
+  /// 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::ProcessSP DoConnectProcess(llvm::StringRef connect_url,
                                    llvm::StringRef plugin_name,
                                    Debugger &debugger, Stream *stream,
                                    Target *target, Status &error);
   bool m_is_host;
   // Set to true when we are able to actually set the OS version while being

lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.h

public:
 
   // lldb_private::Platform functions
   llvm::StringRef GetDescription() override {
     return GetPluginDescriptionStatic(IsHost());
   }
 
   void GetStatus(Stream &strm) override;
 
-  bool GetSupportedArchitectureAtIndex(uint32_t idx, ArchSpec &arch) override;
+  std::vector<ArchSpec> GetSupportedArchitectures() override;
 
   bool CanDebugProcess() override;
 
   void CalculateTrapHandlerSymbolNames() override;
 
   MmapArgList GetMmapArgumentList(const ArchSpec &arch, lldb::addr_t addr,
                                   lldb::addr_t length, unsigned prot,
                                   unsigned flags, lldb::addr_t fd,
                                   lldb::addr_t offset) override;
+
+  std::vector<ArchSpec> m_supported_architectures;
 };
 
 } // namespace platform_freebsd
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PLATFORM_FREEBSD_PLATFORMFREEBSD_H

lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp

void PlatformFreeBSD::Terminate() {
   }
 
   PlatformPOSIX::Terminate();
 }
 
 /// Default Constructor
 PlatformFreeBSD::PlatformFreeBSD(bool is_host)
     : PlatformPOSIX(is_host) // This is the local host platform
-{}
-
-bool PlatformFreeBSD::GetSupportedArchitectureAtIndex(uint32_t idx,
-                                                      ArchSpec &arch) {
-  if (IsHost()) {
+{
+  if (is_host) {
     ArchSpec hostArch = HostInfo::GetArchitecture(HostInfo::eArchKindDefault);
-    if (hostArch.GetTriple().isOSFreeBSD()) {
-      if (idx == 0) {
-        arch = hostArch;
-        return arch.IsValid();
-      } else if (idx == 1) {
-        // If the default host architecture is 64-bit, look for a 32-bit
-        // variant
-        if (hostArch.IsValid() && hostArch.GetTriple().isArch64Bit()) {
-          arch = HostInfo::GetArchitecture(HostInfo::eArchKind32);
-          return arch.IsValid();
-        }
-      }
+    m_supported_architectures.push_back(hostArch);
+    if (hostArch.GetTriple().isArch64Bit()) {
+      m_supported_architectures.push_back(
+          HostInfo::GetArchitecture(HostInfo::eArchKind32));
     }
   } else {
-    if (m_remote_platform_sp)
-      return m_remote_platform_sp->GetSupportedArchitectureAtIndex(idx, arch);
-
-    llvm::Triple triple;
-    // Set the OS to FreeBSD
-    triple.setOS(llvm::Triple::FreeBSD);
-    // Set the architecture
-    switch (idx) {
-    case 0:
-      triple.setArchName("x86_64");
-      break;
-    case 1:
-      triple.setArchName("i386");
-      break;
-    case 2:
-      triple.setArchName("aarch64");
-      break;
-    case 3:
-      triple.setArchName("arm");
-      break;
-    case 4:
-      triple.setArchName("mips64");
-      break;
-    case 5:
-      triple.setArchName("mips");
-      break;
-    case 6:
-      triple.setArchName("ppc64");
-      break;
-    case 7:
-      triple.setArchName("ppc");
-      break;
-    default:
-      return false;
-    }
-    // Leave the vendor as "llvm::Triple:UnknownVendor" and don't specify the
-    // vendor by calling triple.SetVendorName("unknown") so that it is a
-    // "unspecified unknown". This means when someone calls
-    // triple.GetVendorName() it will return an empty string which indicates
-    // that the vendor can be set when two architectures are merged
-
-    // Now set the triple into "arch" and return true
-    arch.SetTriple(triple);
-    return true;
-  }
-  return false;
+    m_supported_architectures = CreateArchList(
+        {llvm::Triple::x86_64, llvm::Triple::x86, llvm::Triple::aarch64,
+         llvm::Triple::arm, llvm::Triple::mips64, llvm::Triple::ppc64,
+         llvm::Triple::ppc},
+        llvm::Triple::FreeBSD);
+
+std::vector<ArchSpec> PlatformFreeBSD::GetSupportedArchitectures() {
+  if (m_remote_platform_sp)
+    return m_remote_platform_sp->GetSupportedArchitectures();
+  return m_supported_architectures;
 }
 
 void PlatformFreeBSD::GetStatus(Stream &strm) {
   Platform::GetStatus(strm);
 
 #if LLDB_ENABLE_POSIX
   // Display local kernel information only when we are running in host mode.
   // Otherwise, we would end up printing non-FreeBSD information (when running

lldb/source/Plugins/Platform/Linux/PlatformLinux.h

public:
 
   // lldb_private::Platform functions
   llvm::StringRef GetDescription() override {
     return GetPluginDescriptionStatic(IsHost());
   }
 
   void GetStatus(Stream &strm) override;
 
-  bool GetSupportedArchitectureAtIndex(uint32_t idx, ArchSpec &arch) override;
+  std::vector<ArchSpec> GetSupportedArchitectures() override;
 
   uint32_t GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info) override;
 
   bool CanDebugProcess() override;
 
   void CalculateTrapHandlerSymbolNames() override;
 
   lldb::UnwindPlanSP GetTrapHandlerUnwindPlan(const llvm::Triple &triple,
                                               ConstString name) override;
 
   MmapArgList GetMmapArgumentList(const ArchSpec &arch, lldb::addr_t addr,
                                   lldb::addr_t length, unsigned prot,
                                   unsigned flags, lldb::addr_t fd,
                                   lldb::addr_t offset) override;
+
+  std::vector<ArchSpec> m_supported_architectures;
 };
 
 } // namespace platform_linux
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PLATFORM_LINUX_PLATFORMLINUX_H

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp

void PlatformLinux::Terminate() {
   }
 
   PlatformPOSIX::Terminate();
 }
 
 /// Default Constructor
 PlatformLinux::PlatformLinux(bool is_host)
     : PlatformPOSIX(is_host) // This is the local host platform
-{}
-
-bool PlatformLinux::GetSupportedArchitectureAtIndex(uint32_t idx,
-                                                    ArchSpec &arch) {
-  if (IsHost()) {
+{
+  if (is_host) {
     ArchSpec hostArch = HostInfo::GetArchitecture(HostInfo::eArchKindDefault);
-    if (hostArch.GetTriple().isOSLinux()) {
-      if (idx == 0) {
-        arch = hostArch;
-        return arch.IsValid();
-      } else if (idx == 1) {
-        // If the default host architecture is 64-bit, look for a 32-bit
-        // variant
-        if (hostArch.IsValid() && hostArch.GetTriple().isArch64Bit()) {
-          arch = HostInfo::GetArchitecture(HostInfo::eArchKind32);
-          return arch.IsValid();
-        }
-      }
+    m_supported_architectures.push_back(hostArch);
+    if (hostArch.GetTriple().isArch64Bit()) {
+      m_supported_architectures.push_back(
+          HostInfo::GetArchitecture(HostInfo::eArchKind32));
     }
   } else {
-    if (m_remote_platform_sp)
-      return m_remote_platform_sp->GetSupportedArchitectureAtIndex(idx, arch);
-
-    llvm::Triple triple;
-    // Set the OS to linux
-    triple.setOS(llvm::Triple::Linux);
-    // Set the architecture
-    switch (idx) {
-    case 0:
-      triple.setArchName("x86_64");
-      break;
-    case 1:
-      triple.setArchName("i386");
-      break;
-    case 2:
-      triple.setArchName("arm");
-      break;
-    case 3:
-      triple.setArchName("aarch64");
-      break;
-    case 4:
-      triple.setArchName("mips64");
-      break;
-    case 5:
-      triple.setArchName("hexagon");
-      break;
-    case 6:
-      triple.setArchName("mips");
-      break;
-    case 7:
-      triple.setArchName("mips64el");
-      break;
-    case 8:
-      triple.setArchName("mipsel");
-      break;
-    case 9:
-      triple.setArchName("s390x");
-      break;
-    default:
-      return false;
-    }
-    // Leave the vendor as "llvm::Triple:UnknownVendor" and don't specify the
-    // vendor by calling triple.SetVendorName("unknown") so that it is a
-    // "unspecified unknown". This means when someone calls
-    // triple.GetVendorName() it will return an empty string which indicates
-    // that the vendor can be set when two architectures are merged
-
-    // Now set the triple into "arch" and return true
-    arch.SetTriple(triple);
-    return true;
-  }
-  return false;
+    m_supported_architectures = CreateArchList(
+        {llvm::Triple::x86_64, llvm::Triple::x86, llvm::Triple::arm,
+         llvm::Triple::aarch64, llvm::Triple::mips64, llvm::Triple::mips64,
+         llvm::Triple::hexagon, llvm::Triple::mips, llvm::Triple::mips64el,
+         llvm::Triple::mipsel, llvm::Triple::systemz},
+        llvm::Triple::Linux);
+
+std::vector<ArchSpec> PlatformLinux::GetSupportedArchitectures() {
+  if (m_remote_platform_sp)
+    return m_remote_platform_sp->GetSupportedArchitectures();
+  return m_supported_architectures;
 }
 
 void PlatformLinux::GetStatus(Stream &strm) {
   Platform::GetStatus(strm);
 
 #if LLDB_ENABLE_POSIX
   // Display local kernel information only when we are running in host mode.
   // Otherwise, we would end up printing non-Linux information (when running on

lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.h

public:
 
   // lldb_private::Platform functions
   llvm::StringRef GetDescription() override {
     return GetPluginDescriptionStatic(IsHost());
   }
 
   void GetStatus(Stream &strm) override;
 
-  bool GetSupportedArchitectureAtIndex(uint32_t idx, ArchSpec &arch) override;
+  std::vector<ArchSpec> GetSupportedArchitectures() override;
 
   uint32_t GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info) override;
 
   bool CanDebugProcess() override;
 
   void CalculateTrapHandlerSymbolNames() override;
 
   MmapArgList GetMmapArgumentList(const ArchSpec &arch, lldb::addr_t addr,
                                   lldb::addr_t length, unsigned prot,
                                   unsigned flags, lldb::addr_t fd,
                                   lldb::addr_t offset) override;
+
+  std::vector<ArchSpec> m_supported_architectures;
 };
 
 } // namespace platform_netbsd
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_PLATFORM_NETBSD_PLATFORMNETBSD_H

lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp

void PlatformNetBSD::Terminate() {
   }
 
   PlatformPOSIX::Terminate();
 }
 
 /// Default Constructor
 PlatformNetBSD::PlatformNetBSD(bool is_host)
     : PlatformPOSIX(is_host) // This is the local host platform
-{}
-
-bool PlatformNetBSD::GetSupportedArchitectureAtIndex(uint32_t idx,
-                                                     ArchSpec &arch) {
-  if (IsHost()) {
+{
+  if (is_host) {
     ArchSpec hostArch = HostInfo::GetArchitecture(HostInfo::eArchKindDefault);
-    if (hostArch.GetTriple().isOSNetBSD()) {
-      if (idx == 0) {
-        arch = hostArch;
-        return arch.IsValid();
-      } else if (idx == 1) {
-        // If the default host architecture is 64-bit, look for a 32-bit
-        // variant
-        if (hostArch.IsValid() && hostArch.GetTriple().isArch64Bit()) {
-          arch = HostInfo::GetArchitecture(HostInfo::eArchKind32);
-          return arch.IsValid();
-        }
-      }
+    m_supported_architectures.push_back(hostArch);
+    if (hostArch.GetTriple().isArch64Bit()) {
+      m_supported_architectures.push_back(
+          HostInfo::GetArchitecture(HostInfo::eArchKind32));
     }
   } else {
-    if (m_remote_platform_sp)
-      return m_remote_platform_sp->GetSupportedArchitectureAtIndex(idx, arch);
-
-    llvm::Triple triple;
-    // Set the OS to NetBSD
-    triple.setOS(llvm::Triple::NetBSD);
-    // Set the architecture
-    switch (idx) {
-    case 0:
-      triple.setArchName("x86_64");
-      break;
-    case 1:
-      triple.setArchName("i386");
-      break;
-    default:
-      return false;
-    }
-    // Leave the vendor as "llvm::Triple:UnknownVendor" and don't specify the
-    // vendor by calling triple.SetVendorName("unknown") so that it is a
-    // "unspecified unknown". This means when someone calls
-    // triple.GetVendorName() it will return an empty string which indicates
-    // that the vendor can be set when two architectures are merged
-
-    // Now set the triple into "arch" and return true
-    arch.SetTriple(triple);
-    return true;
-  }
-  return false;
+    m_supported_architectures = CreateArchList(
+        {llvm::Triple::x86_64, llvm::Triple::x86}, llvm::Triple::NetBSD);
+
+std::vector<ArchSpec> PlatformNetBSD::GetSupportedArchitectures() {
+  if (m_remote_platform_sp)
+    return m_remote_platform_sp->GetSupportedArchitectures();
+  return m_supported_architectures;
 }
 
 void PlatformNetBSD::GetStatus(Stream &strm) {
   Platform::GetStatus(strm);
 
 #if LLDB_ENABLE_POSIX
   // Display local kernel information only when we are running in host mode.
   // Otherwise, we would end up printing non-NetBSD information (when running

lldb/source/Target/Platform.cpp

Platform::GetPlatformForArchitecture(const ArchSpec &arch,
                                      ArchSpec *platform_arch_ptr) {
   lldb::PlatformSP platform_sp;
   Status error;
   if (arch.IsValid())
     platform_sp = Platform::Create(arch, platform_arch_ptr, error);
   return platform_sp;
 }
 
+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;
+}
+
+bool Platform::GetSupportedArchitectureAtIndex(uint32_t idx, ArchSpec &arch) {
+  const auto &archs = GetSupportedArchitectures();
+  if (idx >= archs.size())
+    return false;
+  arch = archs[idx];
+  return true;
+}
+
+std::vector<ArchSpec> Platform::GetSupportedArchitectures() {
+  std::vector<ArchSpec> result;
+  ArchSpec arch;
+  for (uint32_t idx = 0; GetSupportedArchitectureAtIndex(idx, arch); ++idx)
+    result.push_back(arch);
+  return result;
+}
+
 /// Lets a platform answer if it is compatible with a given
 /// architecture and the target triple contained within.
 bool Platform::IsCompatibleArchitecture(const ArchSpec &arch,
                                         bool exact_arch_match,
                                         ArchSpec *compatible_arch_ptr) {
   // If the architecture is invalid, we must answer true...
   if (arch.IsValid()) {
     ArchSpec platform_arch;