Page MenuHomePhabricator

Re-apply "[CMake][compiler-rt][AArch64] Avoid preprocessing LSE builtins separately"
ClosedPublic

Authored by tambre on Dec 14 2020, 10:17 PM.

Details

Summary

aa772fc85e0f526615c78b9c3979c2be945a754c (D92530) has landed fixing relocations on Darwin.
3000c19df64f89ff319590f3a6e4d6b93d20983d (D93236) has landed working around an assembly parser bug on Darwin.
Previous quick-fix d9697c2e6b153ac7dc40a69450d9b672f71b1029 (D93198) included in this commit.

Invoking the preprocessor ourselves is fragile and would require us to replicate CMake's handling of definitions, compiler flags, etc for proper compatibility.
In my toolchain builds this notably resulted in a bunch of warnings from unused flags as my CMAKE_C_FLAGS includes CPU-specific optimization options.
Notably this part was already duplicating the logic for VISIBILITY_HIDDEN define.

Instead, symlink the files and set the proper set of defines on each.
This should also be faster as we avoid invoking the compiler multiple times.

Fixes https://llvm.org/PR48494

Diff Detail

Event Timeline

tambre created this revision.Dec 14 2020, 10:17 PM
tambre requested review of this revision.Dec 14 2020, 10:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 10:17 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
This revision was not accepted when it landed; it landed in state Needs Review.Dec 14 2020, 10:18 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

Early heads-up: We're seeing compile errors on our arm64 bots that might be due to this change: https://bugs.chromium.org/p/chromium/issues/detail?id=1159420

I'll verify locally and make a stand-alone repro. (It's possible that I'll find it's not due to this change while doing this.)

tambre added a comment.EditedDec 16 2020, 7:38 AM

Early heads-up: We're seeing compile errors on our arm64 bots that might be due to this change: https://bugs.chromium.org/p/chromium/issues/detail?id=1159420

I'll verify locally and make a stand-alone repro. (It's possible that I'll find it's not due to this change while doing this.)

A few followup fixes have landed for Darwin:

I suspect the issue you're seeing is fixed by D93378.
Previously the alignment specifier along with some other assembly directives would end up as comments with the Darwin assembly parser due to an incorrect separator being used.
See D93211 for the discussion.

Early heads-up: We're seeing compile errors on our arm64 bots that might be due to this change: https://bugs.chromium.org/p/chromium/issues/detail?id=1159420

I'll verify locally and make a stand-alone repro. (It's possible that I'll find it's not due to this change while doing this.)

A few followup fixes have landed for Darwin:

I suspect the issue you're seeing is fixed by D93378.
Previously the alignment specifier along with some other assembly directives would end up as comments with the Darwin assembly parser due to an incorrect separator being used.
See D93211 for the discussion.

Great, thanks! I'll try again with a newer clang, but does indeed look like it should fix the problem we're seeing.

ikudrin added a subscriber: ikudrin.Mar 5 2021, 5:41 AM

This change causes build failures in my Windows environment.

compiler-rt/lib/builtins/CMakeLists.txt
522

cmake -E create_symlink requires privilege escalation on Windows. Do you mind to fallback to just copy in that case? Something similar to what is used in AddLLVM.cmake and some other places.