threads / discuss / 62504

Build failure with -std=gnu23 (GCC 15 default)

Subject: Build failure with -std=gnu23 (GCC 15 default)

## tl;dr

8 messages between Nov 16, 2024 and Nov 18, 2024.

replies: 7people: 4as markdown or json

Sam James· Nov 16, 2024, 23:18 UTC · lore
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· Nov 17, 2024, 01:31 UTC · re: Sam James · lore

[PATCH 0/2] C23 compatibility

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· Nov 17, 2024, 01:31 UTC · re: brian m. carlson · lore

[PATCH 2/2] reflog: rename unreachable

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 <sandals@crustytoothpaste.net>
---
 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· Nov 17, 2024, 01:31 UTC · re: brian m. carlson · lore

[PATCH 1/2] index-pack: rename struct thread_local

"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 <sandals@crustytoothpaste.net>
---
 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 &nothread_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· Nov 17, 2024, 02:43 UTC · re: brian m. carlson · lore

Re: [PATCH 0/2] C23 compatibility

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 12 quoted lines
> 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 <sam@gentoo.org>
Reviewed-by: Sam James <sam@gentoo.org>
Patrick Steinhardt· Nov 18, 2024, 07:45 UTC · re: brian m. carlson · lore

Re: [PATCH 0/2] C23 compatibility

On Sun, Nov 17, 2024 at 01:31:47AM +0000, brian m. carlson wrote:
Show 12 quoted lines
> 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· Nov 18, 2024, 09:58 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 0/2] C23 compatibility

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· Nov 18, 2024, 22:11 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 0/2] C23 compatibility

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

← back to recent threads