Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Oct 25, 2016 at 11:15:25AM -0400, Jeff King wrote:
> > The "once we've identified" part could be tricky though. This message
> > alone will not give us any clue where it's called since it's buried
> > deep in git_path() usually, which is buried deep elsewhere. Without
> > falling back to core dumps (with debug info), glibc's backtrace
> > (platform specifc), the best we could do is turn git_path() into a
> > macro that takes __FILE__ and __LINE__ and somehow pass the info down
> > here, but "..." in macros is C99 specific, sigh..
> > 
> > Is it too bad to turn git_path() into a macro when we know the
> > compiler is C99 ? Older compilers will have no source location info in
> > git_path(), Hopefully they are rare, which means chances of this fault
> > popping up are also reduced.
> 
> I think you could conditionally make git_path() and all of its
> counterparts macros, similar to the way the trace code works. It seems
> like a pretty maintenance-heavy solution, though. I'd prefer
> conditionally compiling backtrace(); that also doesn't hit 100% of
> cases, but at least it isn't too invasive.

OK, a more polished patch is this. There are warnings about
-fomit-function-pointers in glibc man page, at least in my simple
tests it does not cause any issue.

> But I think I still prefer just letting the corefile and the debugger do
> their job. This error shouldn't happen much, and when it does, it should
> be easily reproducible. Getting the bug reporter to give either a
> reproduction recipe, or to run "gdb git" doesn't seem like that big a
> hurdle.

There are other places where backtrace() support may come handy
too. If -rdynamic was not needed, I would push for this patch. Too bad
backtrace() is not a perfect magic wand.

-- 8< --
diff --git a/config.mak.uname b/config.mak.uname
index b232908..b38f62a 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -40,6 +40,8 @@ ifeq ($(uname_S),Linux)
 	NEEDS_LIBRT = YesPlease
 	HAVE_GETDELIM = YesPlease
 	SANE_TEXT_GREP=-a
+	# for backtrace() support with glibc >= 2.1
+	BASIC_LDFLAGS += -rdynamic
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
 	HAVE_ALLOCA_H = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index 43718da..3561ab9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -408,6 +408,7 @@ extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern NORETURN void BUG(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
@@ -709,6 +710,7 @@ extern int git_vsnprintf(char *str, size_t maxsize,
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
+#define HAVE_BACKTRACE
 #endif
 #endif
 
@@ -722,6 +724,23 @@ static inline char *gitstrchrnul(const char *s, int c)
 }
 #endif
 
+#ifdef HAVE_BACKTRACE
+#include <execinfo.h>
+static inline void dump_backtrace(FILE *fp)
+{
+	void *buffer[32];
+	int nptrs;
+
+	nptrs = backtrace(buffer, sizeof(buffer) / sizeof(*buffer));
+	fflush(fp);
+	backtrace_symbols_fd(buffer, nptrs, fileno(fp));
+}
+#else
+static inline void dump_backtrace(FILE *fp)
+{
+}
+#endif
+
 #ifdef NO_INET_PTON
 int inet_pton(int af, const char *src, void *dst);
 #endif
diff --git a/usage.c b/usage.c
index 17f52c1..b00603c 100644
--- a/usage.c
+++ b/usage.c
@@ -204,3 +204,16 @@ void warning(const char *warn, ...)
 	warn_routine(warn, params);
 	va_end(params);
 }
+
+void NORETURN BUG(const char *fmt, ...)
+{
+	va_list params;
+
+	va_start(params, fmt);
+	vreportf("BUG: ", fmt, params);
+	va_end(params);
+
+	dump_backtrace(error_handle ? error_handle : stderr);
+
+	exit(128);
+}
-- 8< --
--
Duy