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.

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

FindPythonExtensions: set symbol visibility correctly #299

Closed
wants to merge 2 commits into
base: master
from

Conversation

2 participants
@ghost

ghost commented Feb 22, 2018

In addition to aligning the behavior of scikit-build with distutils, this PR attempts a generic fix to issues such as scipy/scipy#8451.

Closes gh-295.

)
elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU" AND
"${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND
"${CMAKE_Fortran_COMPILER_ID}" STREQUAL "GNU")

This comment has been minimized.

@ghost

ghost Feb 22, 2018

What I really want to say is that "this extensions will be linked with ld." If there's a better way to do that, please say so.

@ghost ghost changed the title from FindPythonExtensions: set symbol visibility correclty to FindPythonExtensions: set symbol visibility correctly Feb 22, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Feb 22, 2018

Codecov Report

Merging #299 into master will decrease coverage by 1.57%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
- Coverage   90.74%   89.17%   -1.58%     
==========================================
  Files          26       26              
  Lines        1027     1035       +8     
  Branches      183      184       +1     
==========================================
- Hits          932      923       -9     
- Misses         65       81      +16     
- Partials       30       31       +1
Impacted Files Coverage Δ
skbuild/platform_specifics/windows.py 70.12% <0%> (-22.08%) ⬇️
skbuild/command/bdist_wheel.py 100% <0%> (ø) ⬆️
skbuild/command/egg_info.py 77.27% <0%> (ø) ⬆️
skbuild/setuptools_wrap.py 95.98% <0%> (+0.01%) ⬆️
skbuild/cmaker.py 73.29% <0%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f5cfc6...62b3f71. Read the comment docs.

@@ -317,6 +317,31 @@ if(NOT DEFINED PYTHON_EXTENSION_MODULE_SUFFIX)
set(PYTHON_EXTENSION_MODULE_SUFFIX "${_item}")
endif()
function(_set_python_extension_symbol_visibility _target)

This comment has been minimized.

@jcfr

jcfr Feb 22, 2018

Contributor

To support both python2 and python3, I suggest to have something like this:

function(_set_python_extension_symbol_visibility _target)
  if(PYTHON_VERSION_MAJOR VERSION_GREATER 2)
    set(module_init_function_prefix "PyInit_")
  else()
    set(module_init_function_prefix "init")
  endif()

  if("${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")
    set_target_properties(${_target} PROPERTIES LINK_FLAGS 
        "/EXPORT:${module_init_function_prefix}${_target}"
    )
  elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU")
    set(_script_path
      ${CMAKE_CURRENT_BINARY_DIR}/_skbuild/${_target}-version-script.map
    )
    file(WRITE
      ${_script_path}
      "
      {
        global:
          ${module_init_function_prefix}${_target};
        local: *;
      }
      "      
    )
    set_target_properties(${_target} PROPERTIES LINK_FLAGS 
        "-Wl,--version-script=${_script_path}"
    )
  endif()
endfunction()

This comment has been minimized.

@jcfr

jcfr Feb 22, 2018

Contributor

Also, instead of _skbuild, we should introduce the equivalent of SKBUILD_DIR (already available in python as skbuild.constant.SKBUILD_DIR) in CMake

This comment has been minimized.

@jcfr

jcfr Feb 22, 2018

Contributor

Since extension modules are expected to use PyMODINIT_FUNC, the associated symbol should already exported. For example, see here

On linux, using readelf -s /tmp/pytest-of-jcfr/pytest-4148/test_hello_cython_builds0/_skbuild/linux-x86_64-3.5/cmake-install/hello/_hello.cpython-35m-x86_64-linux-gnu.so also confirmed that the symbol is already global. Same info are reported with/without this change after running pytest tests/test_hello_cython.py::test_hello_cython_builds

I suspect that visibility needs to be set only for the cython generated modules.

readelf -s /tmp/pytest-of-jcfr/pytest-4148/test_hello_cython_builds0/_skbuild/linux-x86_64-3.5/cmake-install/hello/_hello.cpython-35m-x86_64-linux-gnu.so | ack -i init
    66: 0000000000002d10  1828 FUNC    GLOBAL DEFAULT   11 PyInit__hello
    68: 0000000000001608     0 FUNC    GLOBAL DEFAULT    9 _init
    33: 0000000000203d98     0 OBJECT  LOCAL  DEFAULT   16 __frame_dummy_init_array_
   115: 0000000000002d10  1828 FUNC    GLOBAL DEFAULT   11 PyInit__hello
   138: 0000000000001608     0 FUNC    GLOBAL DEFAULT    9 _init

This comment has been minimized.

@ghost

ghost Feb 22, 2018

