Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARM64 outline atomic helpers are built using the C compiler #47838

Closed
tambry opened this issue Dec 12, 2020 · 3 comments
Closed

ARM64 outline atomic helpers are built using the C compiler #47838

tambry opened this issue Dec 12, 2020 · 3 comments
Assignees
@tambry
Labels
bugzilla Issues migrated from bugzilla compiler-rt:builtins

Comments

@tambry
Copy link
Contributor

tambry commented Dec 12, 2020

Bugzilla Link 48494
Resolution FIXED
Resolved on Feb 06, 2021 23:56
Version unspecified
OS All
Fixed by commit(s) d0797e6

Extended Description

Assembly files should not be compiled as C and instead use CMake's assembly language support to allow for vendors and users to specify flags, compiler options, use a different executable, have correct architecture flags, interact properly with toolchain files, etc.

The compiler-rt ARM64 outline atomic helpers are a new and the only case of this improper usage in the codebase.
It seems these should use the already-existing compiler-rt add_asm_sources() helper.

Previous occurrences of this were fixed by 45344cf. The improper usage resulted in multiple reports from users and vendors, requiring a backport to resolve build issues.
It would be best to avoid a recurrence of the problems. Marking as a release blocker due to this.

Pavel, could you take a look at this?
@tambry
Copy link
Contributor Author

tambry commented Dec 12, 2020

assigned to @tambry

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 13, 2020

I am using just C preprocessor to generate outline atomic helpers from lse.S. Then generated assembly files are added to aarch64_SOURCES along with other asm (aarch64/chkstk.S for example) and С sources to compile. There are a lot of *.S files used as sources for compilation as well (see *_SOURCES in compiler-rt/lib/builtins/CMakeLists.txt). Could you clarify is it something APPLE specific and I should use add_asm_sources() in 'if (APPLE)' case?

@tambry
Copy link
Contributor Author

tambry commented Dec 13, 2020

I am using just C preprocessor to generate outline atomic helpers from lse.S. Then generated assembly files are added to aarch64_SOURCES along with other asm (aarch64/chkstk.S for example) and С sources to compile. There are a lot of *.S files used as sources for compilation as well (see *_SOURCES in compiler-rt/lib/builtins/CMakeLists.txt).

Apologies, I should've read the code more carefully.

Nevertheless, passing CMAKE_C_FLAGS to the preprocessor isn't too good. In my case CMAKE_C_FLAGS contains processor-specific options (i.e. -mcpu=carmel) resulting in tons of warnings. I can imagine other vendors having options meant for compiling that will similarly result in warnings or even errors when only preprocessing.

Preprocessing separately seems unnecessary in addition to making things more fragile and complicated.
It seems it'd be simpler to simply symlink and compile each symlink with the definitions.

I went ahead and implemented that:
https://reviews.llvm.org/D93178

Could you clarify is it something APPLE specific and I should use add_asm_sources() in 'if (APPLE)' case?

Nothing Apple-specific. The APPLE and MINGW cases in add_asm_sources() are to workaround ASM language bugs in older CMake versions on those platforms.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tambry tambry

Labels
bugzilla Issues migrated from bugzilla compiler-rt:builtins
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tambry @llvmbot