From ed1db4ac6f58409b374d6a0f0be26bd6e5eb9b63 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Wed, 5 Aug 2026 18:20:31 +0200 Subject: [PATCH] julia_gc: include julia.h globally for better CHANGED_BAG CHANGED_BAG is supposed to have minimal overhead, which normally is achieved by inlining it. But we never had this for the Julia GC, mainly because it was painful to do so in our C++ source files. This PR addresses this. Unfortunately, simply inlining CHANGED_BAG by calling jl_gc_wb_back does not work: Julia declares that function static inline, and C forbids referencing an identifier with internal linkage from an inline function with external linkage. Making CHANGED_BAG itself static inline is not an option either, as it is used by other EXPORT_INLINE functions, which then run into the very same problem; and turning all of those into static inline would drop symbols from libgap which GAP.jl relies on. So instead inline a copy of Julia's write barrier, which only refers to jl_gc_queue_root. Since src/gasman.h now includes julia.h, packages need the Julia headers as well, so pass them on via sysinfo.gap. The flags reported by julia-config.jl are quoted, which does not survive the way sysinfo.gap is consumed, hence strip the quotes; and resolve the clash between Julia's FORCE_INLINE and the one used by our copy of MurmurHash3. Co-Authored-By: Claude Opus 5 --- .github/workflows/CI.yml | 5 +++-- Makefile.rules | 17 ++++++++++++++++- configure.ac | 6 +++++- src/gasman.h | 25 ++++++++++++++++++++++++- src/intfuncs.c | 14 ++++---------- src/julia_gc.c | 7 +------ 6 files changed, 53 insertions(+), 21 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 1419bd526e..1b982d43e1 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -123,10 +123,11 @@ jobs: test-suites: "testbuildsys testmockpkg testinstall" extra: "NO_COVERAGE=1 ABI=32 BUILDDIR=out-of-tree CONFIGFLAGS=\"\"" - # test Julia integration + # test Julia integration; this also runs testmockpkg, to verify + # packages can still be compiled - os: ubuntu-22.04 shell: bash - test-suites: "testinstall" + test-suites: "testmockpkg testinstall" extra: "JULIA=yes CONFIGFLAGS=\"--enable-debug\"" - os: windows-2022 diff --git a/Makefile.rules b/Makefile.rules index e2512739ec..896d0203d0 100644 --- a/Makefile.rules +++ b/Makefile.rules @@ -231,6 +231,17 @@ GAP_CPPFLAGS += $(CPPFLAGS) SYSINFO_CPPFLAGS += $(CPPFLAGS) +######################################################################## +# C and C++ compiler flags +######################################################################## + +# The Julia headers are external to GAP, so we pass their include directory via +# `-isystem` to avoid warnings from julia.h being reported in GAP builds (and +# in package builds, which may use `-Werror`). These flags are needed by +# packages, too, as src/gasman.h includes julia.h. +JULIA_ISYSTEM_CFLAGS = $(patsubst -I%,-isystem %,$(JULIA_CFLAGS)) + + ######################################################################## # C compiler flags ######################################################################## @@ -240,7 +251,8 @@ SYSINFO_CFLAGS = $(ABI_CFLAGS) GAP_CFLAGS += $(PTHREAD_CFLAGS) SYSINFO_CFLAGS += $(PTHREAD_CFLAGS) -GAP_CFLAGS += $(JULIA_CFLAGS) # not added to SYSINFO_CFLAGS +GAP_CFLAGS += $(JULIA_ISYSTEM_CFLAGS) +SYSINFO_CFLAGS += $(JULIA_ISYSTEM_CFLAGS) # Finally add user provided CFLAGS GAP_CFLAGS += $(CFLAGS) @@ -256,6 +268,9 @@ SYSINFO_CXXFLAGS = $(ABI_CFLAGS) GAP_CXXFLAGS += $(PTHREAD_CFLAGS) SYSINFO_CXXFLAGS += $(PTHREAD_CFLAGS) +GAP_CXXFLAGS += $(JULIA_ISYSTEM_CFLAGS) +SYSINFO_CXXFLAGS += $(JULIA_ISYSTEM_CFLAGS) + # Finally add user provided CXXFLAGS GAP_CXXFLAGS += $(CXXFLAGS) SYSINFO_CXXFLAGS += $(CXXFLAGS) diff --git a/configure.ac b/configure.ac index bfe967c276..388ffc7140 100644 --- a/configure.ac +++ b/configure.ac @@ -815,8 +815,12 @@ AS_IF([test "x$with_julia" != xno ],[ AS_IF([test "x$JULIA_CFLAGS" = x],[ AS_IF([test -f "${JL_SHARE}/julia-config.jl"], [], [AC_MSG_ERROR([no julia-config.jl found])]) JULIA_CFLAGS=$(${JULIA} --startup-file=no ${JL_SHARE}/julia-config.jl --cflags 2>/dev/null) - JULIA_CFLAGS=${JULIA_CFLAGS/-std=gnu99/} # need to remove -std=gnu99 for our C11 and C++ code AS_IF([ test $? != 0 ], [AC_MSG_ERROR([failed to obtain JULIA_CFLAGS from julia-config.jl])]) + JULIA_CFLAGS=${JULIA_CFLAGS/-std=gnu99/} # need to remove -std=gnu99 for our C11 and C++ code + JULIA_CFLAGS=${JULIA_CFLAGS/-std=gnu11/} # need to remove -std=gnu11 for our C++ code + # strip quotes: these flags are passed on to packages via sysinfo.gap, + # where they are subject to word splitting but not quote removal + JULIA_CFLAGS=${JULIA_CFLAGS//\'/} ]) AC_MSG_RESULT([${JULIA_CFLAGS}]) diff --git a/src/gasman.h b/src/gasman.h index b53c9eb8a4..03a9955c5d 100644 --- a/src/gasman.h +++ b/src/gasman.h @@ -366,7 +366,30 @@ EXPORT_INLINE void CHANGED_BAG(Bag bag) #elif defined(USE_JULIA_GC) -void CHANGED_BAG(Bag bag); +// Julia's headers include C++ headers when compiled as C++; since our own +// headers may be included from within an `extern "C"` block, we must ensure +// julia.h is not affected by that. +#ifdef __cplusplus +extern "C++" { +#endif +#include +#ifdef __cplusplus +} +#endif + +EXPORT_INLINE void CHANGED_BAG(Bag bag) +{ + // The following is a copy of Julia's write barrier `jl_gc_wb_back` and + // must be kept in sync with it. We cannot just call `jl_gc_wb_back`, as + // Julia declares it `static inline`, and C forbids referencing an + // identifier with internal linkage from an inline function with external + // linkage. Marking `CHANGED_BAG` as `static inline` instead is not an + // option either, as it is used by other `EXPORT_INLINE` functions (such + // as `PushPlist`), which then would run into the very same problem. + void * p = BAG_HEADER(bag); + if (__unlikely(jl_astaggedvalue(p)->bits.gc == 3 /* GC_OLD_MARKED */)) + jl_gc_queue_root((jl_value_t *)p); +} BOOL IsGapObj(void *); diff --git a/src/intfuncs.c b/src/intfuncs.c index 1f8c093b47..c9383832f6 100644 --- a/src/intfuncs.c +++ b/src/intfuncs.c @@ -177,16 +177,10 @@ UInt4 nextrandMT_int32(UInt4* mt) // compile and run any of them on any platform, but your performance with the // non-native version will be less than optimal. -//----------------------------------------------------------------------------- -// MurmurHash3 was written by Austin Appleby, and is placed in the public -// domain. The author hereby disclaims copyright to this source code. - /* Minor modifications to get it to compile in C rather than C++ and integrate with GAP SL*/ -#define FORCE_INLINE static inline - #ifndef SYS_IS_64_BIT //----------------------------------------------------------------------------- @@ -202,7 +196,7 @@ static inline uint32_t rotl32 ( uint32_t x, int8_t r ) // Block read - if your platform needs to do endian-swapping or can only // handle aligned reads, do the conversion here -FORCE_INLINE uint32_t getblock4 ( const uint32_t * p, int i ) +static inline uint32_t getblock4 ( const uint32_t * p, int i ) { return p[i]; } @@ -210,7 +204,7 @@ FORCE_INLINE uint32_t getblock4 ( const uint32_t * p, int i ) //----------------------------------------------------------------------------- // Finalization mix - force all bits of a hash block to avalanche -FORCE_INLINE uint32_t fmix4 ( uint32_t h ) +static inline uint32_t fmix4 ( uint32_t h ) { h ^= h >> 16; h *= 0x85ebca6b; @@ -304,7 +298,7 @@ static inline uint64_t rotl64 ( uint64_t x, int8_t r ) // hope that on archs which don't need this, the compiler will optimize it back // into a direct copy (verified to happen with GCC and clang on x86_64) -FORCE_INLINE uint64_t getblock8 ( const uint64_t * p, int i ) +static inline uint64_t getblock8 ( const uint64_t * p, int i ) { uint64_t val; memcpy(&val, p + i, sizeof(uint64_t)); @@ -314,7 +308,7 @@ FORCE_INLINE uint64_t getblock8 ( const uint64_t * p, int i ) //----------------------------------------------------------------------------- // Finalization mix - force all bits of a hash block to avalanche -FORCE_INLINE uint64_t fmix8 ( uint64_t k ) +static inline uint64_t fmix8 ( uint64_t k ) { k ^= k >> 33; k *= BIG_CONSTANT(0xff51afd7ed558ccd); diff --git a/src/julia_gc.c b/src/julia_gc.c index e1a8977952..6615ae4760 100644 --- a/src/julia_gc.c +++ b/src/julia_gc.c @@ -664,7 +664,7 @@ static void JFinalizer(jl_value_t * obj) UInt tnum = hdr->type; // if a bag needing a finalizer is retyped to a new tnum which no longer - // needs one, it may happen that JFinalize is called even though + // needs one, it may happen that JFinalizer is called even though // TabFreeFuncBags[tnum] is NULL if (TabFreeFuncBags[tnum]) TabFreeFuncBags[tnum]((Bag)&contents); @@ -828,11 +828,6 @@ BOOL IsGapObj(void * p) return jl_typeis(p, DatatypeGapObj); } -void CHANGED_BAG(Bag bag) -{ - jl_gc_wb_back(BAG_HEADER(bag)); -} - void SwapMasterPoint(Bag bag1, Bag bag2) { SWAP(UInt *, bag1->body, bag2->body);