Date: Sun, 3 Jan 2021 15:47:07 +0100 From: =?UTF-8?Q?Ulrich_Sp=C3=B6rlein?= <uspoerlein@gmail.com> To: cem@freebsd.org Cc: src-committers <src-committers@freebsd.org>, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: f7cd7fe51c41 - sys/contrib/zstd: Import zstd 1.4.8 Message-ID: <CAJ9axoSG161zMi3igZDMpuBfCDR9ObgqrR=D2mDp-NwDskRg_A@mail.gmail.com> In-Reply-To: <CAG6CVpVVxhVVpPpvhB6m8G=Z35wzpVUAHWN8Wpo8r54UwfrLmw@mail.gmail.com> References: <202012251556.0BPFu9l7029902@gitrepo.freebsd.org> <CAJ9axoQRp6fb7HTjKTWnm542ZE2wAZWX%2BhGyH8b37OBfK7pBTg@mail.gmail.com> <CAG6CVpVVxhVVpPpvhB6m8G=Z35wzpVUAHWN8Wpo8r54UwfrLmw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Thanks for the quick fix. Could you push that commit to your own freebsd
clone on GitHub? That should give you the CI results after a few minutes.
If not, I can get to it later and confirm whether that works.
On Sun, 3 Jan 2021, 15:30 Conrad Meyer, <cem@freebsd.org> wrote:
> On Sun, Jan 3, 2021 at 4:37 AM Ulrich Sp=C3=B6rlein <uspoerlein@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=3Dhidden, 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/f2ac2b7bcf6294f1c3a8d62b5f2ca78cfc6=
fb980/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=3Dhidden 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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ9axoSG161zMi3igZDMpuBfCDR9ObgqrR=D2mDp-NwDskRg_A>