git-compat-util: add NOT_CONSTANT macro and use it in atfork_prepare()

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.

Introduce NOT_CONSTANT() 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.

and use it as a replacement for the workaround we used that was
somewhat specific to the sigfillset case.  If the compiler already
knows that the call to sigfillset() cannot fail on a particular
platform it is compiling for and declares that the if() condition
would not hold, it is plausible that the next version of the
compiler may learn that sigfillset() that never fails would not
touch errno and decide that in this sequence:

	errno = 0;
	sigfillset(&all)
	if (errno)
		die_errno("sigfillset");

the if() statement will never trigger.  Marking that the value
returned by sigfillset() cannot be a constant would document our
intention better and would not break with such a new version of
compiler that is even more "clever".  With the marco, the above
sequence can be rewritten:

	if (NOT_CONSTANT(sigfillset(&all)))
		die_errno("sigfillset");

which looks almost like other innocuous annotations we have,
e.g. UNUSED.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Makefile b/Makefile
index 97e8385b6643b963c54affb3ae621fc93fad28b5..79121c5a925d3933c0b118a7e61815a312444937 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -985,6 +985,7 @@ LIB_OBJS += compat/nonblock.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += compat/zlib-uncompress2.o
+LIB_OBJS += compiler-tricks/not-constant.o
 LIB_OBJS += config.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o

diff --git a/compiler-tricks/not-constant.c b/compiler-tricks/not-constant.c
new file mode 100644 (file)
index 0000000..1da3ffc
--- /dev/null
+++ b/compiler-tricks/not-constant.c
@@ -0,0 +1,2 @@
+#include <git-compat-util.h>
+int false_but_the_compiler_does_not_know_it_;

diff --git a/git-compat-util.h b/git-compat-util.h
index e283c46c6fa06e4079851296a55c9bd5472a65b4..c4f96dcc7b71f6cf637bba7121b9e86eb42b95ea 100644 (file)
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1593,4 +1593,13 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)
        ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))
 #endif /* !__GNUC__ */
 
+/*
+ * 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_CONSTANT(expr) ((expr) || false_but_the_compiler_does_not_know_it_)
+extern int false_but_the_compiler_does_not_know_it_;
 #endif

diff --git a/meson.build b/meson.build
index 0064eb64f546a6349a8694ce251bd352febda6fe..f5c9dfa95b99564cbdfd0a6b6257a4aa80629a8c 100644 (file)
--- a/meson.build
+++ b/meson.build
@@ -249,6 +249,7 @@ libgit_sources = [
   'compat/obstack.c',
   'compat/terminal.c',
   'compat/zlib-uncompress2.c',
+  'compiler-tricks/not-constant.c',
   'config.c',
   'connect.c',
   'connected.c',

diff --git a/run-command.c b/run-command.c
index d527c4617571047d06eb5767addb62ce0de15764..8833b2336785d92175f37d356ab7aa9904e69d8d 100644 (file)
--- 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 sigfillset() 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_CONSTANT(sigfillset(&all)))
                die_errno("sigfillset");
 #ifdef NO_PTHREADS
        if (sigprocmask(SIG_SETMASK, &all, &as->old))