This is an archive of the discontinued LLVM Phabricator instance.

[llvm-libtool-darwin] Add support for -arch_only
ClosedPublic

Authored by sameerarora101 on Jul 28 2020, 10:28 AM.

Details

Summary

Add support for -arch_only option for llvm-libtool-darwin. This diff
also adds support for accepting universal files as input and flattening
them to create the required static library. Supports input universal
files contaning both Mach-O object files or archives.

Differences from cctools' libtool:

  • -arch_only can be specified multiple times
  • archives containing universal files are considered invalid (libtool allows such archives)

Depends on D84209.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2020, 10:28 AM
sameerarora101 requested review of this revision.Jul 28 2020, 10:28 AM
sameerarora101 edited the summary of this revision. (Show Details)Jul 28 2020, 10:29 AM
sameerarora101 edited the summary of this revision. (Show Details)
sameerarora101 updated this revision to Diff 281379.EditedJul 28 2020, 3:30 PM

Resolving clang-tidy issues

Add documentation for -arch_only option.

jhenderson added inline comments.Jul 31 2020, 1:38 AM
llvm/docs/CommandGuide/llvm-libtool-darwin.rst
64–67 ↗(On Diff #281738)

I should have been paying more attention to this before, but it would be good to have the options listed in alphabetical order, I think.

llvm/test/tools/llvm-libtool-darwin/cpu-subtype-matching.test
23

Nit: delete extra blank line.

llvm/test/tools/llvm-libtool-darwin/universal-file-flattening.test
90–91

Nit: here and in EMPTY-ARCH, add spaces to make the first line's pattern line up with the one below.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
108

Error messages should generally start with a lower-case letter (i.e. "Invalid" -> "invalid")

114

Given that there's no File parameter, perhaps thsi should be "a file's"

117

uint32_t for these, to match the return type of getCPUTypeFromArchitecture.

124–126

Rather than a comment that will be forgotten about and/or ignored, could you not just add the constant?

223

Nit: delete blank line here.

230

Don't use auto here, please.

246

Why are you throwing away this error rahter than reporting it earlier?

sameerarora101 marked 9 inline comments as done.Jul 31 2020, 8:24 AM
sameerarora101 added inline comments.
llvm/docs/CommandGuide/llvm-libtool-darwin.rst
64–67 ↗(On Diff #281738)

Ok, I have ordered them alphabetically now.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
124–126

Is it as simple as adding a constant? Nothing else needs to be done?

Here is a potential place where I think the constant should go (under MachO.h):

enum CPUSubTypeARM {
  CPU_SUBTYPE_ARM_ALL = 0,
  CPU_SUBTYPE_ARM_V4T = 5,
  CPU_SUBTYPE_ARM_V6 = 6,
  CPU_SUBTYPE_ARM_V5 = 7,
  CPU_SUBTYPE_ARM_V5TEJ = 7,
  CPU_SUBTYPE_ARM_XSCALE = 8,
  CPU_SUBTYPE_ARM_V7 = 9,
  //  unused  ARM_V7F     = 10,
  CPU_SUBTYPE_ARM_V7S = 11,
  CPU_SUBTYPE_ARM_V7K = 12,
  CPU_SUBTYPE_ARM_V6M = 14,
  CPU_SUBTYPE_ARM_V7M = 15,
  CPU_SUBTYPE_ARM_V7EM = 16
};

enum CPUSubTypeARM64 {
  CPU_SUBTYPE_ARM64_ALL = 0,
  CPU_SUBTYPE_ARM64E = 2,
};

enum CPUSubTypeARM64_32 { CPU_SUBTYPE_ARM64_32_V8 = 1 };

However, I am also unsure of the RHS value that needs to be assigned to the constant. Any ideas?

246

So a universal member can either be a MachOObjectFile or an Archive - so we need to try casting the member in both ways. First I try casting it as an object file. If that gives an error, it might be the case that the member is an Archive. Now, if I succeed on casting the member as an Archive, then I can throw away the error that I received from casting the member as an object file as the member was an archive and should not have been casted as an object file in first place.

llvm-lipo also seems to be following this design (under printBinaryArchs):

for (const auto &O : UO->objects()) {
      Expected<std::unique_ptr<MachOObjectFile>> MachOObjOrError =
          O.getAsObjectFile();
      if (MachOObjOrError) {
        OS << Slice(MachOObjOrError->get()).getArchString() << " ";
        continue;
      }
      Expected<std::unique_ptr<Archive>> ArchiveOrError = O.getAsArchive();
      if (ArchiveOrError) {
        consumeError(MachOObjOrError.takeError());
        OS << Slice(ArchiveOrError->get()).getArchString() << " ";
        continue;
      }
      consumeError(ArchiveOrError.takeError());
      reportError(Binary->getFileName(), MachOObjOrError.takeError());
    }
sameerarora101 marked 2 inline comments as done.

Alphabetically ordering docs.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
124–126

Added diff for constant V8 here: D85041

jhenderson added inline comments.Aug 3 2020, 1:12 AM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
108

We tend to avoid explicit new lines in error messages. They can make it non-obvious that output belongs to the error message, when stderr and stdout are piped to the same location, which can be confusing. You can probably just have this as a single sentence (i.e. "invalid architecture 'foo': valid architecture names are ..."). Note that I also added the ArchitectureName to the message, since that gives a little more context as to what is wrong.

124–126

I think your other patch is fine. It may be beneficial to look at the history of the file you're modifiying to see what else got changed when other subtypes were added.

246

Okay, makes a degree of sense. consumeError calls should really have a comment explaining why it's safe to throw them away though, so please add one.

sameerarora101 marked 3 inline comments as done.Aug 3 2020, 8:27 AM
sameerarora101 added inline comments.Aug 3 2020, 8:34 AM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
124–126

Yup, thanks. Furthermore, I'll also wait on D85041 to get landed so that I can make modifications here.

Invalid architecture error msg and consumeError comment.

All other changes look fine. Just waiting on D85041 then.

Rebase after adding constant V8.

sameerarora101 added a comment.EditedAug 7 2020, 3:49 PM

I think I need to add V8 support under llvm/include/llvm/TextAPI/MachO/Architecture.def here:

///
/// ARM64 architectures sorted by cpu sub type id.
///
ARCHINFO(arm64, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64_ALL, 64)
ARCHINFO(arm64e, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64E, 64)

What should be the first element for that tuple in case of V8 (arm64_v8 or arm64v8 or maybe something else) ? Thanks.

I think I need to add V8 support under llvm/include/llvm/TextAPI/MachO/Architecture.def here:

///
/// ARM64 architectures sorted by cpu sub type id.
///
ARCHINFO(arm64, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64_ALL, 64)
ARCHINFO(arm64e, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64E, 64)

What should be the first element for that tuple in case of V8 (arm64_v8 or arm64v8 or maybe something else) ? Thanks.

Hmm, what necessitates the TAPI patch? arm64_v8 seems fine to me, though @ributzka and @cishida should weigh in there.

I think I need to add V8 support under llvm/include/llvm/TextAPI/MachO/Architecture.def here:

///
/// ARM64 architectures sorted by cpu sub type id.
///
ARCHINFO(arm64, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64_ALL, 64)
ARCHINFO(arm64e, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64E, 64)

What should be the first element for that tuple in case of V8 (arm64_v8 or arm64v8 or maybe something else) ? Thanks.

Hmm, what necessitates the TAPI patch? arm64_v8 seems fine to me, though @ributzka and @cishida should weigh in there.

Converting a yaml file with Subtype V8 to an object file requires this patch. I was trying to add a test for V8, but my yaml was incorrectly parsed as an object file with unknown architecture.

I think I need to add V8 support under llvm/include/llvm/TextAPI/MachO/Architecture.def here:

///
/// ARM64 architectures sorted by cpu sub type id.
///
ARCHINFO(arm64, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64_ALL, 64)
ARCHINFO(arm64e, MachO::CPU_TYPE_ARM64, MachO::CPU_SUBTYPE_ARM64E, 64)

What should be the first element for that tuple in case of V8 (arm64_v8 or arm64v8 or maybe something else) ? Thanks.

Hmm, what necessitates the TAPI patch? arm64_v8 seems fine to me, though @ributzka and @cishida should weigh in there.

Converting a yaml file with Subtype V8 to an object file requires this patch. I was trying to add a test for V8, but my yaml was incorrectly parsed as an object file with unknown architecture.

Ah, yes reading in tbd files as input will require this addition. I have no strong opinion, but I prefer for arm64v8 as will cause less downstream disruption.

jhenderson accepted this revision.Aug 12 2020, 1:30 AM

LGTM, but please wait for other stakeholders.

This revision is now accepted and ready to land.Aug 12 2020, 1:30 AM
smeenai accepted this revision.Aug 12 2020, 4:39 PM

LGTM.

Let's commit this one as-is, and you can add arm64v8 to TextAPI and add the corresponding test here as a follow-up.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
131

Nit: you don't need braces, since both sides are simple return expressions (the first one is multi-line, but I think it's pretty clear it's part of the same statement).

LGTM.

Let's commit this one as-is, and you can add arm64v8 to TextAPI and add the corresponding test here as a follow-up.

Got it. I also realized yesterday that adding arm64v8 to TextAPI is not the only thing necessary for supporting v8 tests. We would have to update llvm/lib/Object/MachOObjectFile.cpp file as well to be able to recognize the Triple() correctly. I saw your response on D85334. It makes more sense to add the test after arm64e/arm64v8 support is there (as separate patch only). Thanks, I'll commit this now

sameerarora101 marked an inline comment as done.Aug 13 2020, 6:29 AM

Remove redundant braces.

This revision was automatically updated to reflect the committed changes.