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

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

[Sam James]

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

[PATCH 0/2] C23 compatibility

[brian m. carlson]

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(-)

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

[brian m. carlson]

"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);

[PATCH 2/2] reflog: rename unreachable

[brian m. carlson]

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;
 		}

Re: [PATCH 0/2] C23 compatibility

[Sam James]

"brian m. carlson" <sandals@crustytoothpaste.net> 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 <sam@gentoo.org>
Reviewed-by: Sam James <sam@gentoo.org>

Re: [PATCH 0/2] C23 compatibility

[Patrick Steinhardt]

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

Re: [PATCH 0/2] C23 compatibility

[Jeff King]

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

Re: [PATCH 0/2] C23 compatibility

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]