Page MenuHomePhabricator

[llvm-libtool-darwin] Support universal outputs
ClosedPublic

Authored by sameerarora101 on Aug 5 2020, 11:25 AM.

Details

Summary

Add support for producing universal binaries containing archives when
llvm-libtool-darwin is given inputs of multiple architectures.
Depends on D84770 and D84858.

Diff Detail

Event Timeline

sameerarora101 created this revision.Aug 5 2020, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2020, 11:25 AM
sameerarora101 requested review of this revision.Aug 5 2020, 11:25 AM

Adding more tests and resolve clang-tidy

smeenai added inline comments.Aug 10 2020, 10:19 PM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
29

Note that DenseMap doesn't guarantee deterministic iteration order, which is something to watch out for in general (you need std::map or MapVector to get determinism). Over here though, you sort the slices afterwards, so it's fine.

138–139

Not this diff, but it's not ideal to be recomputing this for every single input file.

Since you have to do this computation in createStaticLibrary as well now, perhaps you could make ArchCPUType and ArchCPUSubtype part of your Config, and then pass that into this function?

377

Nit: newline above this.

sameerarora101 marked 3 inline comments as done.Aug 11 2020, 10:07 AM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
29

Got it, thanks!

138–139

oh wow, great idea. It actually helps me compute std::tie(ArchCPUType, ArchCPUSubtype) just once as well (while creating the config). Thanks!

sameerarora101 marked 2 inline comments as done.

Make ArchCPUType and ArchCPUSubtype part of Config.

jhenderson added inline comments.Aug 12 2020, 12:59 AM
llvm/test/tools/llvm-libtool-darwin/universal-output.test
5–6

Rather than having two near-identical YAML blobs, use the -D option to pull out the different bits:

# RUN: yaml2obj %s --docnum=1 -o %t.armv6 -DSUBTYPE=0x6 ...
# RUN: yaml2obj %s --docnum=2 -o %t.armv7 -DSUBTYPE=0x9 ...

There's a lot of extra noise in the two, which I suspect you can get rid of with a bit of effort. You don't need to have the YAML directly represent a real object you've made, as long as it's sufficient to trigger the code paths you need. For example, do you actually need the segment and section contents?

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

Looks to me like this function is only partially tested? What about the other three CPUs and the default case?

353

You should probably avoid auto here. It's not obvious from the immediate context what the type is.

sameerarora101 marked 3 inline comments as done.Aug 12 2020, 9:50 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/universal-output.test
5–6

Thanks, I was able to get rid of section contents as well. However, I still need the segment command or it throws an "invalid object file" error (perhaps because of the string table below?). Updated it now.

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

Ok also added test for CPU_TYPE_ARM64, CPU_TYPE_X86_64 and the default case. CPU_TYPE_ARM64_32 does not have a subtype so I didn't add a test for it.

sameerarora101 marked 2 inline comments as done.

Add tests for different cpu types.

sameerarora101 added inline comments.Aug 12 2020, 9:59 AM
llvm/test/tools/llvm-libtool-darwin/universal-output.test
55–67

for CPU_TYPE_ARM64, I cannot test whether the architecture is present with llvm-lipo as it says "unknown(...)" for arm64e. So I did this check using llvm-objdump (whether the constituent members are present inside the fat file or not). Is this ok? To allow lipo to recognize arm64e I would have to make a modification under llvm/lib/Object/MachOObjectFile.cpp :

case MachO::CPU_TYPE_ARM64:
  switch (CPUSubType & ~MachO::CPU_SUBTYPE_MASK) {
  case MachO::CPU_SUBTYPE_ARM64_ALL:
    if (McpuDefault)
      *McpuDefault = "cyclone";
    if (ArchFlag)
      *ArchFlag = "arm64";
    return Triple("arm64-apple-darwin");
  default:
    return Triple();
  }

Not sure what would go under *McpuDefault = ... and return Triple("..."); for the case of arm64e?

smeenai added inline comments.Aug 12 2020, 5:06 PM
llvm/test/tools/llvm-libtool-darwin/universal-output.test
55–67

Apple hasn't fully upstreamed arm64e to LLVM yet. You can see their implementation of this at https://github.com/llvm/llvm-project-staging/blob/f0fd4f3af0acda5068acc5bbe10b3792c41bdfc4/llvm/lib/Object/MachOObjectFile.cpp#L2695-L2700.

I think it's fine to use objdump for this for now; just add a comment about why.

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

Can you have two files with a CPUType that falls under the default case but different CPUSubtypes, and confirm that they get put in the same slice of the output file?

353

I agree that it's not obvious, but the expanded type for this (and for iterators in general) is also extremely verbose :/ I thought the general guideline was to use auto for range based for loops for that reason. Perhaps we could stick with the auto in the loop and assign the members of the pair to variables in the loop? (It looks like we're only using the second member anyway.)

354–355

You want uint64_t, and you don't need the std::allocator member in the vector (that's the default).

jhenderson added inline comments.Aug 13 2020, 1:01 AM
llvm/test/tools/llvm-libtool-darwin/universal-output.test
5–6

Thanks. I don't know enough about Mach-O to be able to tell you why the object reading code won't handle it without segments. For now, I think this is fine.

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

I thought the general guideline was to use auto for range based for loops for that reason.

I wasn't aware that it was the guidance for all range based for loops? https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable seems to indicate auto where the context makes the type obvious, or the type is already abstracted away (usually applies for iterators).

The trouble is here that I have no idea that NewMembers is even a DenseMap, let alone a DenseMap of integers to vectors of ArchiveMembers. Maybe assigning the member to a local variable could help. Are you able to just use std::pair too, or does that not work? You can get rid of the llvm:: prefixes everywhere here too.

There's some concern elsewhere about using DenseMap with integer keys, because -1/-2 are reserved values that result in assertions if used. Probably not that important, but maybe you would be better off using unordered_map?

sameerarora101 marked 6 inline comments as done.Aug 13 2020, 7:24 AM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
123

Besides the 4 non-default cases below, the file llvm/lib/Object/MachOObjectFile.cpp supports the following CPU types (which will fall under default case):

  • MachO::CPU_TYPE_I386
  • MachO::CPU_TYPE_POWERPC
  • MachO::CPU_TYPE_POWERPC64

However, for all these default CPU types, there is just a single subtype:

  • MachO::CPU_SUBTYPE_I386_ALL
  • MachO::CPU_SUBTYPE_POWERPC_ALL
  • MachO::CPU_SUBTYPE_POWERPC_ALL

Thus, I am unable to have two files having the same CPU type that falls under default case but have different Subtypes :(
Any other way I should test this?

353

ok, updated it to use unordered_map? How does this look now?

sameerarora101 marked 2 inline comments as done.

Use unordered_map in place of DenseMap.

Resolve test on windows (order in which architectures appear in test file).

jhenderson added inline comments.Aug 14 2020, 12:12 AM
llvm/test/tools/llvm-libtool-darwin/universal-output.test
77

Better would be {{(armv6 ppc)|(ppc armv6)}}. The current pattern could match armv6 armv6 for example.

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

I think you can drop the const from the uin64_t, right?

sameerarora101 marked an inline comment as done.Aug 14 2020, 6:18 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/universal-output.test
77

Ah yes, you are right. Thanks!

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

I guess not. If I do that, it doesn't compile and throws the following error:

/home/sameerarora101/local/llvm-project/llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:354:10:   required from here
/usr/include/c++/8/bits/stl_construct.h:75:7: error: use of deleted function ‘llvm::NewArchiveMember::NewArchiveMember(const llvm::NewArchiveMember&)’
     { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }
sameerarora101 marked an inline comment as done.

Reorder architectures in test.

LGTM, but please wait for @smeenai to confirm he is happy.

smeenai accepted this revision.Aug 14 2020, 10:04 AM

LGTM

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

Got it, yeah, I don't think there's anything we can do then.

This revision is now accepted and ready to land.Aug 14 2020, 10:04 AM

unordered_map -> map to guarantee deterministic output across platforms.

This revision was automatically updated to reflect the committed changes.

Hi @sameerarora101,

sorry, but LLVM::universal-output.test test gets failed on the armv7 cross toolchain buider.
Here is a direct link to the failed test log
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/802

the first failed build itself
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/802

Would you take care of it?
Thank you.

Taking a look. Seems like an output format difference with llvm-objdump.

I pushed rG12b4df991950 as a speculative fix. I'll monitor the bot.

That didn't work, but I'm able to repro locally using the CMake cache file used by the builder, so I'm digging into it.

rG402b063c8067 should fix it. Sorry for the breakage!

Thank you @smeenai,
not a problem.