# Build failure with -std=gnu23 (GCC 15 default) 8 messages from 2024-11-16 to 2024-11-18. Participants: Sam James, brian m. carlson, Patrick Steinhardt, Jeff King. Thread: https://gitlist.dev/t/62504 ## Sam James, 2024-11-16 23:18 Subject: Build failure with -std=gnu23 (GCC 15 default) Message-ID: <87ed3apy2u.fsf@gentoo.org> URL: https://gitlist.dev/e/87ed3apy2u.fsf%40gentoo.org ``` Hi, Upcoming GCC 15 defaults to -std=gnu23. git-2.47.0 fails to build as follows: ˋˋˋ builtin/index-pack.c:97:8: error: expected ‘{’ before ‘thread_local’ 97 | struct thread_local { | ^~~~~~~~~~~~ builtin/index-pack.c:120:15: error: expected ‘{’ before ‘thread_local’ 120 | static struct thread_local nothread_data; | ^~~~~~~~~~~~ builtin/index-pack.c:151:15: error: expected ‘{’ before ‘thread_local’ 151 | static struct thread_local *thread_data; | ^~~~~~~~~~~~ ˋˋˋ There may be more issues, but at the very least thread_local became a proper keyword in C23, so that will need renaming. It should be possible to reproduce these with older GCC (and Clang) with -std=gnu23 or -std=c23 set manually. thanks, sam ``` ## brian m. carlson, 2024-11-17 01:31 Subject: [PATCH 0/2] C23 compatibility Message-ID: <20241117013149.576671-1-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20241117013149.576671-1-sandals%40crustytoothpaste.net In-Reply-To: <87ed3apy2u.fsf@gentoo.org> ``` Most of our code works fine in C23, but there are some new additions to the standard that conflict with either our struct tags or functions. With this series, the code compiles and passes the testsuite with -std=c23 on GCC 14.2.0. brian m. carlson (2): index-pack: rename struct thread_local reflog: rename unreachable builtin/index-pack.c | 10 +++++----- reflog.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) ``` ## brian m. carlson, 2024-11-17 01:31 Subject: [PATCH 2/2] reflog: rename unreachable Message-ID: <20241117013149.576671-3-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20241117013149.576671-3-sandals%40crustytoothpaste.net In-Reply-To: <20241117013149.576671-1-sandals@crustytoothpaste.net> ``` In C23, "unreachable" is a macro that invokes undefined behavior if it is invoked. To make sure that our code compiles on a variety of C versions, rename unreachable to "is_unreachable". Signed-off-by: brian m. carlson --- reflog.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/reflog.c b/reflog.c index 875ac1aa66..aeab78c9b7 100644 --- a/reflog.c +++ b/reflog.c @@ -210,7 +210,7 @@ static void mark_reachable(struct expire_reflog_policy_cb *cb) cb->mark_list = leftover; } -static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid) +static int is_unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit, struct object_id *oid) { /* * We may or may not have the commit yet - if not, look it @@ -265,7 +265,7 @@ int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid, return 1; case UE_NORMAL: case UE_HEAD: - if (unreachable(cb, old_commit, ooid) || unreachable(cb, new_commit, noid)) + if (is_unreachable(cb, old_commit, ooid) || is_unreachable(cb, new_commit, noid)) return 1; break; } ``` ## brian m. carlson, 2024-11-17 01:31 Subject: [PATCH 1/2] index-pack: rename struct thread_local Message-ID: <20241117013149.576671-2-sandals@crustytoothpaste.net> URL: https://gitlist.dev/e/20241117013149.576671-2-sandals%40crustytoothpaste.net In-Reply-To: <20241117013149.576671-1-sandals@crustytoothpaste.net> ``` "thread_local" is a keyword in C23. To make sure that our code compiles on a wide variety of C versions, rename struct thread_local to "struct thread_local_data" to avoid a conflict. Signed-off-by: brian m. carlson --- builtin/index-pack.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 9d23b41b3a..a6942233e5 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -94,7 +94,7 @@ static LIST_HEAD(done_head); static size_t base_cache_used; static size_t base_cache_limit; -struct thread_local { +struct thread_local_data { pthread_t thread; int pack_fd; }; @@ -117,7 +117,7 @@ static struct object_entry *objects; static struct object_stat *obj_stat; static struct ofs_delta_entry *ofs_deltas; static struct ref_delta_entry *ref_deltas; -static struct thread_local nothread_data; +static struct thread_local_data nothread_data; static int nr_objects; static int nr_ofs_deltas; static int nr_ref_deltas; @@ -148,7 +148,7 @@ static uint32_t input_crc32; static int input_fd, output_fd; static const char *curr_pack; -static struct thread_local *thread_data; +static struct thread_local_data *thread_data; static int nr_dispatched; static int threads_active; @@ -390,7 +390,7 @@ static NORETURN void bad_object(off_t offset, const char *format, ...) (uintmax_t)offset, buf); } -static inline struct thread_local *get_thread_data(void) +static inline struct thread_local_data *get_thread_data(void) { if (HAVE_THREADS) { if (threads_active) @@ -401,7 +401,7 @@ static inline struct thread_local *get_thread_data(void) return ¬hread_data; } -static void set_thread_data(struct thread_local *data) +static void set_thread_data(struct thread_local_data *data) { if (threads_active) pthread_setspecific(key, data); ``` ## Sam James, 2024-11-17 02:43 Subject: Re: [PATCH 0/2] C23 compatibility Message-ID: <875xompolc.fsf@gentoo.org> URL: https://gitlist.dev/e/875xompolc.fsf%40gentoo.org In-Reply-To: <20241117013149.576671-1-sandals@crustytoothpaste.net> ``` "brian m. carlson" writes: > Most of our code works fine in C23, but there are some new additions to > the standard that conflict with either our struct tags or functions. > With this series, the code compiles and passes the testsuite with > -std=c23 on GCC 14.2.0. > > brian m. carlson (2): > index-pack: rename struct thread_local > reflog: rename unreachable > > builtin/index-pack.c | 10 +++++----- > reflog.c | 4 ++-- > 2 files changed, 7 insertions(+), 7 deletions(-) Thank you for the fast response! It both works and the fixes look correct. For the series: Tested-by: Sam James Reviewed-by: Sam James ``` ## Patrick Steinhardt, 2024-11-18 07:45 Subject: Re: [PATCH 0/2] C23 compatibility Message-ID: URL: https://gitlist.dev/e/Zzrwf2B36RgbhxEa%40pks.im In-Reply-To: <20241117013149.576671-1-sandals@crustytoothpaste.net> ``` On Sun, Nov 17, 2024 at 01:31:47AM +0000, brian m. carlson wrote: > Most of our code works fine in C23, but there are some new additions to > the standard that conflict with either our struct tags or functions. > With this series, the code compiles and passes the testsuite with > -std=c23 on GCC 14.2.0. > > brian m. carlson (2): > index-pack: rename struct thread_local > reflog: rename unreachable > > builtin/index-pack.c | 10 +++++----- > reflog.c | 4 ++-- > 2 files changed, 7 insertions(+), 7 deletions(-) Both of the patches look obviously good to me. I was a bit surprised that this is required in the first place as I thought we were passing `-std=gnu99` to our compilers, but that is not the case with our current Makefile. So I must have been misremembering. Thanks! Patrick ``` ## Jeff King, 2024-11-18 09:58 Subject: Re: [PATCH 0/2] C23 compatibility Message-ID: <20241118095830.GA3991628@coredump.intra.peff.net> URL: https://gitlist.dev/e/20241118095830.GA3991628%40coredump.intra.peff.net In-Reply-To: ``` On Mon, Nov 18, 2024 at 08:45:03AM +0100, Patrick Steinhardt wrote: > Both of the patches look obviously good to me. I was a bit surprised > that this is required in the first place as I thought we were passing > `-std=gnu99` to our compilers, but that is not the case with our current > Makefile. So I must have been misremembering. We do for DEVELOPER=1 mode, but it wouldn't make sense to pass something unportable like "gnu99" for the general case. -Peff ``` ## brian m. carlson, 2024-11-18 22:11 Subject: Re: [PATCH 0/2] C23 compatibility Message-ID: URL: https://gitlist.dev/e/Zzu7oiJ8PTWgmJUc%40tapette.crustytoothpaste.net In-Reply-To: ``` On 2024-11-18 at 07:45:03, Patrick Steinhardt wrote: > Both of the patches look obviously good to me. I was a bit surprised > that this is required in the first place as I thought we were passing > `-std=gnu99` to our compilers, but that is not the case with our current > Makefile. So I must have been misremembering. We can't do that because FreeBSD's system headers require C11 and won't work with `-std=gnu99`, in addition to the general unportability of that construct as mentioned by Peff. There's a comment to that effect in `config.mak.dev`. I'm pleased you found the patches acceptable, though. -- brian m. carlson (they/them or he/him) Toronto, Ontario, CA ```