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

autotools: allow --enable-symbol-hiding with windows #9586

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 25, 2022

This local autotools logic was put in place in 9e24b9c (in 2012) which disabled it for Windows unconditionally. Testing reveals that it actually works with tested toolchains (mingw-w64 and CI ones), so let's allow this build feature on that platform. Bringing this in sync with CMake, which already supported this.

Closes #9586

@vszakats vszakats marked this pull request as draft Sep 25, 2022
@vszakats
Copy link
Member Author

vszakats commented Sep 25, 2022

Tested fine with mingw-w64. Let's test with the legacy mingw and the others.

UPDATE 1: They passed.
UPDATE 2: --enable-symbol-hiding is also the default setting and it's not explicitly disabled in these tests, meaning the option was actually active.

@vszakats vszakats marked this pull request as ready for review Sep 25, 2022
This local autotools logic was put in place in
9e24b9c (in 2012) which disabled it for
Windows unconditionally. Testing reveals that it actually works with
tested toolchains (mingw-w64 and CI ones), so let's allow this build
feature on that platform. Bringing this in sync with CMake, which already
supported this.

Closes curl#9586
@vszakats vszakats force-pushed the autotools-windows-hidesymbols branch from 647d587 to 4e2a131 Compare Sep 25, 2022
@vszakats vszakats added build Windows Windows-specific labels Sep 25, 2022
vszakats added a commit to curl/curl-for-win that referenced this pull request Sep 25, 2022
Fix the upstream autotools option `--enable-symbol-hiding` on Windows,
then use that instead of our hand rolled solution which also involves
patching the build system.

This brings down the number of local patch two just 2. (Still two more
than ideal though.)

Ref: curl/curl#9586
@jay
Copy link
Member

jay commented Sep 25, 2022

But does it actually do anything on windows?

curl/include/curl/curl.h

Lines 112 to 126 in 93d0928

#ifdef CURL_STATICLIB
# define CURL_EXTERN
#elif defined(CURL_WIN32) || defined(__SYMBIAN32__) || \
(__has_declspec_attribute(dllexport) && \
__has_declspec_attribute(dllimport))
# if defined(BUILDING_LIBCURL)
# define CURL_EXTERN __declspec(dllexport)
# else
# define CURL_EXTERN __declspec(dllimport)
# endif
#elif defined(BUILDING_LIBCURL) && defined(CURL_HIDDEN_SYMBOLS)
# define CURL_EXTERN CURL_EXTERN_SYMBOL
#else
# define CURL_EXTERN
#endif

@vszakats
Copy link
Member Author

vszakats commented Sep 25, 2022

Sure, unless I'm fatally misunderstanding something. After the patch, libcurl.dll no longer exports every existing public function linked to it, only the curl API. This let me drop the manual solution using a pre-rolled .def file (as used in Makefile.m32 builds). Ref this curl-for-win patch: curl/curl-for-win@9b71378

The compiler macro is a no-op on Windows, but there is extra logic run around the linking phase, and it may also trigger some linker options. I didn't investigate the internals further.

@vszakats
Copy link
Member Author

vszakats commented Sep 25, 2022

jay
jay approved these changes Sep 25, 2022
Copy link
Member

@jay jay left a comment

there is extra logic run around the linking phase

ah ok

@vszakats vszakats closed this in 61c7cca Sep 25, 2022
@vszakats vszakats deleted the autotools-windows-hidesymbols branch Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Windows Windows-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants