# [PATCH 4/4] Fix unreachable-code warning with clang on Windows 11 messages from 2025-06-03 to 2025-06-06. Participants: Mike Hommey, Junio C Hamano, Patrick Steinhardt, Jeff King. Thread: https://gitlist.dev/t/63575 ## Mike Hommey, 2025-06-03 23:06 Subject: [PATCH 4/4] Fix unreachable-code warning with clang on Windows Message-ID: <20250603230646.2322671-4-mh@glandium.org> URL: https://gitlist.dev/e/20250603230646.2322671-4-mh%40glandium.org In-Reply-To: <20250603230646.2322671-1-mh@glandium.org> ``` ˋˋˋ refs/files-backend.c:3187:5: error: code will never be executed [-Werror,-Wunreachable-code] 3187 | continue; | ^~~~~~~~ 1 error generated. ˋˋˋ Signed-off-by: Mike Hommey --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index bf6f89b1d1..af21eb80a9 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3183,7 +3183,7 @@ static int files_transaction_finish(struct ref_store *ref_store, * next update. If not, we try and create a regular symref. */ if (update->new_target && refs->prefer_symlink_refs) - if (!create_ref_symlink(lock, update->new_target)) + if (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target))) continue; if (update->flags & REF_NEEDS_COMMIT) { -- 2.50.0.rc1.593.g042f21cb9b ``` ## Mike Hommey, 2025-06-03 23:06 Subject: [PATCH 3/4] Fix comma warnings with clang on Windows Message-ID: <20250603230646.2322671-3-mh@glandium.org> URL: https://gitlist.dev/e/20250603230646.2322671-3-mh%40glandium.org In-Reply-To: <20250603230646.2322671-1-mh@glandium.org> ``` e.g. ˋˋˋ compat/mingw.c:723:24: error: possible misuse of comma operator here [-Werror,-Wcomma] 723 | return errno = ENOSYS, -1; | ^ compat/mingw.c:723:10: note: cast expression to void to silence warning 723 | return errno = ENOSYS, -1; | ^~~~~~~~~~~~~~ | (void)( ) /usr/x86_64-w64-mingw32/include/stdlib.h:155:15: note: expanded from macro 'errno' 155 | #define errno (*_errno()) | ^ ˋˋˋ Signed-off-by: Mike Hommey --- compat/mingw.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 8a9972a1ca..cd54937ebd 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -501,8 +501,10 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) DWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING; /* only these flags are supported */ - if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) - return errno = ENOSYS, -1; + if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) { + errno = ENOSYS; + return -1; + } /* * FILE_SHARE_WRITE is required to permit child processes @@ -2497,12 +2499,14 @@ static int start_timer_thread(void) timer_event = CreateEvent(NULL, FALSE, FALSE, NULL); if (timer_event) { timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL); - if (!timer_thread ) - return errno = ENOMEM, - error("cannot start timer thread"); - } else - return errno = ENOMEM, - error("cannot allocate resources for timer"); + if (!timer_thread) { + errno = ENOMEM; + return error("cannot start timer thread"); + } + } else { + errno = ENOMEM; + return error("cannot allocate resources for timer"); + } return 0; } @@ -2535,13 +2539,15 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out) static const struct timeval zero; static int atexit_done; - if (out) - return errno = EINVAL, - error("setitimer param 3 != NULL not implemented"); + if (out) { + errno = EINVAL; + return error("setitimer param 3 != NULL not implemented"); + } if (!is_timeval_eq(&in->it_interval, &zero) && - !is_timeval_eq(&in->it_interval, &in->it_value)) - return errno = EINVAL, - error("setitimer: it_interval must be zero or eq it_value"); + !is_timeval_eq(&in->it_interval, &in->it_value)) { + errno = EINVAL; + return error("setitimer: it_interval must be zero or eq it_value"); + } if (timer_thread) stop_timer_thread(); @@ -2561,12 +2567,14 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out) int sigaction(int sig, struct sigaction *in, struct sigaction *out) { - if (sig != SIGALRM) - return errno = EINVAL, - error("sigaction only implemented for SIGALRM"); - if (out) - return errno = EINVAL, - error("sigaction: param 3 != NULL not implemented"); + if (sig != SIGALRM) { + errno = EINVAL; + return error("sigaction only implemented for SIGALRM"); + } + if (out) { + errno = EINVAL; + return error("sigaction: param 3 != NULL not implemented"); + } timer_fn = in->sa_handler; return 0; -- 2.50.0.rc1.593.g042f21cb9b ``` ## Mike Hommey, 2025-06-03 23:06 Subject: [PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3 Message-ID: <20250603230646.2322671-1-mh@glandium.org> URL: https://gitlist.dev/e/20250603230646.2322671-1-mh%40glandium.org ``` ˋˋˋ In file included from parse-options.c:1: git-compat-util.h: In function ‘get_value’: git-compat-util.h:489:21: error: ‘arg’ may be used uninitialized [-Werror=maybe-uninitialized] 489 | #define error(...) (error(__VA_ARGS__), const_error()) | ^~~~~ parse-options.c:76:21: note: ‘arg’ was declared here 76 | const char *arg; | ^~~ ˋˋˋ Signed-off-by: Mike Hommey --- parse-options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index a9a39ecaef..cf79805bc0 100644 --- a/parse-options.c +++ b/parse-options.c @@ -73,7 +73,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, enum opt_parsed flags, const char **argp) { - const char *arg; + const char *arg = NULL; const int unset = flags & OPT_UNSET; int err; -- 2.50.0.rc1.593.g042f21cb9b ``` ## Mike Hommey, 2025-06-03 23:06 Subject: [PATCH 2/4] Fix use-after-free warning with GCC at -O3 Message-ID: <20250603230646.2322671-2-mh@glandium.org> URL: https://gitlist.dev/e/20250603230646.2322671-2-mh%40glandium.org In-Reply-To: <20250603230646.2322671-1-mh@glandium.org> ``` ˋˋˋ reftable/basics.c: In function ‘parse_names’: reftable/basics.c:233:17: error: pointer ‘names’ may be used after ‘free’ [-Werror=use-after-free] 233 | reftable_free(names[i]); | ^~~~~~~~~~~~~~~~~~~~~~~ In function ‘reftable_free’, inlined from ‘reftable_realloc’ at reftable/basics.c:30:3, inlined from ‘reftable_realloc’ at reftable/basics.c:27:7, inlined from ‘reftable_alloc_grow’ at reftable/basics.h:228:10, inlined from ‘parse_names’ at reftable/basics.c:214:8: reftable/basics.c:44:17: note: call to ‘free’ here 44 | free(p); | ^~~~~~~ ˋˋˋ Signed-off-by: Mike Hommey --- reftable/basics.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/reftable/basics.c b/reftable/basics.c index 9988ebd635..de21fe6ef7 100644 --- a/reftable/basics.c +++ b/reftable/basics.c @@ -229,9 +229,11 @@ char **parse_names(char *buf, int size) return names; err: - for (size_t i = 0; i < names_len; i++) - reftable_free(names[i]); - reftable_free(names); + if (names) { + for (size_t i = 0; i < names_len; i++) + reftable_free(names[i]); + reftable_free(names); + } return NULL; } -- 2.50.0.rc1.593.g042f21cb9b ``` ## Junio C Hamano, 2025-06-03 23:47 Subject: Re: [PATCH 2/4] Fix use-after-free warning with GCC at -O3 Message-ID: URL: https://gitlist.dev/e/xmqqwm9sfkuu.fsf%40gitster.g In-Reply-To: <20250603230646.2322671-2-mh@glandium.org> ``` Mike Hommey writes: > Cc: gitster@pobox.com, Mike Hommey Not to me, but to the designated area expert. Thanks. > ˋˋˋ > reftable/basics.c: In function ‘parse_names’: > reftable/basics.c:233:17: error: pointer ‘names’ may be used after ‘free’ [-Werror=use-after-free] > 233 | reftable_free(names[i]); > | ^~~~~~~~~~~~~~~~~~~~~~~ > In function ‘reftable_free’, > inlined from ‘reftable_realloc’ at reftable/basics.c:30:3, > inlined from ‘reftable_realloc’ at reftable/basics.c:27:7, > inlined from ‘reftable_alloc_grow’ at reftable/basics.h:228:10, > inlined from ‘parse_names’ at reftable/basics.c:214:8: > reftable/basics.c:44:17: note: call to ‘free’ here > 44 | free(p); > | ^~~~~~~ > ˋˋˋ > > Signed-off-by: Mike Hommey > --- > reftable/basics.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/reftable/basics.c b/reftable/basics.c > index 9988ebd635..de21fe6ef7 100644 > --- a/reftable/basics.c > +++ b/reftable/basics.c > @@ -229,9 +229,11 @@ char **parse_names(char *buf, int size) > return names; > > err: > - for (size_t i = 0; i < names_len; i++) > - reftable_free(names[i]); > - reftable_free(names); > + if (names) { > + for (size_t i = 0; i < names_len; i++) > + reftable_free(names[i]); > + reftable_free(names); > + } > return NULL; > } ``` ## Junio C Hamano, 2025-06-03 23:51 Subject: Re: [PATCH 3/4] Fix comma warnings with clang on Windows Message-ID: URL: https://gitlist.dev/e/xmqqplfkfkol.fsf%40gitster.g In-Reply-To: <20250603230646.2322671-3-mh@glandium.org> ``` Mike Hommey writes: > Subject: Re: [PATCH 3/4] Fix comma warnings with clang on Windows Common to all four patches, as "Fix" does not quite tell the story, please choose more appropriate verb. For example, judging from how this was rewritten ... > - if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) > - return errno = ENOSYS, -1; > + if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) { > + errno = ENOSYS; > + return -1; > + } ... the warning is a false positive (i.e. the code does what the author intended it to do), so this is more like "squelching" the warning. I obviously like the style after this patch. I just thought that the proposed log message, especially its title, were not clear enough to tell which ones are real fixes and which ones are workarounds. Thanks. ``` ## Patrick Steinhardt, 2025-06-04 07:36 Subject: Re: [PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3 Message-ID: URL: https://gitlist.dev/e/aD_3Y0PQtfg8Dd9z%40pks.im In-Reply-To: <20250603230646.2322671-1-mh@glandium.org> ``` On Wed, Jun 04, 2025 at 08:06:43AM +0900, Mike Hommey wrote: > ˋˋˋ > In file included from parse-options.c:1: > git-compat-util.h: In function ‘get_value’: > git-compat-util.h:489:21: error: ‘arg’ may be used uninitialized [-Werror=maybe-uninitialized] > 489 | #define error(...) (error(__VA_ARGS__), const_error()) > | ^~~~~ > parse-options.c:76:21: note: ‘arg’ was declared here > 76 | const char *arg; > | ^~~ > ˋˋˋ A bit more explanation whether this warning is a false positive or whether this may be an actual issue would be welcome. Patrick ``` ## Patrick Steinhardt, 2025-06-04 07:36 Subject: Re: [PATCH 2/4] Fix use-after-free warning with GCC at -O3 Message-ID: URL: https://gitlist.dev/e/aD_3ZzWSbyXIk81_%40pks.im In-Reply-To: <20250603230646.2322671-2-mh@glandium.org> ``` On Wed, Jun 04, 2025 at 08:06:44AM +0900, Mike Hommey wrote: > ˋˋˋ > reftable/basics.c: In function ‘parse_names’: > reftable/basics.c:233:17: error: pointer ‘names’ may be used after ‘free’ [-Werror=use-after-free] > 233 | reftable_free(names[i]); > | ^~~~~~~~~~~~~~~~~~~~~~~ > In function ‘reftable_free’, > inlined from ‘reftable_realloc’ at reftable/basics.c:30:3, > inlined from ‘reftable_realloc’ at reftable/basics.c:27:7, > inlined from ‘reftable_alloc_grow’ at reftable/basics.h:228:10, > inlined from ‘parse_names’ at reftable/basics.c:214:8: > reftable/basics.c:44:17: note: call to ‘free’ here > 44 | free(p); > | ^~~~~~~ > ˋˋˋ Same here, only posting the warning isn't sufficient to explain what's going on. > diff --git a/reftable/basics.c b/reftable/basics.c > index 9988ebd635..de21fe6ef7 100644 > --- a/reftable/basics.c > +++ b/reftable/basics.c > @@ -229,9 +229,11 @@ char **parse_names(char *buf, int size) > return names; > > err: > - for (size_t i = 0; i < names_len; i++) > - reftable_free(names[i]); > - reftable_free(names); > + if (names) { > + for (size_t i = 0; i < names_len; i++) > + reftable_free(names[i]); > + reftable_free(names); > + } > return NULL; > } This change shouldn't be needed in theory: `names_len` has a positive value if and only if `names` is non-NULL. So the warning is a false positive. That being said I'm not opposed to squelching this warning. But details like this should be explained in the commit message. Patrick ``` ## Patrick Steinhardt, 2025-06-04 07:36 Subject: Re: [PATCH 4/4] Fix unreachable-code warning with clang on Windows Message-ID: URL: https://gitlist.dev/e/aD_3ahX2jyrtfvjq%40pks.im In-Reply-To: <20250603230646.2322671-4-mh@glandium.org> ``` On Wed, Jun 04, 2025 at 08:06:46AM +0900, Mike Hommey wrote: > ˋˋˋ > refs/files-backend.c:3187:5: error: code will never be executed [-Werror,-Wunreachable-code] > 3187 | continue; > | ^~~~~~~~ > 1 error generated. > ˋˋˋ > > Signed-off-by: Mike Hommey > --- > refs/files-backend.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index bf6f89b1d1..af21eb80a9 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -3183,7 +3183,7 @@ static int files_transaction_finish(struct ref_store *ref_store, > * next update. If not, we try and create a regular symref. > */ > if (update->new_target && refs->prefer_symlink_refs) > - if (!create_ref_symlink(lock, update->new_target)) > + if (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target))) > continue; So the story here is that there are two implementations of `create_ref_symlink()`: - One macro that is defined to `(-1)` which is set when NO_SYMLINK_HEAD is defined. - A function that creates the ref symlink if NO_SYMLINK_HEAD is not defined. The function won't cause the error, but the macro will. So wouldn't it make more sense to wrap the macro itself in `NOT_CONSTANT`, like this: #define create_ref_symlink(a, b) NOT_CONSTANT(-1) Patrick ``` ## Junio C Hamano, 2025-06-04 15:50 Subject: Re: [PATCH 4/4] Fix unreachable-code warning with clang on Windows Message-ID: URL: https://gitlist.dev/e/xmqq8qm7cxq3.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: > The function won't cause the error, but the macro will. So wouldn't it > make more sense to wrap the macro itself in `NOT_CONSTANT`, like this: > > #define create_ref_symlink(a, b) NOT_CONSTANT(-1) That's clever ;-). We cannot unfortunatel do the same at the site that the macro NOT_CONSTANT() was invented for, though. ``` ## Jeff King, 2025-06-06 00:23 Subject: Re: [PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3 Message-ID: <20250606002329.GA3556939@coredump.intra.peff.net> URL: https://gitlist.dev/e/20250606002329.GA3556939%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Jun 04, 2025 at 09:36:03AM +0200, Patrick Steinhardt wrote: > On Wed, Jun 04, 2025 at 08:06:43AM +0900, Mike Hommey wrote: > > ˋˋˋ > > In file included from parse-options.c:1: > > git-compat-util.h: In function ‘get_value’: > > git-compat-util.h:489:21: error: ‘arg’ may be used uninitialized [-Werror=maybe-uninitialized] > > 489 | #define error(...) (error(__VA_ARGS__), const_error()) > > | ^~~~~ > > parse-options.c:76:21: note: ‘arg’ was declared here > > 76 | const char *arg; > > | ^~~ > > ˋˋˋ > > A bit more explanation whether this warning is a false positive or > whether this may be an actual issue would be welcome. I thought at first it was a false positive, as we'd always fill in "arg" via get_arg(), or return an error. But I suspect the culprit is the earlier part of this if/else chain: if (unset) { value = 0; } else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) { value = opt->defval; } else if (get_arg(p, opt, flags, &arg)) { return -1; } ... So if "unset" is true, we set "value" but not "arg". The code the compiler is complaining about is here: if (value < lower_bound) return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"), arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound); In the case of "unset", I think we could never trigger this, since our lower bound will never be above 0. But what about opt->defval? We don't know anything about it here. Probably it would be a programming error to pass in a value that is outside the bounds, but this code doesn't know that. So something like this (it was actually hard to find an integer option with OPTARG!): diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3b6aac2cf7..630403611b 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -28,7 +28,7 @@ int cmd_fmt_merge_msg(int argc, .argh = N_("n"), .help = N_("populate log with at most entries from shortlog"), .flags = PARSE_OPT_OPTARG, - .defval = DEFAULT_MERGE_LOG_LEN, + .defval = -2147483649, }, { .type = OPTION_INTEGER, would cause: git fmt-merge-msg --log to print uninitialized memory. But it is equally wrong with Mike's patch; we'd just segfault! I think this is unlikely to happen in practice, but it does feel like we should be able to solve it in a better way. I came up with two options. One is to BUG() on a bogus default value, like this: diff --git a/parse-options.c b/parse-options.c index a9a39ecaef..7ddf213d0c 100644 --- a/parse-options.c +++ b/parse-options.c @@ -73,7 +73,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, enum opt_parsed flags, const char **argp) { - const char *arg; + const char *arg = NULL; const int unset = flags & OPT_UNSET; int err; @@ -195,9 +195,13 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, optname(opt, flags)); } - if (value < lower_bound) + if (value < lower_bound) { + if (!arg) + BUG("default option value for '%s' is not in range", + optname(opt, flags)); return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"), arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound); + } switch (opt->precision) { case 1: The other is to not bother checking the bounds on the default values at all, by pushing this bounds check into the if/else chain after we know we have something to parse, like this (extended context to make it more obvious how this fits into the chain): diff --git a/parse-options.c b/parse-options.c index a9a39ecaef..d36e56da15 100644 --- a/parse-options.c +++ b/parse-options.c @@ -179,39 +179,38 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p, if (unset) { value = 0; } else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) { value = opt->defval; } else if (get_arg(p, opt, flags, &arg)) { return -1; } else if (!*arg) { return error(_("%s expects a numerical value"), optname(opt, flags)); } else if (!git_parse_signed(arg, &value, upper_bound)) { if (errno == ERANGE) return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"), arg, optname(opt, flags), lower_bound, upper_bound); return error(_("%s expects an integer value with an optional k/m/g suffix"), optname(opt, flags)); - } - - if (value < lower_bound) + } else if (value < lower_bound) { return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"), arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound); + } switch (opt->precision) { case 1: *(int8_t *)opt->value = value; return 0; case 2: *(int16_t *)opt->value = value; return 0; case 4: *(int32_t *)opt->value = value; return 0; case 8: *(int64_t *)opt->value = value; return 0; default: BUG("invalid precision for option %s", optname(opt, flags)); -Peff ```