Re: [PATCH v2] usage: trace2 BUG() invocations

From: Jeff King <peff@peff.net>
Date: Feb 9, 2021, 12:03 UTC
Message-ID: <YCJ6D1wGgmo1a+KT@coredump.intra.peff.net>
In-Reply-To: <20210205200908.805639-1-jonathantanmy@google.com>

On Fri, Feb 05, 2021 at 12:09:08PM -0800, Jonathan Tan wrote:

Show 11 quoted lines

> die() messages are traced in trace2, but BUG() messages are not. Anyone
> tracking die() messages would have even more reason to track BUG().
> Therefore, write to trace2 when BUG() is invoked.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> Thanks everyone for your suggestions. I've used an in_bug static
> variable (which does prevent the infinite loop - I injected the bug that
> Peff described and verified that it indeed loops without the mitigation
> and doesn't loop with the mitigation) and moved the trace2 to be below
> the vreportf (necessitating a va_copy).

Thanks. I think the single static in_bug is sufficient. We can always extend it later if that turns out not to be the case.

Show 13 quoted lines

> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
> index ce7574edb1..81af180c4c 100755
> --- a/t/t0210-trace2-normal.sh
> +++ b/t/t0210-trace2-normal.sh
> @@ -147,6 +147,25 @@ test_expect_success 'normal stream, error event' '
>  	test_cmp expect actual
>  '
>  
> +# Verb 007bug
> +#
> +# Check that BUG writes to trace2
> +
> +test_expect_success 'normal stream, exit code 1' '

Is this description accurate? It doesn't really seem like "exit code 1". Maybe "99", though BUG is probably more accurate.

Show 12 quoted lines

> +	test_when_finished "rm trace.normal actual expect" &&
> +	test_must_fail env GIT_TRACE2="$(pwd)/trace.normal" test-tool trace2 007bug &&
> +	perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
> +	cat >expect <<-EOF &&
> +		version $V
> +		start _EXE_ trace2 007bug
> +		cmd_name trace2 (trace2)
> +		error the bug message
> +		exit elapsed:_TIME_ code:99
> +		atexit elapsed:_TIME_ code:99
> +	EOF
> +	test_cmp expect actual

I wondered how we triggered the BUG_exit_code magic, since I didn't see any environment set up. It turns out that it's hard-coded into test-tool, so we don't need to do anything special here in the test.

-Peff