build: zstd with assembly decompression routines won't build on linux

[jbowens]

In cockroachdb/pebble#2668 we upgraded the version of Pebble's dependency on this zstd package. This builds in Pebble's CI across platforms, locally on MacOS and in a Linux gceworker. An attempt to bump the Pebble version of the cockroach repository was made in #105473. While cockroach builds on that branch on MacOS, it fails on linux (both in CI & on a gceworker):

/tmp/rules_go_work-3841712515/cgo/github.com/DataDog/zstd/_x16.o:huf_decompress.c:function HUF_decompress4X1_usingDTable_internal: error: undefined reference to 'HUF_decompress4X1_usingDTable_internal_fast_asm_loop'
/tmp/rules_go_work-3841712515/cgo/github.com/DataDog/zstd/_x16.o:huf_decompress.c:function HUF_decompress4X2_usingDTable_internal: error: undefined reference to 'HUF_decompress4X2_usingDTable_internal_fast_asm_loop'

These symbols appear to be defined here:
https://github.com/DataDog/zstd/blob/ea68dcab66c0fe39989a14c4b6acc520abf80231/huf_decompress_amd64.S#L355
https://github.com/DataDog/zstd/blob/ea68dcab66c0fe39989a14c4b6acc520abf80231/huf_decompress_amd64.S#L99C1-L99C53

However these symbols are marked 'hidden' for ELF targets for an unknown reason:
https://github.com/DataDog/zstd/blob/ea68dcab66c0fe39989a14c4b6acc520abf80231/portability_macros.h#L69-L74

Relevant internal thread.

Jira issue: CRDB-29108

[jbowens]

cc @rickystewart, @bananabrick

[jbowens]

I reverted the dependency upgrade for now in cockroachdb/pebble#2679.

[pkieltyka]

hi all -- loving the work on cockroachdb + pebble. We use pebble is many of our production applications with great success. We'd love to see this zstd ticket solved so it can unblock pebble to be use zstd without crashing. I'm putting up a bounty on this ticket, for anyone who solves this ticket by Jan 1, 2024 I will offer $2k USD bounty.

[jbowens]

@kenliu-crl, @rickystewart do you think we can schedule work on this soon? This may help reduce storage costs of customers and improve TCO if we can unblock use of zstd.

[kenliu-crl]

Hi @jbowens and @itsbilal let's discuss this and see if we can get this planned for the 24.2 release cycle.

[jbowens]

@kenliu-crl Okay. I'm not sure if this is even large enough to warrant a roadmap item, but I don't know much about the build system at all (:

[jbowens]

I've staged a rebased version of the Pebble changes here: cockroachdb/pebble#3416.

And here's a cockroach branch attempting to use those Pebble changes: master...jbowens:cockroach:zstd-debug

[jbowens]

Is there a means to pass C define flags to a dependency? A workaround we can pursue is disabling the assembly routines. This can be done with a minimal patch:

diff -urN a/portability_macros.h b/portability_macros.h
index ecf5453..1413020 100644
--- a/portability_macros.h
+++ b/portability_macros.h
@@ -115,6 +115,8 @@
 #  define ZSTD_ASM_SUPPORTED 0
 #endif

+#define ZSTD_DISABLE_ASM 1
+
 /**
  * Determines whether we should enable assembly for x86-64
  * with BMI2.

but I imagine there's a way to pass it through bazel?

[rickystewart]

Is there a means to pass C define flags to a dependency?

Yes, we would probably use copts, as in copts = ["-DZSTD_DISABLE_ASM=1"] (docs). That would probably be done as a patch to the dependency (see others in build/patches).

[jbowens]

Glad we found a workaround for 24.1 as the benefit (#120784) is significant. Repurposing this issue to be specifically enabling the use of zstd's assembly decompression routines which likely provide a small performance edge on x86 over the C routines we're using now

[nicktrav]

Linking this as a blocker for #123953.