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.

Sign up for GitHub

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

Jump to bottom

build: 45839th attempt to fix symbol visibility on Windows #1595

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

build: 45839th attempt to fix symbol visibility on Windows #1595

wants to merge 1 commit into from

Conversation

real-or-random
Copy link
Contributor

Fixes #1421. See code comments for rationale.

Related meta-bug: #1181. This reminds me that we should move forward with #1359.

@real-or-random real-or-random force-pushed the 202408-win-visibility branch 2 times, most recently from 5eb211a to 6077065 Compare August 25, 2024 00:10
@hebasto hebasto mentioned this pull request Aug 25, 2024
Open
@fanquake
Copy link
Member

fanquake commented Sep 6, 2024

I can at least confirm that this "fixes" the output I was seeing in #1421. The most recent comment on the upstream GCC issue is not 100% clear to me.

@real-or-random
Copy link
Contributor Author

The most recent comment on the upstream GCC issue is not 100% clear to me.

Can you elaborate on which part is unclear?

In any case, I could rephrase this such that it says it's a GCC bug. When I wrote it, I wasn't sure yet, but the upstream response seems to confirm that this is a bug.

@fanquake
Copy link
Member

I've re-read, and yes, it seems like the conclusion is that it is a GCC issue. I guess what is actually unclear is if/when anyone might fix it upstream.

@real-or-random
Copy link
Contributor Author

Okay. I've nevertheless rephrased the comment to make it (hopefully) clearer.

@fanquake
Copy link
Member

ACK 447334c

@real-or-random
Copy link
Contributor Author

@hebasto Wanna review this? :)

@fanquake
Copy link
Member

also cc @theuni

hebasto
hebasto approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK 447334c, tested on Ubuntu 24.04 using the following commands:

$ env CFLAGS="-flto" cmake -B build --preset dev-mode --toolchain cmake/x86_64-w64-mingw32.toolchain.cmake
$ cmake --build build -j 16 -t tests noverify_tests

include/secp256k1.h
@@ -147,6 +147,15 @@ typedef int (*secp256k1_nonce_function)(
* 1. If using Libtool, it defines DLL_EXPORT automatically.
* 2. In other cases, SECP256K1_DLL_EXPORT must be defined. */
# define SECP256K1_API extern __declspec (dllexport)
# else
/* Building libsecp256k1 as a static library on Windows.
Copy link
Member

@hebasto hebasto Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Referencing the use of a static library does not apply to our cases with the tests and noverify_tests targets using CMake, where object files are linked directly.

theuni
theuni approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK 447334c

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

@theuni theuni theuni approved these changes

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mingw-w64: "visibility attribute not supported in this configuration; ignored" under -flto
4 participants
@real-or-random @fanquake @theuni @hebasto