Re: [PATCH v2 3/3] git-compat-util: add NOT_A_CONST macro and use it in atfork_prepare()
From: Jeff King <hidden>
Date: 2025-03-17 18:00:15
Possibly related (same subject, not in this thread)
- 2025-03-14 · [PATCH v2 3/3] git-compat-util: add NOT_A_CONST macro and use it in atfork_prepare() · Junio C Hamano <hidden>
On Fri, Mar 14, 2025 at 03:29:54PM -0700, Junio C Hamano wrote:
---- >8 ----
Our hope is that the number of code paths that falsely trigger
warnings with the -Wunreachable-code compilation option are small,
and they can be worked around case-by-case basis, like we just did
in the previous commit. If we need such a workaround a bit more
often, however, we may benefit from a more generic and descriptive
facility that helps document the cases we need such workarounds.
Side note: if we need the workaround all over the place, it
simply means -Wunreachable-code is not a good tool for us to
save engineering effort to catch mistakes. We are still
exploring if it helps us, so let's assume that it is not the
case.Yup, I very much agree with this, especially the side note. (I'd probably have just dropped patch 2 and gone straight here, but I don't mind leaving it in as documentation of that other direction).
Introduce NOT_A_CONST() macro, with which, the developer can tell
the compiler:
Do not optimize this expression out, because, despite whatever
you are told by the system headers, this expression should *not*
be treated as a constant.This is definitely better than the other name. I might spell it out as "NOT_A_CONSTANT", just because "const" to me is a variable annotation (for something that _could_ change, but we are not allowed to). Whereas "constant" is something defined to a single value in the program. Maybe splitting hairs, but as somebody who read NOT_A_CONST(foo) I might expect it to be casting away "const" or something.
quoted hunk ↗ jump to hunk
--- a/Makefile +++ b/Makefile@@ -1018,6 +1018,7 @@ LIB_OBJS += ewah/ewah_bitmap.o LIB_OBJS += ewah/ewah_io.o LIB_OBJS += ewah/ewah_rlw.o LIB_OBJS += exec-cmd.o +LIB_OBJS += fbtcdnki.o
That name is a mouthful, for sure. The long name is really an implementation detail. Would calling it not-constant.c or something be more descriptive? (Yes, the macro itself does not appear in the file, but hopefully it links the two semantically in the reader's head). I almost want to suggest a name like "compiler-tricks.c", but part of the point of this particular trick is that there's nothing else in its translation unit. So later when somebody adds another trick, it cannot use this macro. ;)
+/* + * Prevent an overly clever compiler from optimizing an expression + * out, triggering a false positive when building with the + * -Wunreachable-code option. false_but_the_compiler_does_not_know_it_ + * is defined in a compilation unit separate from where the macro is + * used, initialized to 0, and never modified. + */ +#define NOT_A_CONST(expr) ((expr) || false_but_the_compiler_does_not_know_it_) +extern int false_but_the_compiler_does_not_know_it_;
Good explanation. I do wonder if we'd eventually see a compiler that reaches across translation units to optimize, but I'd hope we probably bought ourselves a decade or two.
quoted hunk ↗ jump to hunk
diff --git a/run-command.c b/run-command.c index d527c46175..535c73a059 100644 --- a/run-command.c +++ b/run-command.c@@ -516,14 +516,12 @@ static void atfork_prepare(struct atfork_state *as) sigset_t all; /* - * Do not use the return value of sigfillset(). It is transparently 0 - * on some platforms, meaning a clever compiler may complain that - * the conditional body is dead code. Instead, check for error via - * errno, which outsmarts the compiler. + * POSIX says sitfillset() can fail, but an overly clever + * compiler can see through the header files and decide + * it cannot fail on a particular platform it is compiling for, + * triggering -Wunreachable-code false positive. */ - errno = 0; - sigfillset(&all); - if (errno) + if (NOT_A_CONST(sigfillset(&all))) die_errno("sigfillset");
And this looks much nicer and more descriptive. You could probably even get away without the comment, but I certainly do not mind it. s/sitfillset/sigfillset/ in your comment text, though. -Peff