On Fri, Mar 03, 2023 at 09:42:42AM +0000, Stuart Henderson wrote: > On 2023/03/03 06:49, Bjorn Ketelaars wrote: > > On Fri 03/03/2023 01:55, Theo Buehler wrote: > > > zstd is broken on sparc64 due to unaligned accesses resulting in bus > > > errors. Since __GNUC__ is defined and MEM_FORCE_MEMORY_ACCESS isn't > > > defined, it defaults to 1, and we run into these: > > > > > > https://github.com/facebook/zstd/blob/1be95291a89160be121c987c2e385331a65a4a0e/lib/common/mem.h#L192-L199 > > > > > > One simple fix is to define MEM_FORCE_MEMORY_ACCESS to 0 via CPPFLAGS. > > > This forces all platforms to use memcpy, which may or may not result in > > > some slowdown. We can consider more elaborate platform-specific fixes if > > > that is desired, but then I need instructions. > > > > > > With this diff, the cmake test for zstd doesn't abort with a bus error, > > > which allows me to build qt6/qtbase on sparc64, and in turn we should > > > get a sizable chunk of the tree back to building. > > So it's from this commit > > https://github.com/facebook/zstd/commit/a78c91ae59b9487fc32224b67c4a854dc3720367
This diff isn't a clean revert, of course. > > I did some light benchmarking using your patch and first impression, > > i.e. looking at the numbers, is that there is no slow dowm. I did *not* > > perform statistical analysis to underpin this observation. > > > > That said I have a slight preference for making this change platform > > specific, and tested https://pbot.rmdir.de/9-6H2Qy6ah89TpUnMYCzeg. > > If we're taking that approach, can we have some test (e.g. in > post-build) so that zstd build fails on an arch that requires this > so we can keep the list up to date more easily? Something like this would be enough: post-build: ${WRKRSC}/zstd -o /dev/null /etc/rc Honestly, I think this is going too far. Also, we might not get this into a bulk on some potentially affected hardware (powerpc*?) before release packages are built. As mentioned, this breaks the cmake zstd detection, and I expect a good number of ports to be affected. I think release is getting close too fast to play such games. Another possibility is to do --- lib/common/mem.h.patch.orig Fri Mar 3 07:38:13 2023 +++ lib/common/mem.h Fri Mar 3 07:38:33 2023 @@ -141,7 +141,8 @@ MEM_STATIC size_t MEM_swapST(size_t in); * Default : method 1 if supported, else method 0 */ #ifndef MEM_FORCE_MEMORY_ACCESS /* can be defined externally, on command line for example */ -# ifdef __GNUC__ +# include <endian.h> +# if defined(__GNUC__) && !defined(__STRICT_ALIGNMENT) # define MEM_FORCE_MEMORY_ACCESS 1 # endif #endif > Though I think my preference for now would be to either use the > MEM_FORCE_MEMORY_ACCESS=0 hammer for all archs, or patch to backout > that upstream commit, and report upstream to see what they want to do.. Long story short, I'd like to commit the diff below. bket, if you don't like it, please pick your poison and commit whatever you prefer :) Index: Makefile =================================================================== RCS file: /cvs/ports/archivers/zstd/Makefile,v retrieving revision 1.43 diff -u -p -r1.43 Makefile --- Makefile 11 Feb 2023 22:04:54 -0000 1.43 +++ Makefile 3 Mar 2023 11:30:03 -0000 @@ -2,6 +2,7 @@ COMMENT = zstandard fast real-time comp V = 1.5.4 DISTNAME = zstd-${V} +REVISION = 0 SHARED_LIBS = zstd 6.2 # 1.5.4 @@ -23,12 +24,15 @@ LIB_DEPENDS = archivers/lz4 \ BUILD_DEPENDS = sysutils/ggrep MAKE_ENV = CC="${CC}" \ - CPPFLAGS="-I${LOCALBASE}/include" \ + CPPFLAGS="${CPPFLAGS} -I${LOCALBASE}/include" \ LDFLAGS="${LDFLAGS} -L${LOCALBASE}/lib" MAKE_FLAGS = SHARED_EXT_VER="so.$(LIBzstd_VERSION)" \ SONAME_FLAGS= \ V=1 FAKE_FLAGS = PREFIX="${PREFIX}" + +# Avoid unaligned access; use memcpy. +CPPFLAGS += -DMEM_FORCE_MEMORY_ACCESS=0 USE_GMAKE = Yes Index: patches/patch-programs_fileio_c =================================================================== RCS file: patches/patch-programs_fileio_c diff -N patches/patch-programs_fileio_c --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ patches/patch-programs_fileio_c 3 Mar 2023 11:07:34 -0000 @@ -0,0 +1,14 @@ +Don't crash on 'zstd /etc/motd' -- setvbuf isn't NULL safe. +https://github.com/facebook/zstd/issues/3523 + +Index: programs/fileio.c +--- programs/fileio.c.orig ++++ programs/fileio.c +@@ -644,6 +644,7 @@ FIO_openDstFile(FIO_ctx_t* fCtx, FIO_prefs_t* const pr + #endif + if (f == NULL) { + DISPLAYLEVEL(1, "zstd: %s: %s\n", dstFileName, strerror(errno)); ++ return NULL; + } + /* An increased buffer size can provide a significant performance boost on some platforms. + * Note that providing a NULL buf with a size that's not 0 is not defined in ANSI C, but is defined
