Page MenuHomePhabricator

[CMake] compiler-rt: Add COMPILER_RT_BUILTINS_ENABLE_PIC
ClosedPublic

Authored by JamesNagurne on Jan 17 2020, 12:44 PM.

Details

Summary
The configuration for -fPIC in the builtins library when built standalone
is unconditional, stating that the flags would "normally be added... by
the llvm cmake step"

This is untrue, as the llvm cmake step checks LLVM_ENABLE_PIC, which allows
a client to turn off -fPIC.

I've added an option when compiler-rt builtins are configured standalone, such
as when built as part of the LLVM runtimes system, to guard the application of
-fPIC for users that want it.

Diff Detail

Event Timeline

JamesNagurne created this revision.Jan 17 2020, 12:44 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 17 2020, 12:44 PM
Herald added subscribers: llvm-commits, Restricted Project, mgorny, dberris. · View Herald Transcript
phosek added inline comments.Jan 17 2020, 5:00 PM
compiler-rt/lib/builtins/CMakeLists.txt
589–591

I think this changes the existing behavior. COMPILER_RT_STANDALONE_BUILD is set when compiler-rt is being built as a standalone project, which is not only when being built as part of runtimes, but also when you run CMake in compiler-rt as the source directory. In that case, COMPILER_RT_BUILTINS_ENABLE_PIC` will be unset because the new option is only defined when compiler-rt/lib/builtins is the root source directory, and -fPIC will be dropped in that case. This new option has to be defined in this block as well: https://github.com/llvm/llvm-project/blob/master/compiler-rt/CMakeLists.txt#L76 to avoid changing the current behavior in the standalone case.

JamesNagurne added inline comments.Jan 20 2020, 11:11 AM
compiler-rt/lib/builtins/CMakeLists.txt
589–591

Adding the option to the top-level block is undesirable, as that splits the option origin into two different places, since we need it both when builtins is the top-level, and when compiler-rt is the top-level.

Would adding the option in a block guarded by COMPILER_RT_STANDALONE_BUILD outside and just after of the SOURCE_DIR check it's currently in be a good alternative? That should fire in all cases of COMPILER_RT_STANDALONE_BUILD.

I'll upload the change when I get a chance, in case my description is confusing.

Moved option into a block guarded by COMPILER_RT_STANDLONE_BUILD, to indicate that it is an option that is available when not utilizing a linked LLVM toolchain and its configuration.

phosek accepted this revision.Jan 31 2020, 2:24 PM

I think this makes sense, LGTM.

This revision is now accepted and ready to land.Jan 31 2020, 2:24 PM
This revision was automatically updated to reflect the committed changes.