Fix MSVC C2159: duplicate extern from GGML_API on Windows

[sroller]

Overview

GGML_API already expands to '__declspec(dllexport/dllimport) extern' on Windows shared builds, so the explicit extern on turbo3_cpu_wht_group_size produced 'extern extern', which MSVC rejects. GCC's visibility-attribute expansion of GGML_API doesn't include extern, which is why this built fine on Linux. Made the extern conditional so both platforms get the semantics the original comment intended.

Additional information

compiled and tested on Windows 11 using MSVC 19.44.35227 for x64

Requirements

• I have read and agree with the contributing guidelines

• AI usage disclosure: fix created with information from Claude.ai online

[TheTom]

Thanks @sroller, and thanks for testing on real MSVC and saying which version. Your diagnosis is exactly right and it is my bug: I added that extern in the ODR fix without checking what GGML_API already expands to on the Windows shared path, so __declspec(dllexport) extern plus an explicit extern gives extern extern and MSVC is correct to reject it with C2159. On GCC and Clang it is only -Wduplicate-decl-specifier, which is why nobody caught it here. Confirmed that asymmetry locally.

Before this goes in, I ran the patch through all four build configurations by expanding the real macro from ggml.h against your condition. Two of the four do not come out right:

static (no GGML_SHARED)           => extern extern int turbo3_cpu_wht_group_size;
shared ELF (linux/mac)            => __attribute__((visibility("default"))) extern int ...;
shared MSVC (win, not mingw)      => __declspec(dllexport) extern int ...;
shared MinGW (win + __MINGW32__)  => __attribute__((visibility("default"))) int ...;

MinGW loses the extern entirely. Your condition is defined(_WIN32) && defined(GGML_SHARED), but the macro's Windows branch is guarded by defined(_WIN32) && !defined(__MINGW32__). MinGW defines _WIN32, so your test picks the no-explicit-extern arm while the macro picks the visibility arm, which has no extern. That turns the line back into a second definition, which is precisely the bug the comment directly above it exists to prevent. The two conditions have to agree on __MINGW32__ or they will disagree exactly there.

Static builds still produce extern extern. With BUILD_SHARED_LIBS=OFF there is no GGML_SHARED, GGML_API is plain extern, and your condition is false, so the explicit extern is still emitted. That is a supported configuration, so MSVC static builds would still hit C2159. Worth checking, since your report says you built shared.

Minor, and harmless here: the condition uses bitwise & rather than &&. Since defined() yields 0 or 1 it evaluates the same, so this is style rather than a defect, but && is what is meant.

Suggested alternative

Rather than adding a condition that has to be kept in sync with the macro, put the extern into the macro's visibility branch and leave the declaration plain:

 #    else
-#        define GGML_API __attribute__ ((visibility ("default")))
+#        define GGML_API __attribute__ ((visibility ("default"))) extern
 #    endif

-GGML_API extern int turbo3_cpu_wht_group_size;
+GGML_API int turbo3_cpu_wht_group_size;

Same expansion test on that version:

static (no GGML_SHARED)           => extern int turbo3_cpu_wht_group_size;
shared ELF (linux/mac)            => __attribute__((visibility("default"))) extern int ...;
shared MSVC (win, not mingw)      => __declspec(dllexport) extern int ...;
shared MinGW (win + __MINGW32__)  => __attribute__((visibility("default"))) extern int ...;

All four are a correct declaration, no duplication anywhere, and there is nothing to keep in sync later.

One wart to be aware of if you take that route: the definition site in ggml-turbo-quant.c is GGML_API int turbo3_cpu_wht_group_size = 0;, which then expands to extern int ... = 0. That is still a valid definition in both C and C++, but it warns under -Wextern-initializer. Dropping GGML_API from the definition line, or leaving it and accepting the warning, both work.

Heads up on a collision

This exact change is already in flight. PR #289 and #291 carry the macro-side fix above, and correspondingly change ops.cpp back to GGML_API int. So whichever lands second will conflict. I would rather your version go in, since it is standalone and reviewed on real MSVC hardware, which I cannot test on. If you are willing to switch to the macro approach, I will ask that the equivalent hunk be dropped from the stack so this PR is the single source of the fix.

Either way, thanks for finding this. It is a genuine portability bug I introduced, and the report was well diagnosed.

[sroller]

Great! I'm going to make the suggested change and resubmit the PR later tonight. Do I understand correctly that you don't have a Windows platform to run the tests?
I have msys64/MinGW64 on the same machine but because I ususally compile for CUDA I haven't built this branch on it. I could put this into my routine and provide feedback when necesary.

[TheTom]

Thanks for the rebase @sroller. I hope you do not mind, I pushed a0d29ddb8 onto your branch switching to the macro-side fix, since Tom asked me to get the outstanding PRs moving today. Happy to drop it if you would rather do it yourself.

The reason for changing approach rather than keeping the conditional is the two gaps I mentioned. Here is the expansion of your version against the real ggml.h, all five configurations:

static                 extern extern int ...                      <- still C2159 on MSVC static
shared ELF             visibility(default) extern int ...          ok
shared MSVC dllexport  __declspec(dllexport) extern int ...        ok, your fix
shared MSVC dllimport  __declspec(dllimport) extern int ...        ok, your fix
shared MinGW           visibility(default) int ...                 <- extern lost, second definition

MinGW is the awkward one: it defines _WIN32, so defined(_WIN32) && defined(GGML_SHARED) is true and takes the no-explicit-extern arm, but GGML_API itself is guarded by defined(_WIN32) && !defined(__MINGW32__) and takes the visibility arm, which has no extern. The two conditions disagree exactly there, and the line becomes a second definition again, which is the bug the comment above it exists to prevent.

After the change, same five configurations:

static                 extern int turbo3_cpu_wht_group_size;
shared ELF             __attribute__((visibility("default"))) extern int ...
shared MSVC dllexport  __declspec(dllexport) extern int ...
shared MSVC dllimport  __declspec(dllimport) extern int ...
shared MinGW           __attribute__((visibility("default"))) extern int ...

Correct declaration on every path, and nothing at the use site that has to be kept in sync with the macro later.

Two practical notes:

• This also matches what Pr/metal vulkan v1 #289 and Pr/prefetch weights #291 already carry, so those will no longer conflict with this PR. Whichever lands first, the other is a no-op rather than a merge conflict.
• The definition site in ggml-turbo-quant.c becomes extern int ... = 0, which is still a valid definition in both C and C++ but warns under -Wextern-initializer. I left it as is rather than widen the diff; if it turns up in a -Werror job it is a one-line change to drop GGML_API there.

I still cannot test MSVC, no Windows machine here, so please do re-run your original repro against this version. I verified the preprocessor expansion for all five configurations and that a full macOS build stays clean, but your actual MSVC 19.44 build is the thing that matters and I would rather you confirm it than take my word.

Sorry again for the noise on your first contribution here. The diagnosis was correct and the bug was mine to begin with.

[TheTom]

@sroller apologies, I rewrote this branch's commit metadata and force-pushed without asking first. That was my call to make only about my own commit and I overstepped by doing it on your branch.

What changed: the author email on my commit only, which was wrongly attributed to a work address. Your commit b1ae3af7c is untouched and still authored by you. The tree is byte-identical, so there is no content change whatsoever, and I used --force-with-lease pinned to the prior SHA so a concurrent push from you would have been rejected rather than clobbered.

If you have this branch locally, the SHAs moved:

a0d29ddb8  ->  faaf200de
git fetch origin && git reset --hard origin/fix/msvc-ggml-api-duplicate-extern

Nothing else about the PR changed. It still needs your MSVC 19.44 repro against the macro-side version, which is the part I cannot test from here.

Sorry for the noise on what is your first contribution to this repo.

[TheTom]

Correct, sroller — no Windows machine here at all. Everything I claimed about the MSVC and MinGW behaviour came from expanding the real ggml.h macro through the preprocessor on macOS with -D_WIN32 and friends, which tells you what the declaration becomes but proves nothing about whether MSVC accepts it.

Your msys64/MinGW64 setup would be genuinely useful, and more than the MSVC side. MinGW is the exact configuration where the original conditional went wrong: it defines _WIN32, so defined(_WIN32) && defined(GGML_SHARED) was true and took the no-explicit-extern arm, while GGML_API itself is guarded by defined(_WIN32) && !defined(__MINGW32__) and took the visibility arm, which has no extern. The declaration quietly became a second definition again.

So a MinGW shared build (-DBUILD_SHARED_LIBS=ON) is the direct test of the thing I claimed and cannot check. If it links, the macro-side fix is doing its job on the path that mattered. Static MSVC is the other one worth a pass, since that config still emitted extern extern before the change.

Adding it to your routine would be valuable beyond this PR. The fork has repeatedly shipped code that nobody compiled on a given backend, which is how the HIP quality gate went its entire life without running, so a standing Windows datapoint is worth having.

[TheTom]

Correction to what I said above: we do have a Windows machine, an RTX 3090 box. It is powered down at the moment, so I could not use it for this PR, but it is not true that there is no Windows here and I should not have put it that way.

