m68k-unknown-linux-gnu: zstd-1.4.9 test suite fails badly

[akhuettel]

I've been trying to build zstd on m68k-unknown-linux-gnu (since it is a dependency of the Gentoo package manager). (More precisely, a qemu-user chroot, but that should not have too much impact here.)

Unfortunately, while the build itself runs fine and the command line utility seems to start, the results in the zstd-1.4.9 testsuite look rather messy.
(1.5.0 does not work at all, bailing out with a "generic error").

I've attached the test log. Advice on how serious this is would be appreciated...

build.log

[Cyan4973]

We have never tested zstd on such a platform.
And to be honest, I'm impressed v1.4.9 runs so well on this first attempt.

Beyond that, it's too difficult to debug. There are myriads of things that could go wrong at runtime, especially in the CLI which features a number of system calls beyond the library itself (I expect libzstd to be very portable, but the CLI application is another story).

To ensure libzstd and zstd work on m68k, we would need to add this target to our CI.
This might be possible if it's an available target of qemu-static.

[chewi]

It is indeed a user (and system) target of QEMU.

[Cyan4973]

OK, I could reproduce a qemu-m68k config locally,
and indeed, there is a problem at some point during the test :

../lib/compress/zstd_compress.c: Building LL table 
../lib/compress/zstd_compress_sequences.c: Selected set_compressed 
../lib/compress/zstd_compress_sequences.c: ZSTD_buildCTable (dstCapacity=113538) 
../lib/compress/zstd_compress_sequences.c:281: ERROR!: forwarding error in FSE_buildCTable_wksp(nextCTable, wksp->norm, max, tableLog, wksp->wksp, sizeof(wksp->wksp)): Error (generic): 
../lib/compress/zstd_compress.c: ZSTD_buildCTable for LitLens failed 
../lib/compress/zstd_compress.c:2654: ERROR!: forwarding error in stats.size: Error (generic): ZSTD_buildSequencesStatistics failed!
../lib/compress/zstd_compress.c:2711: ERROR!: forwarding error in cSize: Error (generic): ZSTD_entropyCompressSeqStore_internal failed

So it's not especially trivial, and I'm unable to tell what's wrong just from the traces, but at least, it's reproducible. So there's a likely way forward.

[Cyan4973]

I get it,
it's a (presumed) alignment error.

The code expects to be run on 32-bit CPUs,
therefore, when a structure contains 32-bit fields,
it expects this structure to be aligned on 32-bit boundaries.

So it checks this condition, and errors out if it's not respected.

But apparently, on m68k, this assumption is incorrect.
The intended buffer is only aligned on 2-bytes boundaries.

I presume that m68k is still, at its roots, a 16-bit cpu, and only aligns 32-bit fields on 16-bit boundaries ?
Same way as x86 in 32-bit mode aligns 64-bit long long field on 32-bit boundaries ?

Anyway, this is likely fixable, just need to come up with a proper compatible policy.

[Cyan4973]

OK, so that alignment control was the issue.
Fixing that case makes the test go farther,
though it then fails later.

Yet another case of alignment.
zstd_cwksp.h:426: ZSTD_cwksp_reserve_object: Assertion '(bytes & (sizeof(void*)-1)) == 0' failed.

I have a hunch that most if not all problems found during m68k tests will be alignment related.

[chewi]

Thanks so much for taking this seriously. Hopefully there isn't too many of these to fix. I tried your branch out of interest and got the same result.

If it helps, a similar change was recently made to Python in python/cpython#24624.

[chewi]

I thought I'd have a little stab at this myself. Believe it or not, this was enough to get the test suite to pass. I don't fully understand the changes though so this is probably not the correct fix. 😅

