This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Use arch type to find compiler-rt libraries (on Linux)
ClosedPublic

Authored by mgorny on Nov 17 2016, 6:16 AM.

Details

Summary

Use llvm::Triple::getArchTypeName() when looking for compiler-rt
libraries, rather than the exact arch string from the triple. This is
more correct as it matches the values used when building compiler-rt
(builtin-config-ix.cmake) which are the subset of the values allowed
in triples.

For example, this fixes an issue when the compiler set for
i686-pc-linux-gnu triple would not find an i386 compiler-rt library,
while this is the exact arch that is detected by compiler-rt. The same
applies to any other i?86 variant allowed by LLVM.

This also makes the special case for MSVC unnecessary, since now i386
will be used reliably for all 32-bit x86 variants.

Diff Detail

Event Timeline

mgorny updated this revision to Diff 78359.Nov 17 2016, 6:16 AM
mgorny retitled this revision from to [Driver] Use arch type to find compiler-rt libraries (on Linux).
mgorny updated this object.
mgorny added reviewers: rsmith, compnerd, pcc, rengolin.
mgorny added a subscriber: cfe-commits.
mgorny updated this revision to Diff 78370.Nov 17 2016, 8:43 AM

Updated expected output for existing tests and added an additional test to linux-ld.c.

jroelofs added inline comments.Nov 17 2016, 8:53 AM
test/Driver/print-libgcc-file-name-clangrt.c
17–37

This CHECK's name is now a lie.

mgorny added inline comments.Nov 17 2016, 9:46 AM
test/Driver/print-libgcc-file-name-clangrt.c
17–37

Right. I'll change it.

mgorny updated this revision to Diff 78378.Nov 17 2016, 10:01 AM
mgorny marked 2 inline comments as done.

Updated the -print-libgcc-file-name test name. Additionally, I've added another subvariant of that test using i686-* target to ensure that the mapping works for that function too, in case we ever decide to split it.

rengolin resigned from this revision.Dec 13 2016, 7:38 AM
rengolin removed a reviewer: rengolin.

I don't know enough about the x86 environment to be able to review this patch. Sorry.

Second ping.

eugenis accepted this revision.Dec 29 2016, 1:08 PM
eugenis edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 29 2016, 1:08 PM
compnerd edited edge metadata.Dec 29 2016, 6:11 PM

This would need an upgrade path, since the output name can actually be different from the AchTypeName (e.g. i386 vs i686).

This would need an upgrade path, since the output name can actually be different from the AchTypeName (e.g. i386 vs i686).

I was considering committing this alongside with the i686 removal, and assuming that people are going to upgrade clang and compiler-rt in lockstep. Is there any other case that needs explicit consideration? Do you have another suggestion on handling this?

mgorny added a comment.Jan 6 2017, 1:20 PM

@compnerd, ping. Do you really think this is a real issue? As I said on the other patch, the only case when compiler-rt would generate i686-suffixed library is if the builder specifically used -march=i686 for the compiler-rt build, and in that case the i386 variant (which is identical to the i686 variant since both are built with the same flags) is built anyway.

I do think that this is an issue. Particularly for Windows targets, which have usually had the different architecture value.

Another ping. Since 4.0.0 final has been tagged, I think we should get back to working on this. @compnerd, any suggestion how to proceed here?

mgorny updated this revision to Diff 112796.Aug 26 2017, 12:59 PM

Rebased the patch since it no longer applied.

compnerd accepted this revision.Aug 26 2017, 1:55 PM

I don't think that there is a guarantee that compiler-rt and clang are upgraded in lockstep. At least for the builtins, there is a relatively stable interface. However, I don't think that at this point the Windows MSVC environment depends on compiler-rt much (except for complex multiplication/division in C99+). That can be satisfied with vcclang.lib. So, this is probably better off now than before.

This revision was automatically updated to reflect the committed changes.

Thanks a lot. Let's hope no buildbot complains now ;-).

@eugenis, it seems that this broke the Android buildbot. Could you suggest an appropriate course of action? I suppose we either have to fix Android-something to use i386, or keep using i686 for Android. The latter I know how to do but for the former, I have no clue where the issue might be.

mgorny reopened this revision.Aug 27 2017, 1:58 PM

I have reverted the changes for now to fix Android.

This revision is now accepted and ready to land.Aug 27 2017, 1:58 PM

This one still builds i686 libraries. Maybe it needs a clean cmake? Try "clobber" "1" in build properties.

[171/178] Linking CXX shared library /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/lib/clang/6.0.0/lib/linux/libclang_rt.asan-i686-android.so
[172/178] Linking CXX static library /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/lib/clang/6.0.0/lib/linux/libclang_rt.asan-i686-android.a

  1. http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/2146/steps/run%20instrumented%20asan%20tests%20%5Bi686%2Ffugu-userdebug%2FN2G48C%5D/logs/stdio -- after D26764.

This probably needs an update of zorg/buildbot/builders/sanitizers/buildbot_android.sh in the zorg repo.

@eugenis, thanks for the suggestion. I've found the problematic push (that attempts to move the wrong file) and filed D37226 for review.