git: f7cd7fe51c41 - sys/contrib/zstd: Import zstd 1.4.8

[Conrad Meyer]

On Sun, Jan 3, 2021 at 4:37 AM Ulrich Spörlein <uspoerlein at gmail.com> wrote:
> This broke the kernel-toolchain CI on linux and macos systems, most
> importantly the Github CI actions. They were nerfed due to the master
> -> main switch, but I've bisected this to the merge commit that
> introduces the build failure.

Thanks for investigating it.

> sys/contrib/zstd/lib/compress/zstd_compress_internal.h ends like so
> with this import:
>  +/** ZSTD_cycleLog() :
>  + *  condition for correct operation : hashLog > 1 */
>  +#ifdef        __FreeBSD__     /* This symbol is needed by dll-linked
> CLI zstd(1). */
>  +ZSTDLIB_API U32 ZSTD_cycleLog(U32 hashLog, ZSTD_strategy strat);
>  +#endif
>
> Why would this be specific to FreeBSD as the host OS?

In short: it isn't (wasn't).  The intention was to mark the
FreeBSD-specific modification.  (The modification was not the
definition itself, which is present in 1.4.5, but the ZSTDLIB_API
annotation.)  At the time, the tree was not built on non-FreeBSD
platforms.

It was introduced in the v1.4.5 import: 37f1f2684f26 (r361426), not
v1.4.8.  The intention was to expose the symbol in our
libprivatezstd.so, so that zstd(1) could link it.  Upstream libzstd
builds with -fvisibility=hidden, and ZSTDLIB_API is
__attribute__((visibility("default"))) on GCC-like platforms (i.e.,
Clang).

Why does this show up in 1.4.8?  I'm not sure.  In 1.4.5, the symbol
was only defined in libzstd, but never used.  However, it was used in
zstd(1).  In 1.4.8, it is also used in libzstd: both in
zstdmt_compress.c and in zstd_compress.c (though, after the
definition).  So apparently our non-FreeBSD CI build either skips
zstd(1), or allows invoking implicit undeclared functions in zstd(1).
But not in libzstd.

Despite it being a private symbol, zstd(1) uses it for the flag
mentioned in the 1.4.5 commit (and in v1.4.5: *only* in zstd(1)):

$ nm -D /usr/bin/zstd  | grep cycleLog
                 U ZSTD_cycleLog

I.e., upstream zstd build broke shared-linked zstd(1) in 1.4.5 (or
maybe earlier, but this was a new breakage for us).  See
https://github.com/facebook/zstd/issues/1680 ,
https://github.com/facebook/zstd/issues/1854 .  See also:
https://github.com/facebook/zstd/blob/f2ac2b7bcf6294f1c3a8d62b5f2ca78cfc6fb980/programs/Makefile#L291-L302

Now, we do not actually appear to build libprivatezstd with -fprivate,
so in fact symbols declared in zstd_compress_intenal.h without
ZSTDLIB_API are all visible anyway.  E.g.,

$ nm -D /usr/lib/libprivatezstd.so.5 | grep ZSTD_writeLastEmptyBlock
000000000001ca60 T ZSTD_writeLastEmptyBlock

But maybe it would be nice to restore -fvisibility=hidden to match
upstream eventually.

In the short term, I think this should fix CI:

--- a/sys/contrib/zstd/lib/compress/zstd_compress_internal.h
+++ b/sys/contrib/zstd/lib/compress/zstd_compress_internal.h
@@ -1198,8 +1198,9 @@

 /** ZSTD_cycleLog() :
  *  condition for correct operation : hashLog > 1 */
-#ifdef __FreeBSD__     /* This symbol is needed by dll-linked CLI zstd(1). */
-ZSTDLIB_API U32 ZSTD_cycleLog(U32 hashLog, ZSTD_strategy strat);
-#endif
+/* Begin FreeBSD - This symbol is needed by dll-linked CLI zstd(1). */
+ZSTDLIB_API
+/* End FreeBSD */
+U32 ZSTD_cycleLog(U32 hashLog, ZSTD_strategy strat);

 #endif /* ZSTD_COMPRESS_H */

Best,
Conrad