Skip to content

Fix compilation failure on MacOS X 10.9 (and likely other very old platforms)#1945

Merged
jkbonfield merged 3 commits intosamtools:developfrom
daviesrob:ancient_macos
Oct 13, 2025
Merged

Fix compilation failure on MacOS X 10.9 (and likely other very old platforms)#1945
jkbonfield merged 3 commits intosamtools:developfrom
daviesrob:ancient_macos

Conversation

@daviesrob
Copy link
Member

Adds a configure check for fstatat( , , , AT_SYMLINK_NOFOLLOW) so that the ref-cache can be disabled automatically on platforms that lack this interface. Also adjusts the position of the BUILDING_SIMD_NIBBLE2BASE guard in simd.c so that it doesn't try to include immintrin.h when the code that uses it is not going to be built.

Fixes #1941

Disable ref-cache if fstatat() doesn't work.  Allows builds of
the rest of HTSlib to complete if this interface is not available.
This mainly affects fairly old systems, notable MacOS X 10.9,
which is still supported by MacPorts.
@jmarshall
Copy link
Member

Moving the #ifdef BUILDING_SIMD_NIBBLE2BASE check up would make adding a second SIMD implementation to this source file more difficult, as the two quite likely would want to be independently activated but the #includes and cpu_supports_neon() would need to be there when any of them are activated. (One of these days I'll finish up my base2nibble() branch…)

Instead of the second commit, IMHO it would be better to also probe in configure for presence of the header file and conditionalise #include <immintrin.h> independently. Something like this immintrin-check commit.

daviesrob and others added 2 commits October 2, 2025 14:07
Add checks for `x86intrin.h` for both configure and make-only
builds, and use it to gate x86intrin.h inclusion.

Update `simd.c` to use it in preference to `immintrin.h`.
While the latter is the one preferred by Intel, and possibly
more portable than `x86intrin.h` (which was introduced by gcc
and clang), htscodecs consistently uses `x86intrin.h` so it's
easier to standardise on that one.  The intel compiler now supports
both, so the portability issue is less of a problem than it used
to be.
Prevents failures on older compilers that don't have this
builtin, or don't support using it to test for ssse3.
@daviesrob
Copy link
Member Author

I've pushed an improved version of this with, as suggested, configure checks for the intrinsics header. The file used has changed from immintrin.h to x86intrin.h as the latter is the one used by htscodecs. While the latter isn't strictly the standard one, it is supported by gcc, clang, and recent Intel compilers.

I've also added a check for __builtin_cpu_supports("ssse3") on make-only builds so that they don't fall over if the compiler is too old to understand "ssse3" as a valid input for the builtin.

@jkbonfield
Copy link
Contributor

On an old clang 3.7 (2017ish era?) it has SSE4.1 support (albeit mislabeled as bit_SSE41 instead of bit_SSE4_1, but that's just missing potential optimizations rather than failing). The mod_probe:

@ bionic-dev64[samtools.../htslib]; ./hts_probe_cc.sh 'clang37' '-g -Wall -O2 -fvisibility=hidden ' '-fvisibility=hidden'
# Compiler probe results, generated by ./hts_probe_cc.sh
HTS_HAVE_CPUID = 1
HTS_CFLAGS_SSE4 = -msse4.1 -mpopcnt -mssse3
HTS_BUILD_SSE4 = 1
HTS_CFLAGS_AVX2 = -mavx2 -mpopcnt
HTS_BUILD_AVX2 = 1
HTS_BUILD_AVX512 =
HTS_HAVE_SSSE3_BUILTINS =

So this machine doesn't understand SSSE3, but does understand SSE4. The Makefile (non-configure mode) states:

        if [ "x$(HTS_BUILD_SSE4)" != "x" ]; then \
            echo '#define HAVE_POPCNT 1' >> $@ ; \
            echo '#define HAVE_SSE4_1 1' >> $@ ; \
            echo '#define HAVE_SSSE3 1' >> $@ ; \
            echo '#if defined(HTS_ALLOW_UNALIGNED) && HTS_ALLOW_UNALIGNED == 0' >> $@ ; \
            echo '#define UBSAN 1' >> $@ ; \
            echo '#endif' >> $@ ; \
        fi

So this builds a config.h that claims to have SSSE3 despite hts-probe claiming it doesn't.
I did get a failure in simd.c, but oddly can't reproduce it now. However even without reproducing this it's technically a bug because we're putting incorrect data into config.h

@jkbonfield
Copy link
Contributor

jkbonfield commented Oct 13, 2025

Discussing this in the office, I now see my interpretation of what HTS_HAVE_SSSE3_BUILTINS means is wrong. It's do with with __attribute and __builtin and not specific whether the compiler understands ssse3 intrinsics (which it does). I've no idea how I managed to trigger the failure to build, but I cannot reproduce it so maybe I was in error.

Besides, it's an old compiler and it's fair enough to ask someone to use autoconf if they want whacky or outdated build environments.

@jkbonfield jkbonfield merged commit 1e3cef4 into samtools:develop Oct 13, 2025
9 checks passed
@daviesrob daviesrob deleted the ancient_macos branch October 13, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

error: use of undeclared identifier 'AT_SYMLINK_NOFOLLOW'

3 participants