It should be able to cover both configurations that matter for this change once it is up: MinGW64 shared, which is where the original conditional silently dropped the extern, and MSVC static, which still emitted extern extern before the macro-side fix. I will run those and report back rather than leaving it on you.

Your offer to add Windows to your routine still stands on its own merits, since one machine that is sometimes off is not coverage. But you should not feel obliged to carry this particular verification.

[TheTom]

The Windows box is up, so I ran what I said I would rather than leaving it with you. MSVC 19.28.29915, VS 2019 Community, x64. Building only ggml-base and ggml-cpu, since that is where the definition and the declaration live.

Result on your branch with the macro-side change (faaf200de):

BUILD_SHARED_LIBS=OFF   exit 0
BUILD_SHARED_LIBS=ON    exit 0

And the control, your original commit b1ae3af7c, same compiler, same configs:

BUILD_SHARED_LIBS=OFF   error C2159: more than one storage class specified   (ops.cpp:25)
BUILD_SHARED_LIBS=ON    exit 0

So the static gap I claimed is real and now measured rather than argued from preprocessor expansion. Your conditional fixes the shared build, which is what your C2159 report was about, and the static build still failed underneath it. The macro-side version passes both.

One correction on my own method, because it nearly produced a false all-clear. My first control run came back exit 0 and I almost reported that your version was fine on static. It was not: I had built only the ggml-base target, and the extern extern is in ggml-cpu/ops.cpp, which that target never compiles. The control was watching a build that could not have contained the bug. Rebuilding with --target ggml-cpu produced the C2159 above. Worth mentioning since it is exactly the kind of thing that makes a verification look conclusive when it is empty.

What I still have not covered: MinGW. That box has no msys64 and no gcc, only MSVC, so I cannot test it here. MinGW is the configuration where your original conditional silently dropped the extern entirely and turned the declaration back into a second definition, which is a quieter failure than C2159 because it links and then misbehaves. If you do get to a MinGW64 shared build, that is the one remaining unverified path and the one I would most want a result from.

Also worth saying plainly: my MSVC is 19.28 and yours is 19.44. C2159 is conformance behaviour so I would expect it to reproduce on both, but the versions differ and I would not want to claim your compiler is covered by mine.

Thanks for the report and for chasing it into the right place. It was my bug, and the diagnosis in your first message was correct.

[TheTom]

MinGW is covered now. I installed MSYS2 on that box, so gcc 16.2.0, and ran the case neither of us had.

This is the one where a passing build proves nothing, so the check is the symbol table rather than the exit code. Both configurations build and link with no error at all:

FIXED (this branch)
  ggml-turbo-quant.c.obj:   B turbo3_cpu_wht_group_size    definition
  ops.cpp.obj:              U turbo3_cpu_wht_group_size    reference only

CONTROL (b1ae3af7c, your original)
  ggml-turbo-quant.c.obj:   B turbo3_cpu_wht_group_size    definition
  ops.cpp.obj:              B turbo3_cpu_wht_group_size    second definition

B is a definition in BSS, U is an undefined reference. So on MinGW shared, the pre-fix version really does turn the declaration back into a second definition, exactly as predicted, and nothing complains. Each DLL ends up with its own copy: the CPU SET_ROWS handler writes one, quantize_row_turbo{2,3}_0_ref reads the other, and the group size silently stays on its (k%128==0) ? 128 : 64 fallback. That is a worse failure than the MSVC one precisely because it ships.

Full picture across every configuration now:

	pre-fix	fixed
MSVC static	error C2159	builds
MSVC shared	builds	builds
MinGW shared	builds, two definitions	builds, one definition

So your original conditional fixed the loud case and left both quiet ones. The macro-side version is correct on all three.

Two things I want to be straight about. My MSVC is 19.28 and yours is 19.44, so your compiler still is not literally covered by mine, though C2159 is conformance behaviour and I would expect it to match. And this is gcc 16.2.0 from MSYS2, which is not the only MinGW distribution in the world.

Given that, I am happy to merge this whenever you are ready, or leave it for you to run your own repro first if you would rather confirm on your setup. Your call, it is your PR.

[sroller]

I pulled the pr 292 and was able to compile on my windows machine.
I tried the same with my Msys64/MinGW system but I ran into problems. I need to investigate how to setup a proper dev environment. CUDA might not be supported on MinGW anyway, at least not officially.

I'm a bit confused about the process because I've never contributed on Github to a larger project. My major version management tools were RCS and VSS :-).
Please tell me, what you want me to do now.

BTW: I just found out that this fork is so amazingly fast on my limited hardware (RTX5080) because the MoE cache has been implemented, Chapeau!

[sroller]

Re-reading this in the morning: Please go ahead and merge.