The purpose of the script on Linux to hide all of the other symbols. Can you confirm the script has the intended effect?

}
"
)
set_target_properties(${_target} PROPERTIES LINK_FLAGS

This comment has been minimized.

@jcfr

jcfr Feb 23, 2018

Contributor

It turns out that this a compile flags and not a link flags. COMPILE_FLAGS should be used

This comment has been minimized.

@ghost

ghost Feb 23, 2018

On windows, link flags are used (I think).

This comment has been minimized.

@jcfr

jcfr Feb 23, 2018

Contributor

I talked to fast ... in fact the following is happening:

  • _set_python_extension_symbol_visibility set the linker flags as it should
  • but these are reset to -Wl,--unresolved-symbols=ignore-all when gnu is used. See function target_link_libraries_with_dynamic_lookup called lower in this file.

I will try to call the function _set_python_extension_symbol_visibility afterward

This comment has been minimized.

@jcfr

jcfr Feb 23, 2018

Contributor

Now that the linker is flag is passed, here is the error:

[100%] Linking CXX shared module _hello.cpython-35m-x86_64-linux-gnu.so
cd /tmp/pytest-of-jcfr/pytest-jcfr/_skbuild/linux-x86_64-3.5/cmake-build/hello && /home/jcfr/Software/cmake-3.10.0-Linux-x86_64/bin/cmake -E cmake_link_script CMakeFiles/_hello.dir/link.txt --verbose=1
/usr/bin/c++ -fPIC -O3 -DNDEBUG -Wl,--version-script=/tmp/pytest-of-jcfr/pytest-jcfr/_skbuild/linux-x86_64-3.5/cmake-build/hello/_skbuild/_hello-version-script.map -shared  -o _hello.cpython-35m-x86_64-linux-gnu.so CMakeFiles/_hello.dir/_hello.cxx.o 
/usr/bin/ld:/tmp/pytest-of-jcfr/pytest-jcfr/_skbuild/linux-x86_64-3.5/cmake-build/hello/_skbuild/_hello-version-script.map:7: syntax error in VERSION script
collect2: error: ld returned 1 exit status
hello/CMakeFiles/_hello.dir/build.make:94: recipe for target 'hello/_hello.cpython-35m-x86_64-linux-gnu.so' failed
make[2]: *** [hello/_hello.cpython-35m-x86_64-linux-gnu.so] Error 1

This comment has been minimized.

@ghost

ghost Feb 23, 2018

If that is the case, then it's probably best to append the version script to the -Wl,--unresolved-symbols=ignore-all argument.

@ghost

This comment has been minimized.

ghost commented Feb 23, 2018

@jcfr Are the issues addressed now?

@jcfr

This comment has been minimized.

Contributor

jcfr commented Feb 23, 2018

With the last fix, it seems to have the intended effect:

With version script

Symbol table '.dynsym' contains 14 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000708     0 SECTION LOCAL  DEFAULT    9 
     2: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _ITM_deregisterTMCloneTab
     3: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __stack_chk_fail@GLIBC_2.4 (2)
     4: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PyLong_FromLong
     5: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND __gmon_start__
     6: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PyModule_Create2
     7: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _Py_NoneStruct
     8: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PyArg_ParseTuple
     9: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _Jv_RegisterClasses
    10: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _ITM_registerTMCloneTable
    11: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PySys_WriteStdout
    12: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND __cxa_finalize@GLIBC_2.2.5 (3)
    13: 0000000000000930    17 FUNC    GLOBAL DEFAULT   11 PyInit__hello

Without version script

Symbol table '.dynsym' contains 19 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 00000000000007c8     0 SECTION LOCAL  DEFAULT    9 
     2: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _ITM_deregisterTMCloneTab
     3: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __stack_chk_fail@GLIBC_2.4 (2)
     4: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PyLong_FromLong
     5: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND __gmon_start__
     6: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PyModule_Create2
     7: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND _Py_NoneStruct
     8: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PyArg_ParseTuple
     9: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _Jv_RegisterClasses
    10: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND _ITM_registerTMCloneTable
    11: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND PySys_WriteStdout
    12: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND __cxa_finalize@GLIBC_2.2.5 (3)
    13: 0000000000201160     0 NOTYPE  GLOBAL DEFAULT   22 _edata
    14: 0000000000201168     0 NOTYPE  GLOBAL DEFAULT   23 _end
    15: 00000000000009f0    17 FUNC    GLOBAL DEFAULT   11 PyInit__hello
    16: 0000000000201160     0 NOTYPE  GLOBAL DEFAULT   23 __bss_start
    17: 00000000000007c8     0 FUNC    GLOBAL DEFAULT    9 _init
    18: 0000000000000a04     0 FUNC    GLOBAL DEFAULT   12 _fini
@ghost

This comment has been minimized.

ghost commented Feb 23, 2018

Great!

xoviat and others added some commits Feb 22, 2018

FindPythonExtensions: Set symbol visibility to export only module ini…
…t function

This commit updates link flags associated with GNU and MSVC compiler to
ensure only the init function is exported.

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>
xoviat
@jcfr

This comment has been minimized.

Contributor

jcfr commented Apr 2, 2018

Thanks again @xoviat

Changes integrated in 8b15eff

@jcfr jcfr closed this Apr 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment