Re: [PATCH v2 0/4] Finish the conversion from die("BUG: ...") to BUG()

From: Jeff King <peff@peff.net>
Date: May 7, 2018, 09:01 UTC
Message-ID: <20180507090109.GA367@sigill.intra.peff.net>
In-Reply-To: <cover.1525253892.git.johannes.schindelin@gmx.de>

On Wed, May 02, 2018 at 11:38:13AM +0200, Johannes Schindelin wrote:

Show 8 quoted lines

> The BUG() macro was introduced in this patch series:
> https://public-inbox.org/git/20170513032414.mfrwabt4hovujde2@sigill.intra.peff.net
> 
> The second patch in that series converted one caller from die("BUG: ")
> to use the BUG() macro.
> 
> It seems that there was no concrete plan to address the same issue in
> the rest of the code base.

I had a plan; it was that people would convert these as they touched the relevant areas. :)

I'm happy to see a mass-conversion, though.

Show 6 quoted lines

> For that reason, the commit message contains the precise Unix shell
> invocation (GNU sed semantics, not BSD sed ones, because I know that the
> Git maintainer as well as the author of the patch introducing BUG() both
> use Linux and not macOS or any other platform that would offer a BSD
> sed). It should be straight-forward to handle merge
> conflicts/non-applying patches by simply re-running that command.

I suspect this could have been done with coccinelle, but it's definitely not worth going back to re-do it now.

Show 6 quoted lines

> Changes since v2:
> 
> - Avoided the entire discussion about the previous 2/6 (now dropped)
>   that prepared t1406 to handle SIGABRT by side-stepping the issue: the
>   ref-store test helper will no longer call abort() in BUG() calls but
>   exit with exit code 99 instead.

I actually think this should be a runtime flag in the environment, like GIT_BUG_EXIT_CODE (and if not set, continue to abort). That can help if you come across a BUG() in real Git code, but for some reason your environment makes aborting a pain. For example, maybe you're debugging a racy BUG() and don't want to generate a bunch of coredumps.

Or here's an interesting related case I came across a few months ago. t6210 has a test that's known to segfault due to stack exhaustion. It's marked test_expect_failure, so all is good, right? Normally, yes, but when I run the test suite inside our local Jenkins setup, it detects a segfault in _any_ child process of the test runner and aborts the whole thing. This is great as a belt-and-suspenders if we miss an unexpected segfault, but is obviously annoying in this case.

Triggering BUG()s in the test suite, even inside an expect_failure, would introduce the same headache. It would be nice if I could just do:

  GIT_BUG_EXIT_CODE=134 make test

to avoid it. Possibly expect_failure should even set that automatically.

I also suspect that nobody really needs to set a specific exit code. Using 134 is enough to avoid all of the unpleasantness of SIGABRT, but still enough to trigger test_must_fail to distinguish it from a non-BUG death. The callers that intentionally trigger bugs probably ought to be using test_expect_code to make sure they are hitting a BUG() and not some other death, anyway.

So we could probably simplify it to something like this:

diff --git a/usage.c b/usage.c
index cdd534c9df..50651bed40 100644
--- a/usage.c
+++ b/usage.c
@@ -221,7 +221,11 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
 		snprintf(prefix, sizeof(prefix), "BUG: ");
 
 	vreportf(prefix, fmt, params);
-	abort();
+
+	if (git_env_bool("GIT_BUG_ABORT"), 1)
+		abort();
+	else
+		exit(134);
 }
 
 #ifdef HAVE_VARIADIC_MACROS

-Peff