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

BLD: ensure libnpymath and highway static libs use hidden visibility #26286

Merged
merged 1 commit into from Apr 19, 2024

Conversation

rgommers
Copy link
Member

The effect this has is that symbols don't get re-exported as public by Python extension modules that link with these shared libraries. E.g., building on macOS arm64 with Clang changes the set of exported symbols for _multiarray_umath from:

% dy2ld_info -exports build/numpy/_core/_multiarray_umath.cpython-39-darwin.so
build/numpy/_core/_multiarray_umath.cpython-39-darwin.so [arm64]:
    -exports:
        offset      symbol
        0x00112BA4  _PyInit__multiarray_umath
        0x001C955C  _npy_spacingf
        0x001C95F8  _npy_spacing
        ...
        <many more _npy symbols>

to:

  build/numpy/_core/_multiarray_umath.cpython-311-darwin.so [arm64]:
    -exports:
        offset      symbol
        0x0010B8F8  _PyInit__multiarray_umath

This works for all compilers that support GNU-style __attribute__((visibility("hidden")), which both GCC and Clang do. Tested on SciPy as well, it has the same effect there. For example, scipy.special._ufuncs was re-exporting all _npy_* symbols and after this change that is no longer the case.

Note that the libnpyrandom static library is left alone here. Trying to change visibility there breaks a test for CFFI, because that test is accessing private symbols. This is clearly wrong, but explicitly documented at
https://numpy.org/devdocs/reference/random/extending.html#cffi. So leaving that alone here. libnpyrandom isn't used by SciPy anymore and may well have zero users left, so it's not critical.

The effect this has is that symbols don't get re-exported as public
by Python extension modules that link with these shared libraries.
E.g., building on macOS arm64 with Clang changes the set of exported
symbols for `_multiarray_umath` from:

```
% dy2ld_info -exports build/numpy/_core/_multiarray_umath.cpython-39-darwin.so
build/numpy/_core/_multiarray_umath.cpython-39-darwin.so [arm64]:
    -exports:
        offset      symbol
        0x00112BA4  _PyInit__multiarray_umath
        0x001C955C  _npy_spacingf
        0x001C95F8  _npy_spacing
        ...
        <many more _npy symbols>
```
to:
```
  build/numpy/_core/_multiarray_umath.cpython-311-darwin.so [arm64]:
    -exports:
        offset      symbol
        0x0010B8F8  _PyInit__multiarray_umath
```

This works for all compilers that support GNU-style
`__attribute__((visibility("hidden"))`, which both GCC and Clang do.

Note that the `libnpyrandom` static library is left alone here. Trying
to change visibility there breaks a test for CFFI, because that test
is accessing private symbols. This is clearly wrong, but explicitly
documented at
https://numpy.org/devdocs/reference/random/extending.html#cffi. So
leaving that alone here. `libnpyrandom` isn't used by SciPy anymore
and may well have zero users left, so it's not critical.
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

LGTM, is highway part of NumPy 2 and we should backport it? OTOH, I guess it isn't really high priority and if someone ends up relying on it by accident it isn't the end of the world.

@rgommers
Copy link
Member Author

Highway is included in the 2.0 branch indeed: https://github.com/numpy/numpy/tree/maintenance/2.0.x/numpy/_core/src.

Backporting is a toss-up I'd say. It may be useful to pull it in, since we're doing another RC. It's not critical though, and it's conceivable (if pretty unlikely) that some package is relying on private symbols being exported by some other package or extension module somehow. So I'm happy either way.

@rgommers
Copy link
Member Author

Thanks for the review @seberg. Going to hit the green button on this now that it's still fresh in memory, so we can use it in SciPy once the new nightlies are available.

@rgommers rgommers merged commit 9022b0e into numpy:main Apr 19, 2024
65 checks passed
@rgommers rgommers deleted the staticlib-hiddensymbols branch April 19, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
36 - Build Build related PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants