[PATCH] D32835: [compiler-rt] [cmake] Support generic installation

[Chris Bieneman via Phabricator via llvm-commits]

beanz added inline comments.


================
Comment at: CMakeLists.txt:41
+set(COMPILER_RT_LIBNAME "" CACHE STRING 
+  "Customize the library installation name")
+
----------------
clm wrote:
> beanz wrote:
> > What is the intention for this variable? Other than checking if it is set this doesn't seem to be used at all.
> The intention is to allow for a custom library name to be defined when cmake is invoked either by -C <cache_file> or -D options.
> 
> By using a combination of  COMPILER_RT_LIBRARY_INSTALL_DIR and COMPILER_RT_LIBNAME, different multilib combinations from the same architecture can be built and differentiated by directory name.
> 
> For example:
> 
> install/lib/clang-runtimes/arm-none-eabi/v7/libclang_rt.builtins.a
> install/lib/clang-runtimes/arm-none-eabi/v8/libclang_rt.builtins.a
> 
> etc.
> 
> 
> 
It doesn't do that. In fact, the variable is unused.

Also since this really only pertains to the builtins library, I'd rather it not be titled so generically. Maybe `COMPILER_RT_BUILTIN_LIBNAME` instead?


================
Comment at: cmake/Modules/AddCompilerRT.cmake:138
+  elseif(COMPILER_RT_LIBNAME)
+    set(libname "${name}")
+    set(output_name_${libname} ${libname}${COMPILER_RT_OS_SUFFIX})
----------------
jroelofs wrote:
> this should be:
> 
> ```
> set(libname ${COMPILER_RT_LIBNAME})
> ```
Given that this variable should only ever apply to the builtins library I think the override for the library name should be done elsewhere. Doing it in `AddCompilerRT` could have strange side-effects if the user configured more than just the builtins.

I would suggest adding a named parameter `LIBNAME` to `add_compiler_rt_runtime` that can be optionally set in lib/builtins/CMakeLists.txt if the user-exposed variable is set.


https://reviews.llvm.org/D32835