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

[Jeff King <peff@peff.net>]

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

> 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.

> 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.

> 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