Page MenuHomePhabricator

[compiler-rt] [cmake] Remove i686 target that is duplicate to i386
ClosedPublic

Authored by mgorny on Nov 16 2016, 12:44 PM.

Details

Summary

Remove the explicit i686 target that is completely duplicate to
the i386 target, with the latter being used more commonly.

  1. The runtime built for i686 will be identical to the one built for

i386.

  1. Supporting both -i386 and -i686 suffixes causes unnecessary confusion

on the clang end which has to expect either of them.

  1. The checks are based on wrong assumption that i686 is defined for

all newer x86 CPUs. In fact, it is only declared when -march=i686 is
explicitly used. It is not available when a more specific (or newer)
-march is used.

Curious enough, if CFLAGS contain -march=i686, the runtime will be built
both for i386 and i686. For any other value, only i386 variant will be
built.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny updated this revision to Diff 78246.Nov 16 2016, 12:44 PM
mgorny retitled this revision from to [compiler-rt] [cmake] Remove i686 target that is duplicate to i386.
mgorny updated this object.
mgorny added reviewers: samsonov, etienneb, beanz.
mgorny added a subscriber: cfe-commits.
beanz added a subscriber: eugenis.

Looping in @eugenis, who added i686 support in r218761.

eugenis, since i686 is identical to i386 generating it as a separate target is undesirable. Can you help advise as to how we can better meet your needs and not duplicate an effectively identical target?

Looping in @eugenis, who added i686 support in r218761.

eugenis, since i686 is identical to i386 generating it as a separate target is undesirable. Can you help advise as to how we can better meet your needs and not duplicate an effectively identical target?

I suspect D26796 attempts to solve the same problem.

eugenis edited edge metadata.Nov 28 2016, 10:39 AM
eugenis added subscribers: srhines, danalbert.
eugenis edited edge metadata.Nov 28 2016, 10:44 AM

I think this is the right move together with D26796.
Android build system will need to support both names for some time, depending on the toolchain version - looks doable. Technically, it can stick with the old name forever.

Ping. @eugenis, do you consider it good to go then? Or do you want me to change something specifically?

eugenis accepted this revision.Dec 29 2016, 11:47 AM
eugenis edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 29 2016, 11:47 AM
mgorny updated this revision to Diff 112807.Aug 27 2017, 12:45 AM

I'm going to commit this now to see if it helps with the Android buildbot. Updating to the rebased patch that's going to be committed.

This revision was automatically updated to reflect the committed changes.
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 revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Aug 29 2017, 11:27 AM
hans added inline comments.
compiler-rt/trunk/lib/asan/scripts/asan_device_setup
98

There are lots of copies of this script in various project that now all need to be updated (searching the web for asan_device_setup shows some).

For me personally, this means updating Chromium (also the build system needs an update) to handle different names before and after this revision.

Do you have any suggestions for how we should handle this rename? Is there some backwards-compatibility fix we could do?

As you noticed this broke some LLVM buildbots. It also broke Chromium buildbots, and I suppose other projects which use asan. I'm wondering if the fallout is worth the benefits here.

I may have underestimated the number of places that will need to be patched because of this change. Perhaps we should restore the special case in the android library name?

I'm happy to help with whatever needs to be done to keep breakage to the minimum, provided that we determine some clear path forward that doesn't involve regressing to the half-broken state for non-Android Linux systems and it's doable in the free time I can spare. However, I should point out that it was a very bad idea to hardcode paths all over the projects in the first place.

IMHO it was a very bad idea to include the "architecture name" (not even a target triple!) in the library path in the first place. No one else does that. GCC made the right choice and called theirs "libasan.so".

My plan is to tweak the code that sets the library name to use i686 when the target is i386 and the os is android, both in compiler-rt and in clang. There will be no duplicate targets.

hans added a comment.Aug 29 2017, 2:07 PM

IMHO it was a very bad idea to include the "architecture name" (not even a target triple!) in the library path in the first place. No one else does that. GCC made the right choice and called theirs "libasan.so".

My plan is to tweak the code that sets the library name to use i686 when the target is i386 and the os is android, both in compiler-rt and in clang. There will be no duplicate targets.

Sounds good to me. Let me know if you think this will take a while and we should revert to green in the meantime.

I've landed the android special case in r312048

rnk added a subscriber: rnk.Aug 29 2017, 3:29 PM

IMHO it was a very bad idea to include the "architecture name" (not even a target triple!) in the library path in the first place. No one else does that. GCC made the right choice and called theirs "libasan.so".

+1, one day we should untangle the compiler-rt library names.

My plan is to tweak the code that sets the library name to use i686 when the target is i386 and the os is android, both in compiler-rt and in clang. There will be no duplicate targets.

elram added a subscriber: elram.Apr 10 2018, 11:10 AM

This patch breaks cross-compiling the builtins for i686, with COMPILER_RT_DEFAULT_TARGET_ONLY=YES.
CC is set to

clang -target i686-linux-musl -m32 -march=i686

But no supported target is detected by cmake and nothing gets built.

-- Builtin supported architectures: