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

pull real-or-random wants to merge 1 commits into bitcoin-core:master from real-or-random:202408-win-visibility changing 1 files +12 −2

real-or-random commented at 11:49 pm on August 24, 2024: 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 added the label build on Aug 24, 2024
real-or-random force-pushed on Aug 25, 2024
real-or-random force-pushed on Aug 25, 2024
fanquake commented at 10:13 am on September 6, 2024: member

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 commented at 7:16 pm on September 9, 2024: contributor

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 commented at 8:45 am on September 10, 2024: 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.
include: Avoid visibility("default") on Windows
Fixes #1421.
447334cb06
real-or-random force-pushed on Sep 17, 2024
real-or-random commented at 2:23 pm on September 17, 2024: contributor

Okay. I’ve nevertheless rephrased the comment to make it (hopefully) clearer.
fanquake commented at 12:21 pm on October 15, 2024: member

ACK 447334cb06de229024161021cc79b3af32ce8b5c
real-or-random commented at 2:01 pm on October 15, 2024: contributor

@hebasto Wanna review this? :)
fanquake commented at 8:37 am on October 16, 2024: member

also cc @theuni
in include/secp256k1.h:151 in 447334cb06
146@@ -147,6 +147,15 @@ typedef int (*secp256k1_nonce_function)( 147 * 1. If using Libtool, it defines DLL_EXPORT automatically. 148 * 2. In other cases, SECP256K1_DLL_EXPORT must be defined. */ 149 # define SECP256K1_API extern __declspec (dllexport) 150+# else 151+ /* Building libsecp256k1 as a static library on Windows.
hebasto commented at 2:33 pm on October 16, 2024:

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.

real-or-random commented at 2:14 pm on October 17, 2024:

You mean https://github.com/bitcoin-core/secp256k1/blob/master/src/CMakeLists.txt#L89, right? True, and it’s also true for bench_internal and bench_ecmult.

Would you prefer “Building libsecp256k1 not as a DLL.”? (I also dropped the Windows part because it’s anyway only Windows in the outer #ifdef.)

hebasto commented at 4:20 pm on October 17, 2024:

You mean https://github.com/bitcoin-core/secp256k1/blob/master/src/CMakeLists.txt#L89, right? True, and it’s also true for bench_internal and bench_ecmult.

Would you prefer “Building libsecp256k1 not as a DLL.”? (I also dropped the Windows part because it’s anyway only Windows in the outer #ifdef.)

Looks good. However, I’m not sure if it worth invalidating all ACKs.

real-or-random commented at 4:02 pm on October 21, 2024:

Looks good. However, I’m not sure if it worth invalidating all ACKs.

Ok, let’s just get it merged.
hebasto approved

hebasto commented at 2:33 pm on October 16, 2024: member

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

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

theuni approved
theuni commented at 3:59 pm on October 16, 2024: contributor

ACK 447334cb06de229024161021cc79b3af32ce8b5c
real-or-random merged this on Oct 21, 2024
real-or-random closed this on Oct 21, 2024

Contributors
real-or-random fanquake hebasto theuni

Labels
build