diff --git a/lib/compress/huf_compress.c b/lib/compress/huf_compress.c
index 67b7ef95..f9f2366f 100644
--- a/lib/compress/huf_compress.c
+++ b/lib/compress/huf_compress.c
@@ -97,7 +97,7 @@ static size_t HUF_compressWeights(void* dst, size_t dstSize, const void* weightT
 
     unsigned maxSymbolValue = HUF_TABLELOG_MAX;
     U32 tableLog = MAX_FSE_TABLELOG_FOR_HUFF_HEADER;
-    HUF_CompressWeightsWksp* wksp = (HUF_CompressWeightsWksp*)HUF_alignUpWorkspace(workspace, &workspaceSize, sizeof(U32));
+    HUF_CompressWeightsWksp* wksp = (HUF_CompressWeightsWksp*)HUF_alignUpWorkspace(workspace, &workspaceSize, sizeof(U16));
 
     if (workspaceSize < sizeof(HUF_CompressWeightsWksp)) return ERROR(GENERIC);
 
@@ -176,7 +176,7 @@ size_t HUF_writeCTable_wksp(void* dst, size_t maxDstSize,
     HUF_CElt const* const ct = CTable + 1;
     BYTE* op = (BYTE*)dst;
     U32 n;
-    HUF_WriteCTableWksp* wksp = (HUF_WriteCTableWksp*)HUF_alignUpWorkspace(workspace, &workspaceSize, sizeof(U32));
+    HUF_WriteCTableWksp* wksp = (HUF_WriteCTableWksp*)HUF_alignUpWorkspace(workspace, &workspaceSize, sizeof(U16));
 
     /* check conditions */
     if (workspaceSize < sizeof(HUF_WriteCTableWksp)) return ERROR(GENERIC);
@@ -683,7 +683,7 @@ static void HUF_buildCTableFromTree(HUF_CElt* CTable, nodeElt const* huffNode, i
 
 size_t HUF_buildCTable_wksp (HUF_CElt* CTable, const unsigned* count, U32 maxSymbolValue, U32 maxNbBits, void* workSpace, size_t wkspSize)
 {
-    HUF_buildCTable_wksp_tables* const wksp_tables = (HUF_buildCTable_wksp_tables*)HUF_alignUpWorkspace(workSpace, &wkspSize, sizeof(U32));
+    HUF_buildCTable_wksp_tables* const wksp_tables = (HUF_buildCTable_wksp_tables*)HUF_alignUpWorkspace(workSpace, &wkspSize, sizeof(U16));
     nodeElt* const huffNode0 = wksp_tables->huffNodeTbl;
     nodeElt* const huffNode = huffNode0+1;
     int nonNullRank;
diff --git a/lib/compress/zstd_cwksp.h b/lib/compress/zstd_cwksp.h
index 2656d26c..3617e466 100644
--- a/lib/compress/zstd_cwksp.h
+++ b/lib/compress/zstd_cwksp.h
@@ -423,7 +423,7 @@ MEM_STATIC void* ZSTD_cwksp_reserve_object(ZSTD_cwksp* ws, size_t bytes) {
         "cwksp: reserving %p object %zd bytes (rounded to %zd), %zd bytes remaining",
         alloc, bytes, roundedBytes, ZSTD_cwksp_available_space(ws) - roundedBytes);
     assert(((size_t)alloc & (sizeof(void*)-1)) == 0);
-    assert((bytes & (sizeof(void*)-1)) == 0);
+    assert((bytes & (sizeof(void*)-1)) % 2 == 0);
     ZSTD_cwksp_assert_internal_consistency(ws);
     /* we must be in the first phase, no advance is possible */
     if (ws->phase != ZSTD_cwksp_alloc_objects || end > ws->workspaceEnd) {

[Cyan4973]

I think it clarifies that m68k issues are entirely concentrated in the alignment policy.

And while this fix is likely correct for m68k, it's obviously not universally portable,
so we'll have to find something proper that scales from m68k to IBM Z.

[glaubitz]

CC @jrtc27

[jrtc27]

Use alignof or similar (_Alignof in C11, stdalign.h gives you an alignof alias, and __alignof__/__alignof has long existed as a GNU extension in both GCC and Clang, with MSVC also supporting __alignof)