Re: [PATCH 7/7] setup_git_env: avoid blind fall-back to ".git"
From: Duy Nguyen <hidden>
Date: 2016-10-26 10:29:50
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Tue, Oct 25, 2016 at 11:15:25AM -0400, Jeff King wrote:
quoted
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