[PATCH] D72950: [CMake] compiler-rt: Add COMPILER_RT_BUILTINS_ENABLE_PIC

Mon Jan 20 11:12:58 PST 2020

JamesNagurne added inline comments.

================
Comment at: compiler-rt/lib/builtins/CMakeLists.txt:583
   if(COMPILER_RT_STANDALONE_BUILD)
-    append_list_if(COMPILER_RT_HAS_FPIC_FLAG -fPIC BUILTIN_CFLAGS)
+    if(COMPILER_RT_BUILTINS_ENABLE_PIC)
+      append_list_if(COMPILER_RT_HAS_FPIC_FLAG -fPIC BUILTIN_CFLAGS)
----------------
phosek wrote:
> 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72950/new/

https://reviews.llvm.org/D72950