CMake: Set lapacke POSITION_INDEPENDENT_CODE=ON

[jschueller]

This ensures all static libs are compiled with -fPIC (only BLAS/CBLAS had it enabled)

[codecov]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (90398ae) 0.00% compared to head (ba94130) 0.00%.
Report is 3 commits behind head on master.

❗ Current head ba94130 differs from pull request most recent head 3709184. Consider uploading reports for the commit 3709184 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #932   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files        1918     1918           
  Lines      188653   188653           
=======================================
  Misses     188653   188653           

Files	Coverage Δ
SRC/cunbdb6.f	0.00% <ø> (ø)
SRC/dorbdb6.f	0.00% <ø> (ø)
SRC/sorbdb6.f	0.00% <ø> (ø)
SRC/zunbdb6.f	0.00% <ø> (ø)
SRC/cbdsqr.f	0.00% <0.00%> (ø)
SRC/zbdsqr.f	0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[christoph-conrads]

To best honest I do not know why the build setup should always activate this option for three reasons:

• There is no way to disable this setting while it is already easy to enable it in the current setup by calling env CFLAGS='-fPIC' FFLAGS='-fPIC' cmake lapack/.
• The flag makes certain optimizations impossible (specifically inlining) when the output is in ELF format because of symbol interposition (see Drepper: How To Write Shared Libraries, §1.5)
• CMake can infer automatically when this flag is needed unless one tries to build shared and static libraries with one CMake invocation (see the example below).

(only BLAS/CBLAS had it enabled)

It was added in commit 4c2236a. As stated above CMake can infer when this flag is needed unless one tries to create shared and static libraries at the same time so I wonder why this was done.

Here is a minimal CMake example demonstrating the inference capability.

Contents of CMakeLists.txt (note the lack of the PIC-related options):

cmake_minimum_required(VERSION 3.2)

project(cmake-automatic-pic-inference)

option(BUILD_SHARED_LIBS "Build shared libraries" OFF)

add_library(foo foo.c)

Execution (note the presence of of the -fPIC compiler flag only during the second compiler execution):

$ cat src/foo.c 
#include <stdio.h>

void foo() {
	printf("calling foo\n");
}
$ mkdir build
$ cd build/
$ cmake ../src/ # build a static library
[snip]
$ make VERBOSE=1
[snip]
[ 50%] Building C object CMakeFiles/foo.dir/foo.c.o
/usr/bin/cc    -MD -MT CMakeFiles/foo.dir/foo.c.o -MF CMakeFiles/foo.dir/foo.c.o.d -o CMakeFiles/foo.dir/foo.c.o -c /tmp/src/foo.c
[snip]
$ cmake -DBUILD_SHARED_LIBS=ON ../src/
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/build
$ make VERBOSE=1
[snip]
[ 50%] Building C object CMakeFiles/foo.dir/foo.c.o
/usr/bin/cc -Dfoo_EXPORTS  -fPIC -MD -MT CMakeFiles/foo.dir/foo.c.o -MF CMakeFiles/foo.dir/foo.c.o.d -o CMakeFiles/foo.dir/foo.c.o -c /tmp/src/foo.c
[snip]

[jschueller]

indeed, this patch is needed for building static libs, sorry if this wasnt clear
without it lapack libs are unusable for linking in a shared library and linker errors with:

relocation R_X86_64_32 against .rodata.str1.1 can not be used when making a shared object; recompile with -fPIC

it could be eventually an opt-out setting if you want to modify it after this lands

[christoph-conrads]

indeed, this patch is needed for building static libs, sorry if this wasnt clear
without it lapack libs are unusable for linking in a shared library and linker errors with:

(Emphasis mine.)

No, it is not (see the first bullet item in my post). You can have CMake pass the necessary flags to compiler already:

env CFLAGS='-fPIC' FFLAGS='-fPIC' cmake ../lapack/

[jschueller]

of course, but the point of the patch is to make that easier

[scivision]

CMake docs say when this property is used, CheckPIESupported should be used first https://cmake.org/cmake/help/latest/module/CheckPIESupported.html

[jschueller]

that only checks if its actually supported (we know its supported on standard linux/gcc platforms)
also that's for executables, which is out of scope, we're only interested in static libs

[phetdam]

If PIC is desired when compiling static libraries so they can be linked into ELF shared libraries it's probably better to make it an opt-in instead of forcing it to be set on targets as that is rather intrusive.

A CMake option like LAPACK_PIC_STATIC_LIBS that defaults to OFF as usual could be added, and when set to ON, will enable an if block that would then set the LAPACK and LAPACKE targets' POSITION_INDEPENDENT_CODE property, e.g.

if(LAPACK_PIC_STATIC_LIBS)
  set_target_properties(${LAPACKLIB} PROPERTIES POSITION_INDEPENDENT_CODE ON)
  set_target_properties(${LAPACKELIB} PROPERTIES POSITION_INDEPENDENT_CODE ON)
endif()

This is also preferable to globally setting CMAKE_POSITION_INDEPENDENT_CODE to ON, as that would affect compilation for all targets. Therefore, in my opinion this is a nice way to enable PIC static libraries if desired while not breaking existing behavior.

[jschueller]

@phetdam are there any actual use-cases to enable POSITION_INDEPENDENT_CODE for some target and not all ?
if not just just relying on CMAKE_POSITION_INDEPENDENT_CODE is simpler than adding a custom option for each

to be clear, this PR only makes LAPACK & LAPACKE consistent with BLAS & CBLAS target for which it is already enabled:
https://github.com/Reference-LAPACK/lapack/blob/master/BLAS/SRC/CMakeLists.txt#L155

if this is not wanted we might just as well remove it the pre-existing ones too and rely on CMAKE_POSITION_INDEPENDENT_CODE or CFLAGS/FFLAGS flags

[phetdam]

@jschueller sorry, I didn't realize that when BLAS/CBLAS are already compiled as PIC for static libraries. Given the fact that BLAS and LAPACK are usually used together, I think it would be unnecessarily granular in terms of build configuration and rather strange in general to require BLAS static libs to be PIC while leaving LAPACK static libs as the default non-PIC.

Static libraries are not compiled as PIC by default on UNIX-like systems, so to follow the principle of least astonishment, the requirement for BLAS/CBLAS to be compiled as PIC even when built statically should be removed. It also seems that requiring PIC inhibits some compiler optimizations, but either way I think it's best to not force static libs to be PIC unless requested.

I still favor having a project-specific flag, e.g. LAPACK_PIC_STATIC_LIBS, that can be set to ON to enable building BLAS + LAPACK static libs as PIC if so desired instead of using CMAKE_POSITION_INDEPENDENT_CODE directly. This is because the POSITION_INDEPENDENT_CODE property also affects executable targets, which might not be desired.

However, if that's not an issue, then using CMAKE_POSITION_INDEPENDENT_CODE would be the way to go.