Skip to content
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

improve weak link handling #3

Merged
merged 4 commits into from
Jul 16, 2016
Merged

improve weak link handling #3

merged 4 commits into from
Jul 16, 2016

Conversation

opadron
Copy link
Owner

@opadron opadron commented Jul 8, 2016

Improves the weak-linking CMake module proposed in scikit-build/scikit-build#47.
Also updates the test script output and expected output table in the README.

  • prefers not linking on Linux over double-linking; consistent with the original version of this module. Falls back to linking only when necessary.
  • added a new linker option to check: "--unresolved-symbols=ignore-all" for GNU ld on Linux.
    • This option is necessary when trying to weakly-link executable targets.
  • parameterize weak-link support checking based on the types of the target and the library.
    • Necessary because Linux handles STATIC binaries, SHARED binaries, and EXECUTABLES differently.
  • Guards system check against the case of crosscompiling without an emulator.
    • In this case, the relevant HAS_DYNAMIC_LOOKUP_<ABC>_<XYZ> and DYNAMIC_LOOKUP_FLAGS_<ABC>_<XYZ> variables are pre-cleared and must be set manually.

|d00 |:white_check_mark:|:white_check_mark:|:x:RTE | 2|
|d01 |:white_check_mark:|:white_check_mark:|:white_check_mark:| 5|
|d10 |:white_check_mark:|:white_check_mark:|:x:RTE | 2|
|d11 |:white_check_mark:|:white_check_mark:|:white_check_mark:| 7|

###### OSX (XCODE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify the compiler used, and version.

@blowekamp
Copy link

How does "--unresolved-symbols=ignore-all" relate to the ENABLE_EXPORTS executable property [1]?

I have found the generated project to be overly verbose for a simple test case. To simplify the generated test project consider: replacing the set/get with a single count_incr, which returns the value, doing away with the header files and just specifying "extern func" in the c files.

Your table nicely summarized the results, I think it's as I expect. It took me a little bit to decode the "s00" etc.

I am not sure if wearing linking executables needs to be considering, although it was an interesting experiment. Are you considering MODULES to be the same as SHARED for the weak-link checking?

[1] https://cmake.org/cmake/help/v3.0/prop_tgt/ENABLE_EXPORTS.html

@opadron
Copy link
Owner Author

opadron commented Jul 8, 2016

How does "--unresolved-symbols=ignore-all" relate to the ENABLE_EXPORTS executable property [1]?

I'm pretty sure the two are not related. This argument is only needed to convince GNU ld to leave symbols unresolved in an executable (it's fine with leaving them unresolved for shared libraries).

I have found the generated project to be overly verbose for a simple test case. To simplify the generated test project consider: replacing the set/get with a single count_incr, which returns the value, doing away with the header files and just specifying "extern func" in the c files.

Good point, I've added an update that tries to cut down on the code.

I am not sure if wearing linking executables needs to be considering, although it was an interesting experiment.

I'm of the opinion that a lack of apparent use cases shouldn't be sole justification for leaving something out. Sure, it's applicability is crippled on Linux, but on OSX, it seems to get along just fine. Since we've already worked out a sensible MO for handling this case, I don't see a point in going out of our way to remove it, now.

Are you considering MODULES to be the same as SHARED for the weak-link checking?

No, not for the internal test project.

For the table that is created, the test only considers STATIC and SHARED libs (L). The module (M) is always a MODULE library. For the internal test project in the CMake module, every combination of SHARED,STATIC,MODULE, and EXE target is tested separately for linking against SHARED or STATIC libraries (the tests are cached and are only performed on-demand). The exact configuration of the test project is adjusted to test the particular combination at hand.

For example, for an EXE target weak linking against a STATIC lib, the links are [1]:

M ==> L
E --> L
E ==> M

In this case, the target is the EXE.

Whereas to test a SHARED target, the module is made a shared library:

M --> L
E ==> L
E ==> M

And for the typical MODULE target:

M --> L
E ==> L
E <-- M

[1]

Legend
L the library
M the module
E the executable
==> "is properly linked against"
--> "is weakly linked against"
<-- "dynamically loads ___ at runtime"

@blowekamp
Copy link

LGTM!

@opadron opadron merged commit b6c029a into master Jul 16, 2016
@opadron opadron deleted the improved-weak-link-handling branch July 16, 2016 16:16
@jcfr
Copy link
Contributor

jcfr commented Sep 24, 2016

Here, shouldn't E <-- M be E --> M. The executable is loading the module not the other way around ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants