Page MenuHomePhabricator

[llvm-ar] Use target triple to deduce archive kind for bitcode inputs
ClosedPublic

Authored by pirama on Mar 19 2020, 4:19 PM.

Details

Summary

When using full LTO on cross-compile settings, instead of generating the
default archive kind of the host platform, we could deduce the archive
kind based on the target triple.

This specifically addresses https://github.com/android/ndk/issues/1209
by making it possible to drop llvm-ar in place of GNU ar without extra
flags.

Diff Detail

Event Timeline

pirama created this revision.Mar 19 2020, 4:19 PM
Herald added a project: Restricted Project. ยท View Herald TranscriptMar 19 2020, 4:19 PM

Thanks for fixing this ๐Ÿ‘

llvm/tools/llvm-ar/llvm-ar.cpp
904

Looks like there are a few more options: https://llvm.org/doxygen/classllvm_1_1object_1_1Archive.html#ab288378fa8bfa0678dd25e36b5198a87. I think this regresses behavior for those other hosts.

https://llvm.org/doxygen/classllvm_1_1object_1_1Archive.html#ab288378fa8bfa0678dd25e36b5198a87. I think this regresses behavior for those other hosts.

*BSD may need the __.SYMDEF format.

Have you considered passing an explicit --format= via something like ARFLAGS?

pirama updated this revision to Diff 251521.Mar 19 2020, 6:20 PM

Handle bitcode inputs with BSD target triple.

pirama marked an inline comment as done.Mar 19 2020, 6:40 PM

https://llvm.org/doxygen/classllvm_1_1object_1_1Archive.html#ab288378fa8bfa0678dd25e36b5198a87. I think this regresses behavior for those other hosts.

*BSD may need the __.SYMDEF format.

Have you considered passing an explicit --format= via something like ARFLAGS?

The goal is to avoid requiring all users of the NDK on Darwin from having to set --format= when switching to LLVM binutils. This format deduction only runs when no --format is provided and works for all cases except full LTO. Also based on the deduction based on ObjectFile at the beginning of this function, it seems that all BSD environments require --format=bsd to be specified because it uses GNU archive kind for all ELF files.

llvm/tools/llvm-ar/llvm-ar.cpp
904

Note that getDefaultForHost is also incorrect on BSD (it returns K_GNU if the host triple is not Darwin). So this doesn't exactly regress that behavior. But it's good to handle this correctly for the narrow cases where we can. I will fix getDefaultForHost in a follow up as it'd require a different test.

I also don't think llvm-ar is set up to pass through the other archive kinds (GNU64, DARWIN64, COFF).

This comment was removed by pirama.
pirama updated this revision to Diff 251551.Mar 19 2020, 9:45 PM

Apply clang-format.

MaskRay added inline comments.Mar 19 2020, 10:42 PM
llvm/test/tools/llvm-ar/Inputs/bsd.ll
1 โ†—(On Diff #251551)

The ModuleID comment is not needed now.

I suggest that you use RUN: echo -e '....' > %t.bsd.ll to inline the trivial test. Also drop dso_local

4 โ†—(On Diff #251551)

Since you dropped the attribute group, you need to drop the comment as well.

llvm/tools/llvm-ar/llvm-ar.cpp
905

I will ask some BSD people whether this is correct.

(Honestly I don't like the logic much.)

MaskRay added inline comments.Mar 20 2020, 10:08 AM
llvm/include/llvm/ADT/Triple.h
506 โ†—(On Diff #251551)

We should not need this helper.

llvm/tools/llvm-ar/llvm-ar.cpp
905

It seems that we can simply use K_GNU. Edited from Bdragon's words:

K_BSD, "__.SYMDEF", the 4.4BSD archive format (under development in 4.3BSD and finalized in 4.4BSD), is no longer as common use at the moment as people might think it is.

"openbsd appears to have tossed their ar and ranlib completely 6 years ago, and NetBSD switched to binutils ar 21 years ago."

So the only difference is between Darwin and GNU.

pirama updated this revision to Diff 251689.Mar 20 2020, 10:13 AM

Simplify test.

MaskRay added inline comments.Mar 20 2020, 10:31 AM
llvm/test/tools/llvm-ar/lto-kind-from-triple.test
2

Newly added tests (e.g. symtab.test) start to use ## for comments and # for RUN/CHECK lines even if llvm-ar does not read anything from the text file.

3

Darwin archive format is the only different platform now. We can special case it and treat everything else GNU.

5

Use echo -e '...' to avoid quoting "

22

Can be deleted because 4.4BSD archive format is out of fashion nowadays.

pirama updated this revision to Diff 251697.Mar 20 2020, 10:34 AM

Use K_GNU for all ELF targets.

pirama updated this revision to Diff 251704.Mar 20 2020, 10:47 AM

Treat any bitode with non-Darwin triple as GNU.
Test cleanup.

pirama marked 7 inline comments as done.Mar 20 2020, 10:51 AM
pirama added inline comments.
llvm/test/tools/llvm-ar/lto-kind-from-triple.test
3

Assuming this is just a general observation and no action needed.

llvm/tools/llvm-ar/llvm-ar.cpp
905

Thanks for following up on this. I've updated the latest patch to use K_GNU for all non-Darwin triples. It was previously set to K_GNU only when OSBinFormat was ELF. The new logic is in sync with what this deduction is doing everywhere else:

non-Darwin object file -> K_GNU
non-Darwin host -> K_GNU.
pirama updated this revision to Diff 251705.Mar 20 2020, 10:57 AM
pirama marked an inline comment as done.

Use isOSDarwin() instead of isOSBinFormatMachO()

Harbormaster completed remote builds in B49919: Diff 251697.
Harbormaster failed remote builds in B49925: Diff 251704!
MaskRay added inline comments.Mar 20 2020, 11:25 AM
llvm/test/tools/llvm-ar/lto-kind-from-triple.test
21

Add another test demonstrating that adding %t.macho.ll will not change GNU format to darwin format.

llvm/tools/llvm-ar/llvm-ar.cpp
897

Optional: you can write

if (auto ObjOrErr = ...) {
} else {

consumeError(ObjOrErr.takeError());

}

902

Optional: Triple(IRObject.getTargetTriple()).isOSDarwin()

906

Nit: capitalize the initial word in the comment.

pirama updated this revision to Diff 251716.Mar 20 2020, 12:06 PM

Cleanup based on review comments.

MaskRay accepted this revision.Mar 20 2020, 12:35 PM
This revision is now accepted and ready to land.Mar 20 2020, 12:35 PM

Merged. Thanks for the review @MaskRay.

This revision was automatically updated to reflect the committed changes.