Re: [PATCH 1/2] Remove noreturn function pointers in usage.c

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2011, 00:36 UTC
Message-ID: <7v4o3z264s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1307569417-8924-1-git-send-email-andi@firstfloor.org>

Andi Kleen <andi@firstfloor.org> writes:

Show 11 quoted lines

> From: Andi Kleen <ak@linux.intel.com>
>
> Due to a bug in gcc 4.6+ it can crash when doing profile feedback
> with a noreturn function pointer
>
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
>
> Remove the NORETURNs from the die functions for now to work
> around this. Doesn't seem to make any difference.
>
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

I would expect a better patch from a well respected kernel person, though.

 - There are many more NORETURN and NORETURN_PTR in the code, and the
   proposed commit log message does not explain why these two are the only
   ones that are problematic and needs to be worked around. It does not
   guide other people who might want to add NORETURN/NORETURN_PTR when
   deciding if their change would break the "fix" this change brought in.

 - Potential impact to people who do not use Gcc 4.6 with profile feedback
   is not explained away well, except for "Doesn't seem to make any
   difference."

 - If other NORETURN/NORETURN_PTR could/should also go (I don't know due
   to the first bullet point above) when using the problematic compiler
   with the profile feedback feature, wouldn't it be a better workaround
   would be to introduce a Makefile variable to ask git-compat-util.h to
   make these two a no-op, perhaps?

A patch to do so may look like this.

I did not like the triple negation "make NO_NORETURN=NoThanks" and wanted to name this AVOID_NORETURN instead, but decided to go with other existing Makefile variables.

 Makefile          |    6 ++++++
 git-compat-util.h |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index d2e2ea1..70c814c 100644
--- a/Makefile
+++ b/Makefile
@@ -144,6 +144,9 @@ all::
 # that tells runtime paths to dynamic libraries;
 # "-Wl,-rpath=/path/lib" is used instead.
 #
+# Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
+# as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
+#
 # Define USE_NSEC below if you want git to care about sub-second file mtimes
 # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and
 # it will BREAK YOUR LOCAL DIFFS! show-diff and anything using it will likely
@@ -1349,6 +1352,9 @@ endif
 ifdef USE_ST_TIMESPEC
 	BASIC_CFLAGS += -DUSE_ST_TIMESPEC
 endif
+ifdef NO_NORETURN
+	BASIC_CFLAGS += -DNO_NORETURN
+endif
 ifdef NO_NSEC
 	BASIC_CFLAGS += -DNO_NSEC
 endif
diff --git a/git-compat-util.h b/git-compat-util.h
index 40498b3..13bc26f 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -218,7 +218,7 @@ extern char *gitbasename(char *);
 #if __HP_cc >= 61000
 #define NORETURN __attribute__((noreturn))
 #define NORETURN_PTR
-#elif defined(__GNUC__)
+#elif defined(__GNUC__) && !defined(NO_NORETURN)
 #define NORETURN __attribute__((__noreturn__))
 #define NORETURN_PTR __attribute__((__noreturn__))
 #elif defined(_MSC_VER)