meson: Deny shared build on MSVC compiler

[tesuji]

Apparently, zstd has never tested on MSVC 2019 and currently fall to build on that compiler.
It would make sense to gate Meson until there is proper support for that compiler.
cc #2358

This PR also fixes some minor correctness issues.

[o-mdr]

Looking at the output from meson compile -v -C builddir, I am not sure include_directories are actually passed in to the resource compiler.

Anyone else observes this issue?

[tesuji]

It will, but I have MSVC compiler error about syntax: https://github.com/lzutao/zstd/runs/1264653094#step:8:130.

[o-mdr]

Interesting, I cannot get beyond compiling resources on my system, whereas in your build it goes passed it and compiles the resource just fine.

That error is not about syntax, it's a linker error, basically it's saying that there were a couple of symbols observed in functions (ZSTD_cycleLog, ZSTD_XXH64, ZDICT_trainFromBuffer_unsafe_legacy) that were not actually defined. Could it be a missing source/header file or compiler/linker flag?

It's probably ok to compile it with MSVC compiler, just needs to get all the files. It seems like this used to work with previous versions of the compiler, so should work with the newer (maybe just missing some flags)

[tesuji]

That was a shared build. All the missing symbols were in zstd.lib, which was included the the link command.

There was also a static build, which has syntax error: https://github.com/lzutao/zstd/runs/1267569335#step:8:124.

[tesuji]

I am sorry for giving the wrong link.

[o-mdr]

Oh, so it is, not seen that one

[terrelln]

@lzutao It looks like min is defined as a macro, which is messing with the calls to std::min(). I am fairly certain the definition is coming from one of the windows includes from platform.h. Defining NOMINMAX in programs/platform.h, before the windows includes, should probably fix the issue.

[tesuji]

Thank you, @terrelln. Your way really fixes the static build.
The shared build still fall with unresolved symbols error. Hope you could take a look.

At worst case, I could disable shared build for msvc.

[ghost]

Could you add /Ob3 c compile option to MSVC2019?

Visual Studio 2019 has a new optimization option /Ob3, which turns on aggressive inlining.
https://docs.microsoft.com/en-us/cpp/build/reference/ob-inline-function-expansion?view=vs-2019

In zstd v1.4.5, just turn on this option, no code changed, run a test code: 6.97s -> 4.77s.
See issue #2314 for more information.

In the upcoming zstd v1.4.6, commit cc88eb7 accelerates from 6.97s to 4.85s, still a bit slower than 4.77s.

This option was added in MSVC2019, if use this option in MSVC2017, it will emit a warning:
cl : Command line warning D9002 : ignoring unknown option '/Ob3'

[tesuji]

@animalize I could. However is the same flag used anywhere else in the other build systems?
I don't want to use some custom optimization flags for meson only.
Also adding that flag would be better in another PR.

[ghost]

@animalize I could. However does the same flag used anywhere else in the other build systems?
I don't want to use some custom optimization flags for meson only.
Also adding that flag would be better in another PR.

Thanks.

A separated PR sounds reasonable.
The flag should not be used in other build systems. This problem was discovered last month.

[terrelln]

The shared build still fall with unresolved symbols error. Hope you could take a look.

Adding ZDICTLIB_VISIBILITY to this declaration should fix it.

zstd/lib/dictBuilder/zdict.c

Line 972 in 614e446

size_t ZDICT_trainFromBuffer_unsafe_legacy(

Like

ZDICTLIB_VISIBILITY size_t ZDICT_trainFromBuffer_unsafe_legacy(
                            void* dictBuffer, size_t maxDictSize,
                            const void* samplesBuffer, const size_t* samplesSizes, unsigned nbSamples,
                            ZDICT_legacy_params_t params);

[tesuji]

I need some help to resolve this error: https://github.com/facebook/zstd/pull/2363/checks?check_run_id=1275024168#step:3:1708

[Cyan4973]

I initially thought that the topic was about the shared dynamic library,
but apparently, it's larger than that:
it also applies to a variant of zstd CLI which would be linked against the dynamic library.

The zstd CLI is not designed to be linked to the standard dynamic library libzstd.
That's because it relies on private symbols, which are not exposed by default in the library.

For the CLI to link against a shared library, the library would need to expose basically all symbols,
the same way a static library does (zstd CLI links correctly against the static library).

But there is a reason we only expose selected symbols by default : the rest is considered "internal stuff", and may change at any moment. So it's not good to let applications believe they can depend on these accidental features.

I would suggest to not attempt delivering this feature through meson.

[tesuji]

Thanks for your explanation. I think Meson had a way to disable statically linking with the library. I've been away from my computer for a while. If anyone want to patch this issue, please don't hesitate.