[MachOObjectFile]Added Valid Architecture Function

[anushabasana]

Summary

Added array of valid architectures and function returning array.
Modified llvm-lipo to include list of valid architectures in error message for invalid arch.

Test Plan:

[alexander-shaposhnikov]

llvm/lib/Object/MachOObjectFile.cpp

58 ↗

(On Diff #206280)

missing const,
plus i'd move it out of the anonymous namespace and make it static (it doesn't change anything, just a convention in the LLVM codebase).
P.S. unfortunately, it looks like it can't be "constexpr" since the constructor
StringRef(const char *Str) is not constexpr at the moment.

2726 ↗

(On Diff #206280)

cbegin, cend

[anushabasana]

Addressed review comments.

[smeenai]

Looking good, just a few style nits.

llvm/lib/Object/MachOObjectFile.cpp

61 ↗

(On Diff #206280)

Nit: add a trailing comma, so the closing brace is on its own line.

I'm not a huge fan of the binpacking clang-format is doing here, but let's stick with what it wants.

2731 ↗

(On Diff #206280)

Nit: you can just do return validArchs; and the ArrayRef will be implicitly constructed.

llvm/tools/llvm-lipo/llvm-lipo.cpp

100 ↗

(On Diff #206280)

Perhaps "architecture names" instead of "architecture flags"? I know cctools lipo says "architecture flags", but their error message also says "unknown architecture specification flag"; without that earlier use of "flag", the context isn't as clear here.

103 ↗

(On Diff #206280)

Nit: I'd just inline the call to MachOObjectFile::getValidArchs() instead of creating a separate variable.

Super nit: after inlining and removing the variable, you can also remove the newline above this.

[anushabasana]

Fixed style comments.

[smeenai]

LGTM

[smeenai]

I'll wait for a day in case any other reviewers have comments and then commit this for you.

[mtrent]

llvm/include/llvm/Object/MachO.h

574 ↗

(On Diff #206306)

Super duper nitty nit: consider "getKnownArchs" instead of "getValidArchs". I'm wrestling with the idea of what makes an arch "valid", considering the list supported here is only the subset of archs that llvm knows about. E.g., is m68k an invalid Mach-O file? And so on. I readily concede this is the most pedantic of concerns.

I can see how an argument can be invalid, and so have no concerns over llvm-lipo's validateArchitectureName function name.

llvm/tools/llvm-lipo/llvm-lipo.cpp

99 ↗

(On Diff #206306)

super duper nitty nit from above applies here, too: consider "Unknown architecture" instead of "Invalid architecture."

[smeenai]

llvm/include/llvm/Object/MachO.h

574 ↗

(On Diff #206306)

We went with "valid" because that's the terminology the existing isValidArch was using. Should we rename that to isKnownArch as well?

[alexander-shaposhnikov]

LG

[smeenai]

llvm/include/llvm/Object/MachO.h

574 ↗

(On Diff #206306)

Ping @mtrent on the renaming question :)

[compnerd]

llvm/include/llvm/Object/MachO.h

574 ↗

(On Diff #206306)

I think maybe Supported is more appropriate. PPC should be a known valid arch as IIRC even though it is not currently supported by the interface.

[smeenai]

llvm/include/llvm/Object/MachO.h

574 ↗

(On Diff #206306)

I like Supported, but for PowerPC specifically I'll note that the current isValidArch does return true for it. If we renamed it to isSupportedArch I'm not sure if Darwin PPC is still considered supported...

[smeenai]

llvm/include/llvm/Object/MachO.h

574 ↗

(On Diff #206306)

Thoughts on the best terminology here?

[smeenai]

Given that we're following an existing name here, and we'd have to change that existing name as well were we to decide on a new name, I'm going to go ahead and commit this on @anushabasana's behalf. We can figure out the best name to use here and then change all of them at once post-commit.

[steven_wu]

This adds an unnecessary static initializer to the MachOObject.cpp. Can you move the array of validArchs into filescope? You can declare it in getValidArchs() and use getValidArchs() in isValidArchs.

llvm/trunk/lib/Object/MachOObjectFile.cpp

2727

llvm::find is easier here.

[smeenai]

In D63735#1587999, @steven_wu wrote:

This adds an unnecessary static initializer to the MachOObject.cpp. Can you move the array of validArchs into filescope? You can declare it in getValidArchs() and use getValidArchs() in isValidArchs.

I was hoping that std::array wouldn't necessitate a static initializer, since it's just supposed to be a zero-cost wrapper around a C array.

Could you clarify what you mean by file scope? It's already a static const inside MachOObjectFile.cpp; do you mean moving the declaration into getValidArchs()?

[steven_wu]

In D63735#1588025, @smeenai wrote:

In D63735#1587999, @steven_wu wrote:

This adds an unnecessary static initializer to the MachOObject.cpp. Can you move the array of validArchs into filescope? You can declare it in getValidArchs() and use getValidArchs() in isValidArchs.

I was hoping that std::array wouldn't necessitate a static initializer, since it's just supposed to be a zero-cost wrapper around a C array.

Not if you create std::array<StringRef>.

Could you clarify what you mean by file scope? It's already a static const inside MachOObjectFile.cpp; do you mean moving the declaration into getValidArchs()?

ArrayRef<StringRef> MachOObjectFile::getValidArchs() {
  static const std::array<StringRef, 17> validArchs = {
      "i386",   "x86_64", "x86_64h",  "armv4t",  "arm",    "armv5e",
      "armv6",  "armv6m", "armv7",    "armv7em", "armv7k", "armv7m",
      "armv7s", "arm64",  "arm64_32", "ppc",     "ppc64",
  };
  return validArchs;
}

llvm/trunk/include/llvm/Object/MachO.h

public:
   static StringRef guessLibraryShortName(StringRef Name, bool &isFramework,
                                          StringRef &Suffix);
 
   static Triple::ArchType getArch(uint32_t CPUType);
   static Triple getArchTriple(uint32_t CPUType, uint32_t CPUSubType,
                               const char **McpuDefault = nullptr,
                               const char **ArchFlag = nullptr);
   static bool isValidArch(StringRef ArchFlag);
+  static ArrayRef<StringRef> getValidArchs();
   static Triple getHostArch();
 
   bool isRelocatableObject() const override;
 
   StringRef mapDebugSectionName(StringRef Name) const override;
 
   bool hasPageZeroSegment() const { return HasPageZeroSegment; }
 

llvm/trunk/lib/Object/MachOObjectFile.cpp

namespace {
 
   struct section_base {
     char sectname[16];
     char segname[16];
   };
 
 } // end anonymous namespace
 
+static const std::array<StringRef, 17> validArchs = {
+    "i386",   "x86_64", "x86_64h",  "armv4t",  "arm",    "armv5e",
+    "armv6",  "armv6m", "armv7",    "armv7em", "armv7k", "armv7m",
+    "armv7s", "arm64",  "arm64_32", "ppc",     "ppc64",
+};
+
 static Error malformedError(const Twine &Msg) {
   return make_error<GenericBinaryError>("truncated or malformed object (" +
                                             Msg + ")",
                                         object_error::parse_failed);
 }
 
 // FIXME: Replace all uses of this function with getStructOrErr.
 template <typename T>
Triple MachOObjectFile::getArchTriple(uint32_t CPUType, uint32_t CPUSubType,
   }
 }
 
 Triple MachOObjectFile::getHostArch() {
   return Triple(sys::getDefaultTargetTriple());
 }
 
 bool MachOObjectFile::isValidArch(StringRef ArchFlag) {
-  return StringSwitch<bool>(ArchFlag)
-      .Case("i386", true)
-      .Case("x86_64", true)
-      .Case("x86_64h", true)
-      .Case("armv4t", true)
-      .Case("arm", true)
-      .Case("armv5e", true)
-      .Case("armv6", true)
-      .Case("armv6m", true)
-      .Case("armv7", true)
-      .Case("armv7em", true)
-      .Case("armv7k", true)
-      .Case("armv7m", true)
-      .Case("armv7s", true)
-      .Case("arm64", true)
-      .Case("arm64_32", true)
-      .Case("ppc", true)
-      .Case("ppc64", true)
-      .Default(false);
+  return std::find(validArchs.cbegin(), validArchs.cend(), ArchFlag) !=
+         validArchs.cend();
 }
 
+ArrayRef<StringRef> MachOObjectFile::getValidArchs() { return validArchs; }
+
 Triple::ArchType MachOObjectFile::getArch() const {
   return getArch(getCPUType(*this));
 }
 
 Triple MachOObjectFile::getArchTriple(const char **McpuDefault) const {
   return getArchTriple(Header.cputype, Header.cpusubtype, McpuDefault);
 }
 

llvm/trunk/test/tools/llvm-lipo/thin-universal-binary.test

 # RUN: yaml2obj %s > %t
 
-# RUN: not llvm-lipo %t -thin arc -output %t.out 2>&1 | FileCheck --check-prefix=ARCH_NOT_IN_FILE %s
-# ARCH_NOT_IN_FILE: does not contain the specified architecture arc to thin it to
+# RUN: not llvm-lipo %t -thin arm64_32 -output %t.out 2>&1 | FileCheck --check-prefix=ARCH_NOT_IN_FILE %s
+# ARCH_NOT_IN_FILE: does not contain the specified architecture arm64_32 to thin it to
 
 # RUN: not llvm-lipo %t -thin aarch101 -output %t.out 2>&1 | FileCheck --check-prefix=INVALID_ARCH %s
 # INVALID_ARCH: Invalid architecture: aarch101
 
 # RUN: yaml2obj %p/Inputs/i386-x86_64-universal.yaml > %t-universal.o
 # RUN: llvm-lipo %t-universal.o -thin i386 -output %t32.o
 # RUN: yaml2obj %p/Inputs/i386-slice.yaml > %t-basic32.o
 # RUN: cmp %t32.o %t-basic32.o

llvm/trunk/tools/llvm-lipo/llvm-lipo.cpp

struct Config {
   std::string ThinArchType;
   std::string OutputFile;
   LipoAction ActionToPerform;
 };
 
 } // end namespace
 
 static void validateArchitectureName(StringRef ArchitectureName) {
-  if (Triple(ArchitectureName).getArch() == Triple::ArchType::UnknownArch)
-    reportError("Invalid architecture: " + ArchitectureName);
+  if (!MachOObjectFile::isValidArch(ArchitectureName)) {
+    std::string Buf;
+    raw_string_ostream OS(Buf);
+    OS << "Invalid architecture: " << ArchitectureName
+       << "\nValid architecture names are:";
+    for (auto arch : MachOObjectFile::getValidArchs())
+      OS << " " << arch;
+    reportError(OS.str());
+  }
 }
 
 static Config parseLipoOptions(ArrayRef<const char *> ArgsArr) {
   Config C;
   LipoOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);