Skip to content

Conversation

@K20shores
Copy link

Description

This fixes #5883.

Really, I've just applied the patch that @baburton pasted in the mentioned issue
This checks whether we are compiling on Windows with ARM; if so, we export default visibility.

You can see passing actions that build on arm-windows in this successful github action in my scratch repository, so it appears to work

Suggested changelog entry:

Exports default visibility on ARM-based Windows builds

This is really a patch made by @baburton. This checks if we are
compiling on widows with arm and if so we export default
visibility.
@rwgk
Copy link
Collaborator

rwgk commented Dec 6, 2025

@K20shores @baburton do you think this change is a good & proper fix long term?

What's your opinion on the stale #4072?

Basically what I'm asking:

  • What's the best solution long-term in your opinions?
  • What's actually practical, and what are the trade-offs, short and long term?

I really have no idea at this minute.

How much trouble is it to add one Windows ARM job to the CI here?

@baburton
Copy link

baburton commented Dec 6, 2025

@K20shores @baburton do you think this change is a good & proper fix long term?

In my patch I add __attribute__((visibility("default"))) alongside the existing __declspec(dllexport) exactly when building under windows/arm64. This was an attempt to be as targeted as possible without affecting other platforms. However, on further thought I suspect what you actually want is to do this exactly when building under windows and using gcc-like compilers.

The reasons:

  • I have zero experience with MSVC++, but it's not clear to me that the Microsoft compiler supports __attribute__ at all (and I do see that other uses of __attribute__ in pybind11 are targeted to gcc/clang). In particular, pybind11 only applies hidden visibility by default when __GNUG__ is defined (at the bottom of pybind11_namespace_macros.h), so presumably this problem never appears under the Microsoft compiler anyway.
  • I don't think my patch is specific to arm64 (originally I was trying to be as precisely targeted as possible, to unbreak pybind11 on windows/arm64/clang without affecting other working platforms). I am no expert on how visibility flags interact with the DLL export manifest; however, it would seem to me that if an entity is in the DLL export manifest (i.e., part of the public DLL interface) then surely it should be visible. So I cannot see the harm in allowing this patch to apply to all windows gcc-like builds.

So: I expect that where my patch used #if defined(__aarch64__), what you really want is #if defined(__GNUG__).

This would expand the patch to cover windows/x64 builds that use msys2 with gcc or clang. Unfortunately I cannot test this myself, since msys2 does not install on my windows x64 VMs. From my past experience, windows/x64/gcc was already working, and I had never tried windows/x64/clang. (The build failure being discussed here was on windows/arm64/clang, and msys2 does not provide a windows/arm64/gcc option at all).

What's your opinion on the stale #4072?

Issue #4072 appears to be about widespread changes to the visibility attributes used in pybind11 - for this I have no informed opinion I'm afraid (I don't know enough about the pros, cons and behaviour of different visibility flags under different linkers on different platforms).

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.

[BUG]: Visibility is incorrectly exported on ARM based windows

3 participants