Page MenuHomePhabricator

[MachOObjectFile]Added Valid Architecture Function
ClosedPublic

Authored by anushabasana on Jun 24 2019, 12:26 PM.

Details

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:

Diff Detail

Repository
rL LLVM

Event Timeline

anushabasana created this revision.Jun 24 2019, 12:26 PM
alexshap added inline comments.Jun 24 2019, 2:38 PM
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

Addressed review comments.

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.

Fixed style comments.

smeenai accepted this revision.Jun 24 2019, 3:34 PM

LGTM

This revision is now accepted and ready to land.Jun 24 2019, 3:34 PM

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

mtrent accepted this revision.Jun 24 2019, 3:36 PM
mtrent added inline comments.
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 added inline comments.
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?

smeenai added inline comments.Jun 27 2019, 9:37 AM
llvm/include/llvm/Object/MachO.h
574 ↗(On Diff #206306)

Ping @mtrent on the renaming question :)

compnerd added inline comments.Jun 27 2019, 10:03 AM
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 added inline comments.Jun 27 2019, 1:40 PM
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 added inline comments.Jul 1 2019, 5:09 PM
llvm/include/llvm/Object/MachO.h
574 ↗(On Diff #206306)

Thoughts on the best terminology here?

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.

This revision was automatically updated to reflect the committed changes.

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.

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()?

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;
}