* [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:58 ` Ævar Arnfjörð Bjarmason
2022-06-01 18:37 ` Junio C Hamano
2022-05-31 16:58 ` [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Change the exit() wrapper added in ee4512ed481 (trace2: create new
combined trace facility, 2019-02-22) so that we'll split up the trace2
logging concerns from wanting to wrap the "exit()" function itself for
other purposes.
This makes more sense structurally, as we won't seem to conflate
non-trace2 behavior with the trace2 code. I'd previously added an
explanation for this in 368b5843158 (common-main.c: call exit(), don't
return, 2021-12-07), that comment is being adjusted here.
Now the only thing we'll do if we're not using trace2 is to truncate
the "code" argument to the lowest 8 bits.
We only need to do that truncation on non-POSIX systems, but in
ee4512ed481 that "if defined(__MINGW32__)" code added in
47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
2009-07-05) was made to run everywhere. It might be good for clarify
to narrow that down by an "ifdef" again, but I'm not certain that in
the interim we haven't had some other non-POSIX systems rely the
behavior. On a POSIX system taking the lowest 8 bits is implicit, see
exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
1. https://man7.org/linux/man-pages/man3/exit.3.html
2. https://man7.org/linux/man-pages/man2/wait.2.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
common-main.c | 19 +++++++++++++++++--
git-compat-util.h | 12 ++++++++++--
trace2.c | 8 ++------
trace2.h | 8 +-------
4 files changed, 30 insertions(+), 17 deletions(-)
diff --git a/common-main.c b/common-main.c
index 29fb7452f8a..bb0100f6024 100644
--- a/common-main.c
+++ b/common-main.c
@@ -56,9 +56,24 @@ int main(int argc, const char **argv)
result = cmd_main(argc, argv);
/*
- * We define exit() to call trace2_cmd_exit_fl() in
- * git-compat-util.h. Whether we reach this or exit()
+ * We define exit() to call common_exit(), which will in turn
+ * call trace2_cmd_exit_fl(). Whether we reach this or exit()
* elsewhere we'll always run our trace2 exit handler.
*/
exit(result);
}
+
+int common_exit(const char *file, int line, int code)
+{
+ /*
+ * For non-POSIX systems: Take the lowest 8 bits of the "code"
+ * to e.g. turn -1 into 255. On a POSIX system this is
+ * redundant, see exit(3) and wait(2), but as it doesn't harm
+ * anything there we don't need to guard this with an "ifdef".
+ */
+ code &= 0xff;
+
+ trace2_cmd_exit_fl(file, line, code);
+
+ return code;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index 96293b6c43a..a446a363de2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1447,12 +1447,20 @@ static inline int is_missing_file_error(int errno_)
int cmd_main(int, const char **);
+/**
+ * The exit() function is defined as common_exit() in
+ * git-compat-util.h.
+ *
+ * Intercepting the exit() allows us to hook in at-exit behavior, such
+ * emitting trace2 logging before calling the real exit().
+ */
+int common_exit(const char *file, int line, int code);
+
/*
* Intercept all calls to exit() and route them to trace2 to
* optionally emit a message before calling the real exit().
*/
-int trace2_cmd_exit_fl(const char *file, int line, int code);
-#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))
+#define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
/*
* You can mark a stack variable with UNLEAK(var) to avoid it being
diff --git a/trace2.c b/trace2.c
index e01cf77f1a8..0c0a11e07d5 100644
--- a/trace2.c
+++ b/trace2.c
@@ -202,17 +202,15 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)
argv);
}
-int trace2_cmd_exit_fl(const char *file, int line, int code)
+void trace2_cmd_exit_fl(const char *file, int line, int code)
{
struct tr2_tgt *tgt_j;
int j;
uint64_t us_now;
uint64_t us_elapsed_absolute;
- code &= 0xff;
-
if (!trace2_enabled)
- return code;
+ return;
trace_git_fsync_stats();
trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);
@@ -226,8 +224,6 @@ int trace2_cmd_exit_fl(const char *file, int line, int code)
if (tgt_j->pfn_exit_fl)
tgt_j->pfn_exit_fl(file, line, us_elapsed_absolute,
code);
-
- return code;
}
void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
diff --git a/trace2.h b/trace2.h
index 1b109f57d0a..88d906ea830 100644
--- a/trace2.h
+++ b/trace2.h
@@ -101,14 +101,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv);
/*
* Emit an 'exit' event.
- *
- * Write the exit-code that will be passed to exit() or returned
- * from main().
- *
- * Use this prior to actually calling exit().
- * See "#define exit()" in git-compat-util.h
*/
-int trace2_cmd_exit_fl(const char *file, int line, int code);
+void trace2_cmd_exit_fl(const char *file, int line, int code);
#define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))
--
2.36.1.1100.g16130010d07
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
2022-05-31 16:58 ` [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
@ 2022-06-01 18:37 ` Junio C Hamano
0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:37 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
Glen Choo, Andrei Rybak, Emily Shaffer
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> diff --git a/common-main.c b/common-main.c
> index 29fb7452f8a..bb0100f6024 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -56,9 +56,24 @@ int main(int argc, const char **argv)
> result = cmd_main(argc, argv);
>
> /*
> - * We define exit() to call trace2_cmd_exit_fl() in
> - * git-compat-util.h. Whether we reach this or exit()
> + * We define exit() to call common_exit(), which will in turn
> + * call trace2_cmd_exit_fl(). Whether we reach this or exit()
> * elsewhere we'll always run our trace2 exit handler.
> */
I was expecting to see this comment to speak in a more generic terms
(i.e. "after the main processing is done, we clean up after
ourselves, either by exiting cmd_main and coming here or explicitly
calling exit()"), now we have introduced common_exit().
> exit(result);
> }
> +
> +int common_exit(const char *file, int line, int code)
> +{
> + /*
> + * For non-POSIX systems: Take the lowest 8 bits of the "code"
> + * to e.g. turn -1 into 255. On a POSIX system this is
> + * redundant, see exit(3) and wait(2), but as it doesn't harm
> + * anything there we don't need to guard this with an "ifdef".
> + */
> + code &= 0xff;
And I expected that the trace2 specific comment, if it is still
needed, would migrate here before this call.
Not huge enough to cause a reroll, but if there are other reasons to
see an updated version, it would be good to fix it while at it.
> + trace2_cmd_exit_fl(file, line, code);
> +
> + return code;
> +}
> +/**
> + * The exit() function is defined as common_exit() in
> + * git-compat-util.h.
> + *
> + * Intercepting the exit() allows us to hook in at-exit behavior, such
> + * emitting trace2 logging before calling the real exit().
> + */
> +int common_exit(const char *file, int line, int code);
Nicely explained.
Thanks.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG()
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:58 ` Ævar Arnfjörð Bjarmason
2022-05-31 18:18 ` Glen Choo
2022-06-01 18:50 ` Junio C Hamano
2022-05-31 16:58 ` [PATCH v2 3/6] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 2 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Add a bug() function to use in cases where we'd like to indicate a
runtime BUG(), but would like to defer the BUG() call because we're
possibly accumulating more bug() callers to exhaustively indicate what
went wrong.
We already have this sort of facility in various parts of the
codebase, just in the form of ad-hoc re-inventions of the
functionality that this new API provides. E.g. this will be used to
replace optbug() in parse-options.c, and the 'error("BUG:[...]' we do
in a loop in builtin/receive-pack.c.
Unlike the code this replaces we'll log to trace2 with this new bug()
function (as with other usage.c functions, including BUG()), we'll
also be able to avoid calls to xstrfmt() in some cases, as the bug()
function itself accepts variadic sprintf()-like arguments.
Any caller to bug() can follow up such calls with BUG_if_bug(),
which will BUG() out (i.e. abort()) if there were any preceding calls
to bug(), callers can also decide not to call BUG_if_bug() and leave
the resulting BUG() invocation until exit() time. There are currently
no bug() API users that don't call BUG_if_bug() themselves after a
for-loop, but allowing for not calling BUG_if_bug() keeps the API
flexible. As the tests and documentation here show we'll catch missing
BUG_if_bug() invocations in our exit() wrapper.
I'd previously proposed this as part of another series[1], in that
use-case we ended thinking a BUG() would be better (and eventually
96e41f58fe1 (fsck: report invalid object type-path combinations,
2021-10-01) ended up with neither). Here though we'll end up
converting various existing code that was already doing what we're
doing better with this new API.
1. https://lore.kernel.org/git/YGRXomWwRYPdXZi3@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
.../technical/api-error-handling.txt | 19 ++++++-
Documentation/technical/api-trace2.txt | 4 +-
common-main.c | 11 ++++
git-compat-util.h | 12 +++++
t/helper/test-trace2.c | 22 +++++++-
t/t0210-trace2-normal.sh | 52 +++++++++++++++++++
usage.c | 30 +++++++++--
7 files changed, 140 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt
index 8be4f4d0d6a..c4131dc3a2c 100644
--- a/Documentation/technical/api-error-handling.txt
+++ b/Documentation/technical/api-error-handling.txt
@@ -1,12 +1,29 @@
Error reporting in git
======================
-`BUG`, `die`, `usage`, `error`, and `warning` report errors of
+`BUG`, `bug`, `die`, `usage`, `error`, and `warning` report errors of
various kinds.
- `BUG` is for failed internal assertions that should never happen,
i.e. a bug in git itself.
+- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but
+ prints a "BUG" message instead of calling `abort()`.
++
+A call to `bug()` will then result in a "real" call to the `BUG()`
+function, either explicitly by invoking `BUG_if_bug()` after call(s)
+to `bug()`, or implicitly at `exit()` time where we'll check if we
+encountered any outstanding `bug()` invocations.
++
+If there were no prior calls to `bug()` before invoking `BUG_if_bug()`
+the latter is a NOOP. The `BUG_if_bug()` function takes the same
+arguments as `BUG()` itself. Calling `BUG_if_bug()` explicitly isn't
+necessary, but ensures that we die as soon as possible.
++
+This is for the convenience of APIs who'd like to potentially report
+more than one "bug", such as the optbug() validation in
+parse-options.c.
+
- `die` is for fatal application errors. It prints a message to
the user and exits with status 128.
diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
index f4a8a690878..77a150b30ee 100644
--- a/Documentation/technical/api-trace2.txt
+++ b/Documentation/technical/api-trace2.txt
@@ -465,8 +465,8 @@ completed.)
------------
`"error"`::
- This event is emitted when one of the `BUG()`, `error()`, `die()`,
- `warning()`, or `usage()` functions are called.
+ This event is emitted when one of the `BUG()`, `bug()`, `error()`,
+ `die()`, `warning()`, or `usage()` functions are called.
+
------------
{
diff --git a/common-main.c b/common-main.c
index bb0100f6024..0e35243724d 100644
--- a/common-main.c
+++ b/common-main.c
@@ -63,6 +63,16 @@ int main(int argc, const char **argv)
exit(result);
}
+static void check_bug_if_BUG(void)
+{
+ if (!bug_called_must_BUG)
+ return;
+
+ /* BUG_vfl() calls exit(), which calls us again */
+ bug_called_must_BUG = 0;
+ BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
+}
+
int common_exit(const char *file, int line, int code)
{
/*
@@ -73,6 +83,7 @@ int common_exit(const char *file, int line, int code)
*/
code &= 0xff;
+ check_bug_if_BUG();
trace2_cmd_exit_fl(file, line, code);
return code;
diff --git a/git-compat-util.h b/git-compat-util.h
index a446a363de2..a80079a06fe 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1320,9 +1320,21 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
/* usage.c: only to be used for testing BUG() implementation (see test-tool) */
extern int BUG_exit_code;
+/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */
+extern int bug_called_must_BUG;
+
__attribute__((format (printf, 3, 4))) NORETURN
void BUG_fl(const char *file, int line, const char *fmt, ...);
#define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
+__attribute__((format (printf, 3, 4)))
+void bug_fl(const char *file, int line, const char *fmt, ...);
+#define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define BUG_if_bug(...) do { \
+ if (bug_called_must_BUG) { \
+ bug_called_must_BUG = 0; \
+ BUG_fl(__FILE__, __LINE__, __VA_ARGS__); \
+ } \
+} while (0)
#ifdef __APPLE__
#define FSYNC_METHOD_DEFAULT FSYNC_METHOD_WRITEOUT_ONLY
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index 59b124bb5f1..97346554674 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -198,7 +198,7 @@ static int ut_006data(int argc, const char **argv)
return 0;
}
-static int ut_007bug(int argc, const char **argv)
+static int ut_007BUG(int argc, const char **argv)
{
/*
* Exercise BUG() to ensure that the message is printed to trace2.
@@ -206,6 +206,22 @@ static int ut_007bug(int argc, const char **argv)
BUG("the bug message");
}
+static int ut_008bug(int argc, const char **argv)
+{
+ bug("a bug message");
+ bug("another bug message");
+ BUG_if_bug("an explicit BUG_if_bug() following bug() call(s) is nice, but not required");
+ return 0;
+}
+
+static int ut_009bug_BUG(int argc, const char **argv)
+{
+ bug("a bug message");
+ bug("another bug message");
+ /* The BUG_if_bug(...) isn't here, but we'll spot bug() calls on exit()! */
+ return 0;
+}
+
/*
* Usage:
* test-tool trace2 <ut_name_1> <ut_usage_1>
@@ -222,7 +238,9 @@ static struct unit_test ut_table[] = {
{ ut_004child, "004child", "[<child_command_line>]" },
{ ut_005exec, "005exec", "<git_command_args>" },
{ ut_006data, "006data", "[<category> <key> <value>]+" },
- { ut_007bug, "007bug", "" },
+ { ut_007BUG, "007bug", "" },
+ { ut_008bug, "008bug", "" },
+ { ut_009bug_BUG, "009bug_BUG","" },
};
/* clang-format on */
diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
index 37c359bd5a2..1b63945cf91 100755
--- a/t/t0210-trace2-normal.sh
+++ b/t/t0210-trace2-normal.sh
@@ -168,6 +168,58 @@ test_expect_success 'BUG messages are written to trace2' '
test_cmp expect actual
'
+test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '
+ test_when_finished "rm trace.normal actual expect" &&
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
+ test-tool trace2 008bug 2>err &&
+ cat >expect <<-\EOF &&
+ a bug message
+ another bug message
+ an explicit BUG_if_bug() following bug() call(s) is nice, but not required
+ EOF
+ sed "s/^.*: //" <err >actual &&
+ test_cmp expect actual &&
+
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
+ cat >expect <<-EOF &&
+ version $V
+ start _EXE_ trace2 008bug
+ cmd_name trace2 (trace2)
+ error a bug message
+ error another bug message
+ error an explicit BUG_if_bug() following bug() call(s) is nice, but not required
+ exit elapsed:_TIME_ code:99
+ atexit elapsed:_TIME_ code:99
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'bug messages without explicit BUG_if_bug() are written to trace2' '
+ test_when_finished "rm trace.normal actual expect" &&
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
+ test-tool trace2 009bug_BUG 2>err &&
+ cat >expect <<-\EOF &&
+ a bug message
+ another bug message
+ had bug() call(s) in this process without explicit BUG_if_bug()
+ EOF
+ sed "s/^.*: //" <err >actual &&
+ test_cmp expect actual &&
+
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
+ cat >expect <<-EOF &&
+ version $V
+ start _EXE_ trace2 009bug_BUG
+ cmd_name trace2 (trace2)
+ error a bug message
+ error another bug message
+ error on exit(): had bug() call(s) in this process without explicit BUG_if_bug()
+ exit elapsed:_TIME_ code:99
+ atexit elapsed:_TIME_ code:99
+ EOF
+ test_cmp expect actual
+'
+
sane_unset GIT_TRACE2_BRIEF
# Now test without environment variables and get all Trace2 settings
diff --git a/usage.c b/usage.c
index b738dd178b3..c37b94f1a40 100644
--- a/usage.c
+++ b/usage.c
@@ -290,18 +290,24 @@ void warning(const char *warn, ...)
/* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
int BUG_exit_code;
-static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)
+static void BUG_vfl_common(const char *file, int line, const char *fmt,
+ va_list params)
{
char prefix[256];
- va_list params_copy;
- static int in_bug;
-
- va_copy(params_copy, params);
/* truncation via snprintf is OK here */
snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
vreportf(prefix, fmt, params);
+}
+
+static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)
+{
+ va_list params_copy;
+ static int in_bug;
+
+ va_copy(params_copy, params);
+ BUG_vfl_common(file, line, fmt, params);
if (in_bug)
abort();
@@ -322,6 +328,20 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
va_end(ap);
}
+int bug_called_must_BUG;
+void bug_fl(const char *file, int line, const char *fmt, ...)
+{
+ va_list ap, cp;
+
+ bug_called_must_BUG = 1;
+
+ va_copy(cp, ap);
+ va_start(ap, fmt);
+ BUG_vfl_common(file, line, fmt, ap);
+ va_end(ap);
+ trace2_cmd_error_va(fmt, cp);
+}
+
#ifdef SUPPRESS_ANNOTATED_LEAKS
void unleak_memory(const void *ptr, size_t len)
{
--
2.36.1.1100.g16130010d07
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG()
2022-05-31 16:58 ` [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
@ 2022-05-31 18:18 ` Glen Choo
2022-06-01 18:50 ` Junio C Hamano
1 sibling, 0 replies; 49+ messages in thread
From: Glen Choo @ 2022-05-31 18:18 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> +static void check_bug_if_BUG(void)
> +{
> + if (!bug_called_must_BUG)
> + return;
> +
> + /* BUG_vfl() calls exit(), which calls us again */
> + bug_called_must_BUG = 0;
> + BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
> +}
Hm thinking out loud here, what if we set bug_called_must_BUG = 0 inside
of BUG()? Then we'd be guaranteed that BUG() will never infinitely
recurse.
Another benefit is that we can invoke bug() and follow up with BUG() to
unconditionally exit without a bogus warning "had bug() call(s) without
explicit BUG_if_bug()". This pattern could be useful for 6/6 [1], where
we no longer exit after printing the "BUG:" messages.
Just an idea :)
[1] https://lore.kernel.org/git/patch-v2-6.6-cbbe0276966-20220531T164806Z-avarab@gmail.com
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG()
2022-05-31 16:58 ` [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
2022-05-31 18:18 ` Glen Choo
@ 2022-06-01 18:50 ` Junio C Hamano
1 sibling, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:50 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
Glen Choo, Andrei Rybak, Emily Shaffer
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Unlike the code this replaces we'll log to trace2 with this new bug()
> function (as with other usage.c functions, including BUG()),
I do not terribly mind repeated fprintf(stderr, ...) or error() in a
loop, but this aspect of the change is probably among the two things
that make the series shine (the other obviously being bug_if() which
allows us to lose the "did we see any bug?" counter).
> Any caller to bug() can follow up such calls with BUG_if_bug(),
> which will BUG() out (i.e. abort()) if there were any preceding calls
> to bug(), callers can also decide not to call BUG_if_bug() and leave
> the resulting BUG() invocation until exit() time. There are currently
> no bug() API users that don't call BUG_if_bug() themselves after a
> for-loop, but allowing for not calling BUG_if_bug() keeps the API
> flexible. As the tests and documentation here show we'll catch missing
> BUG_if_bug() invocations in our exit() wrapper.
OK.
> I'd previously proposed this as part of another series[1], in that
> use-case we ended thinking a BUG() would be better (and eventually
> 96e41f58fe1 (fsck: report invalid object type-path combinations,
> 2021-10-01) ended up with neither). Here though we'll end up
> converting various existing code that was already doing what we're
> doing better with this new API.
I think the last paragraph is an after-three-dash material.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 3/6] parse-options.c: use new bug() API for optbug()
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:58 ` Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
When we run into bugs in parse-options.c usage it's good to be able to
note all the issues we ran into before dying. This use-case is why we
have the optbug() function introduced in 1e5ce570ca3 (parse-options:
clearer reporting of API misuse, 2010-12-02)
Let's change this code to use the new bug() API introduced in the
preceding commit, which cuts down on the verbosity of
parse_options_check().
There are existing uses of BUG() in adjacent code that should have
been using optbug() that aren't being changed here. That'll be done in
a subsequent commit. This only changes the optbug() callers.
Since this will invoke BUG() the previous exit(128) code will be
changed, but in this case that's what we want, i.e. to have
encountering a BUG() return the specific "BUG" exit code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
parse-options.c | 36 +++++++++++++++++-------------------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 6e57744fd22..78b46ae9698 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -14,15 +14,15 @@ enum opt_parsed {
OPT_UNSET = 1<<1,
};
-static int optbug(const struct option *opt, const char *reason)
+static void optbug(const struct option *opt, const char *reason)
{
- if (opt->long_name) {
- if (opt->short_name)
- return error("BUG: switch '%c' (--%s) %s",
- opt->short_name, opt->long_name, reason);
- return error("BUG: option '%s' %s", opt->long_name, reason);
- }
- return error("BUG: switch '%c' %s", opt->short_name, reason);
+ if (opt->long_name && opt->short_name)
+ bug("switch '%c' (--%s) %s", opt->short_name,
+ opt->long_name, reason);
+ else if (opt->long_name)
+ bug("option '%s' %s", opt->long_name, reason);
+ else
+ bug("switch '%c' %s", opt->short_name, reason);
}
static const char *optname(const struct option *opt, enum opt_parsed flags)
@@ -441,28 +441,27 @@ static void check_typos(const char *arg, const struct option *options)
static void parse_options_check(const struct option *opts)
{
- int err = 0;
char short_opts[128];
memset(short_opts, '\0', sizeof(short_opts));
for (; opts->type != OPTION_END; opts++) {
if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
(opts->flags & PARSE_OPT_OPTARG))
- err |= optbug(opts, "uses incompatible flags "
- "LASTARG_DEFAULT and OPTARG");
+ optbug(opts, "uses incompatible flags "
+ "LASTARG_DEFAULT and OPTARG");
if (opts->short_name) {
if (0x7F <= opts->short_name)
- err |= optbug(opts, "invalid short name");
+ optbug(opts, "invalid short name");
else if (short_opts[opts->short_name]++)
- err |= optbug(opts, "short name already used");
+ optbug(opts, "short name already used");
}
if (opts->flags & PARSE_OPT_NODASH &&
((opts->flags & PARSE_OPT_OPTARG) ||
!(opts->flags & PARSE_OPT_NOARG) ||
!(opts->flags & PARSE_OPT_NONEG) ||
opts->long_name))
- err |= optbug(opts, "uses feature "
- "not supported for dashless options");
+ optbug(opts, "uses feature "
+ "not supported for dashless options");
switch (opts->type) {
case OPTION_COUNTUP:
case OPTION_BIT:
@@ -471,7 +470,7 @@ static void parse_options_check(const struct option *opts)
case OPTION_NUMBER:
if ((opts->flags & PARSE_OPT_OPTARG) ||
!(opts->flags & PARSE_OPT_NOARG))
- err |= optbug(opts, "should not accept an argument");
+ optbug(opts, "should not accept an argument");
break;
case OPTION_CALLBACK:
if (!opts->callback && !opts->ll_callback)
@@ -494,10 +493,9 @@ static void parse_options_check(const struct option *opts)
}
if (opts->argh &&
strcspn(opts->argh, " _") != strlen(opts->argh))
- err |= optbug(opts, "multi-word argh should use dash to separate words");
+ optbug(opts, "multi-word argh should use dash to separate words");
}
- if (err)
- exit(128);
+ BUG_if_bug("invalid 'struct option'");
}
static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
--
2.36.1.1100.g16130010d07
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-05-31 16:58 ` [PATCH v2 3/6] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:58 ` Ævar Arnfjörð Bjarmason
2022-05-31 17:38 ` Josh Steadmon
2022-05-31 16:58 ` [PATCH v2 5/6] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Change the assertions added in bf3ff338a25 (parse-options: stop
abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
instead of BUG(). At this point we're looping over individual options,
so if we encounter any issues we'd like to report the offending option.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
parse-options.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 78b46ae9698..243016ae30f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -473,21 +473,30 @@ static void parse_options_check(const struct option *opts)
optbug(opts, "should not accept an argument");
break;
case OPTION_CALLBACK:
- if (!opts->callback && !opts->ll_callback)
- BUG("OPTION_CALLBACK needs one callback");
- if (opts->callback && opts->ll_callback)
- BUG("OPTION_CALLBACK can't have two callbacks");
+ if (!opts->callback && !opts->ll_callback) {
+ optbug(opts, "OPTION_CALLBACK needs one callback");
+ break;
+ }
+ if (opts->callback && opts->ll_callback) {
+ optbug(opts, "OPTION_CALLBACK can't have two callbacks");
+ break;
+ }
break;
case OPTION_LOWLEVEL_CALLBACK:
- if (!opts->ll_callback)
- BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
- if (opts->callback)
- BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+ if (!opts->ll_callback) {
+ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
+ break;
+ }
+ if (opts->callback) {
+ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+ break;
+ }
break;
case OPTION_ALIAS:
- BUG("OPT_ALIAS() should not remain at this point. "
- "Are you using parse_options_step() directly?\n"
- "That case is not supported yet.");
+ optbug(opts, "OPT_ALIAS() should not remain at this point. "
+ "Are you using parse_options_step() directly?\n"
+ "That case is not supported yet.");
+ break;
default:
; /* ok. (usually accepts an argument) */
}
--
2.36.1.1100.g16130010d07
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
2022-05-31 16:58 ` [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
@ 2022-05-31 17:38 ` Josh Steadmon
2022-06-01 18:55 ` Junio C Hamano
0 siblings, 1 reply; 49+ messages in thread
From: Josh Steadmon @ 2022-05-31 17:38 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Glen Choo, Andrei Rybak, Emily Shaffer
On 2022.05.31 18:58, Ævar Arnfjörð Bjarmason wrote:
> Change the assertions added in bf3ff338a25 (parse-options: stop
> abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
> instead of BUG(). At this point we're looping over individual options,
> so if we encounter any issues we'd like to report the offending option.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> parse-options.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 78b46ae9698..243016ae30f 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -473,21 +473,30 @@ static void parse_options_check(const struct option *opts)
> optbug(opts, "should not accept an argument");
> break;
> case OPTION_CALLBACK:
> - if (!opts->callback && !opts->ll_callback)
> - BUG("OPTION_CALLBACK needs one callback");
> - if (opts->callback && opts->ll_callback)
> - BUG("OPTION_CALLBACK can't have two callbacks");
> + if (!opts->callback && !opts->ll_callback) {
> + optbug(opts, "OPTION_CALLBACK needs one callback");
> + break;
> + }
> + if (opts->callback && opts->ll_callback) {
> + optbug(opts, "OPTION_CALLBACK can't have two callbacks");
> + break;
> + }
> break;
> case OPTION_LOWLEVEL_CALLBACK:
> - if (!opts->ll_callback)
> - BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
> - if (opts->callback)
> - BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
> + if (!opts->ll_callback) {
> + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
> + break;
> + }
> + if (opts->callback) {
> + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
> + break;
> + }
> break;
A minor point, but I'm not sure I understand why we're adding breaks for
the two cases above. In the OPTION_CALLBACK case, the if conditions are
mutually exclusive and are followed by an unconditional break, so adding
additional breaks seems unnecessary. For the OPTION_LOWLEVEL_CALLBACK
case, the conditions are not mutually exclusive, but isn't this exactly
the issue that optbug() is intended to address? I.e., wouldn't the
caller want to see both optbug()s if both are relevant?
> case OPTION_ALIAS:
> - BUG("OPT_ALIAS() should not remain at this point. "
> - "Are you using parse_options_step() directly?\n"
> - "That case is not supported yet.");
> + optbug(opts, "OPT_ALIAS() should not remain at this point. "
> + "Are you using parse_options_step() directly?\n"
> + "That case is not supported yet.");
> + break;
> default:
> ; /* ok. (usually accepts an argument) */
> }
> --
> 2.36.1.1100.g16130010d07
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
2022-05-31 17:38 ` Josh Steadmon
@ 2022-06-01 18:55 ` Junio C Hamano
0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:55 UTC (permalink / raw)
To: Josh Steadmon
Cc: Ævar Arnfjörð Bjarmason, git, Bagas Sanjaya,
Abhradeep Chakraborty, Glen Choo, Andrei Rybak, Emily Shaffer
Josh Steadmon <steadmon@google.com> writes:
>> + if (!opts->ll_callback) {
>> + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
>> + break;
>> + }
>> + if (opts->callback) {
>> + optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
>> + break;
>> + }
>> break;
>
> A minor point, but I'm not sure I understand why we're adding breaks for
> the two cases above. In the OPTION_CALLBACK case, the if conditions are
> mutually exclusive and are followed by an unconditional break, so adding
> additional breaks seems unnecessary.
Yeah, good thinking.
> For the OPTION_LOWLEVEL_CALLBACK
> case, the conditions are not mutually exclusive, but isn't this exactly
> the issue that optbug() is intended to address? I.e., wouldn't the
> caller want to see both optbug()s if both are relevant?
Exactly.
Thanks for a careful reading.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 5/6] receive-pack: use bug() and BUG_if_bug()
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-05-31 16:58 ` [PATCH v2 4/6] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:58 ` Ævar Arnfjörð Bjarmason
2022-05-31 16:58 ` [PATCH v2 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Amend code added in a6a84319686 (receive-pack.c: shorten the
execute_commands loop over all commands, 2015-01-07) and amended to
hard die in b6a4788586d (receive-pack.c: die instead of error in case
of possible future bug, 2015-01-07) to use the new bug() function
instead.
Let's also rename the warn_if_*() function that code is in to
BUG_if_*(), its name became outdated in b6a4788586d.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/receive-pack.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index daa153d0446..4a62327dee9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1810,21 +1810,17 @@ static int should_process_cmd(struct command *cmd)
return !cmd->error_string && !cmd->skip_update;
}
-static void warn_if_skipped_connectivity_check(struct command *commands,
+static void BUG_if_skipped_connectivity_check(struct command *commands,
struct shallow_info *si)
{
struct command *cmd;
- int checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd->next) {
- if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {
- error("BUG: connectivity check has not been run on ref %s",
- cmd->ref_name);
- checked_connectivity = 0;
- }
+ if (should_process_cmd(cmd) && si->shallow_ref[cmd->index])
+ bug("connectivity check has not been run on ref %s",
+ cmd->ref_name);
}
- if (!checked_connectivity)
- BUG("connectivity check skipped???");
+ BUG_if_bug("connectivity check skipped???");
}
static void execute_commands_non_atomic(struct command *commands,
@@ -2005,7 +2001,7 @@ static void execute_commands(struct command *commands,
execute_commands_non_atomic(commands, si);
if (shallow_update)
- warn_if_skipped_connectivity_check(commands, si);
+ BUG_if_skipped_connectivity_check(commands, si);
}
static struct command **queue_command(struct command **tail,
--
2.36.1.1100.g16130010d07
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v2 6/6] cache-tree.c: use bug() and BUG_if_bug()
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-05-31 16:58 ` [PATCH v2 5/6] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
@ 2022-05-31 16:58 ` Ævar Arnfjörð Bjarmason
2022-06-01 18:52 ` Junio C Hamano
2022-05-31 17:39 ` [PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug() Josh Steadmon
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
7 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-05-31 16:58 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Change "BUG" output originally added in a97e4075a16 (Keep
rename/rename conflicts of intermediate merges while doing recursive
merge, 2007-03-31), and later made to say it was a "BUG" in
19c6a4f8369 (merge-recursive: do not return NULL only to cause
segfault, 2010-01-21) to use the new bug() function.
This gets the same job done with slightly less code, as we won't need
to prefix lines with "BUG: ". More importantly we'll now log the full
set of messages via trace2, before this we'd only log the one BUG()
invocation.
While we're at it let's replace "There" with "there" in the message,
i.e. not start a message with a capital letter, per the
CodingGuidelines.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
cache-tree.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index 6752f69d515..b91995af602 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -692,14 +692,14 @@ struct tree* write_in_core_index_as_tree(struct repository *repo) {
ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL);
if (ret == WRITE_TREE_UNMERGED_INDEX) {
int i;
- fprintf(stderr, "BUG: There are unmerged index entries:\n");
+ bug("there are unmerged index entries:");
for (i = 0; i < index_state->cache_nr; i++) {
const struct cache_entry *ce = index_state->cache[i];
if (ce_stage(ce))
- fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
- (int)ce_namelen(ce), ce->name);
+ bug("%d %.*s", ce_stage(ce),
+ (int)ce_namelen(ce), ce->name);
}
- BUG("unmerged index entries when writing inmemory index");
+ bug("unmerged index entries when writing inmemory index");
}
return lookup_tree(repo, &index_state->cache_tree->oid);
--
2.36.1.1100.g16130010d07
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v2 6/6] cache-tree.c: use bug() and BUG_if_bug()
2022-05-31 16:58 ` [PATCH v2 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-06-01 18:52 ` Junio C Hamano
0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-01 18:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
Glen Choo, Andrei Rybak, Emily Shaffer
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Change "BUG" output originally added in a97e4075a16 (Keep
> rename/rename conflicts of intermediate merges while doing recursive
> merge, 2007-03-31), and later made to say it was a "BUG" in
> 19c6a4f8369 (merge-recursive: do not return NULL only to cause
> segfault, 2010-01-21) to use the new bug() function.
>
> This gets the same job done with slightly less code, as we won't need
> to prefix lines with "BUG: ". More importantly we'll now log the full
> set of messages via trace2, before this we'd only log the one BUG()
> invocation.
>
> While we're at it let's replace "There" with "there" in the message,
> i.e. not start a message with a capital letter, per the
> CodingGuidelines.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> cache-tree.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 6752f69d515..b91995af602 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -692,14 +692,14 @@ struct tree* write_in_core_index_as_tree(struct repository *repo) {
> ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL);
> if (ret == WRITE_TREE_UNMERGED_INDEX) {
> int i;
> - fprintf(stderr, "BUG: There are unmerged index entries:\n");
> + bug("there are unmerged index entries:");
> for (i = 0; i < index_state->cache_nr; i++) {
> const struct cache_entry *ce = index_state->cache[i];
> if (ce_stage(ce))
> - fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
> - (int)ce_namelen(ce), ce->name);
> + bug("%d %.*s", ce_stage(ce),
> + (int)ce_namelen(ce), ce->name);
> }
> - BUG("unmerged index entries when writing inmemory index");
> + bug("unmerged index entries when writing inmemory index");
> }
I do not quite understand the last change here. Shouldn't this be
either a BUG() that flushes previous bug() or BUG_if_bug() followed
by either a BUG() or an exit()?
>
> return lookup_tree(repo, &index_state->cache_tree->oid);
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug()
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2022-05-31 16:58 ` [PATCH v2 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-05-31 17:39 ` Josh Steadmon
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
7 siblings, 0 replies; 49+ messages in thread
From: Josh Steadmon @ 2022-05-31 17:39 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Glen Choo, Andrei Rybak, Emily Shaffer
On 2022.05.31 18:58, Ævar Arnfjörð Bjarmason wrote:
> This series adds a bug() (lower-case) function to go along with
> BUG(). As seen in 3-5/6 this makes it much easier to handle the cases
> such as parse-options.c where we'd like to call BUG(), but would like
> to first exhaustively accumulate the N issues we spot before doing so,
> and not merely BUG() out on the first one.
>
> Changes since v1
> (https://lore.kernel.org/git/cover-0.5-00000000000-20220521T170939Z-avarab@gmail.com/):
>
> * Move the exit() wrapper to common-main.c, I tried to add a
> "common-exit.c" or just rename "common-main.c" to "common.c", but
> due to how the CMake build system declares it those changes would
> result in a lot of churn, so for now just adding it to
> common-main.c makes more sense.
>
> * Typo/grammar fixes in commit messages, as pointed out in review.
>
> * The BUG_if_bug() function is now optional, and the docs have been
> updated to reflect that.
>
> * The BUG_if_bug() function now takes a va_args like BUG() to
> indicate what the problem was.
>
> * Updated 3/6 to note that the exit(128) code changes with a
> migration to BUG().
>
> * Fix logic error in 4/6: We now "break" after calling bug(), to
> behave as the previous code did.
>
> * Fix logic error in 5/6, which now makes use of the new argument(s)
> to BUG_if_bug().
>
> * There was some suggestion of ejecting 6/6, but I've instead
> replaced it with the implementation Glen suggested in
> http://lore.kernel.org/git/kl6lk0a6mzp0.fsf@chooglen-macbookpro.roam.corp.google.com
>
> Ævar Arnfjörð Bjarmason (6):
> common-main.o: move non-trace2 exit() behavior out of trace2.c
> usage.c: add a non-fatal bug() function to go with BUG()
> parse-options.c: use new bug() API for optbug()
> parse-options.c: use optbug() instead of BUG() "opts" check
> receive-pack: use bug() and BUG_if_bug()
> cache-tree.c: use bug() and BUG_if_bug()
>
> .../technical/api-error-handling.txt | 19 +++++-
> Documentation/technical/api-trace2.txt | 4 +-
> builtin/receive-pack.c | 16 ++---
> cache-tree.c | 8 +--
> common-main.c | 30 ++++++++-
> git-compat-util.h | 24 ++++++-
> parse-options.c | 67 ++++++++++---------
> t/helper/test-trace2.c | 22 +++++-
> t/t0210-trace2-normal.sh | 52 ++++++++++++++
> trace2.c | 8 +--
> trace2.h | 8 +--
> usage.c | 30 +++++++--
> 12 files changed, 217 insertions(+), 71 deletions(-)
Thanks for the updates! I have a minor question about 4/6, but
regardless of if/how that's addressed, I think this version looks good.
Reviewed-by: Josh Steadmon <steadmon@google.com>
^ permalink raw reply [flat|nested] 49+ messages in thread* [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug()
2022-05-31 16:58 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2022-05-31 17:39 ` [PATCH v2 0/6] usage API: add and use a bug() + BUG_if_bug() Josh Steadmon
@ 2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
` (6 more replies)
7 siblings, 7 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
This series adds a bug() (lower-case) function to go along with
BUG(). As seen in 3-5/6 this makes it much easier to handle the cases
such as parse-options.c where we'd like to call BUG(), but would like
to first exhaustively accumulate the N issues we spot before doing so,
and not merely BUG() out on the first one.
Changes since v2[1]:
* Fix up commentary in 1/6 to address Junio's suggestions, and
de-duplicate previously added commentary (just the one existing
comment in git-compat-util.h sufficed).
* The BUG() function itself now clears bug_called_must_BUG, as
suggested by Glen. This simplifies things, and allows "BUG()" to be
called after a sequence of "BUG()" instead of "BUG_if_bug()". That
pattern is now used in 6/6.
I pondered adding a message that BUG() would emit in that case,
i.e. to say that we'd previously called bug() and were now calling
BUG(), but left it. I think the result in 6/6 is better as a
result.
* Used "else if" in 4/6 and got rid of "break" to exhaustively log
errors.
* Got rid of a "below the fold" part of a commit message.
1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20220531T164806Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (6):
common-main.o: move non-trace2 exit() behavior out of trace2.c
usage.c: add a non-fatal bug() function to go with BUG()
parse-options.c: use new bug() API for optbug()
parse-options.c: use optbug() instead of BUG() "opts" check
receive-pack: use bug() and BUG_if_bug()
cache-tree.c: use bug() and BUG_if_bug()
.../technical/api-error-handling.txt | 24 +++++-
Documentation/technical/api-trace2.txt | 4 +-
builtin/receive-pack.c | 16 ++--
cache-tree.c | 6 +-
common-main.c | 28 ++++++-
git-compat-util.h | 14 +++-
parse-options.c | 53 +++++++------
t/helper/test-trace2.c | 29 ++++++-
t/t0210-trace2-normal.sh | 76 +++++++++++++++++++
trace2.c | 8 +-
trace2.h | 8 +-
usage.c | 33 ++++++--
12 files changed, 230 insertions(+), 69 deletions(-)
Range-diff against v2:
1: d446e4679d4 ! 1: 9c4f8d840e9 common-main.o: move non-trace2 exit() behavior out of trace2.c
@@ Commit message
## common-main.c ##
@@ common-main.c: int main(int argc, const char **argv)
+
result = cmd_main(argc, argv);
- /*
-- * We define exit() to call trace2_cmd_exit_fl() in
-- * git-compat-util.h. Whether we reach this or exit()
-+ * We define exit() to call common_exit(), which will in turn
-+ * call trace2_cmd_exit_fl(). Whether we reach this or exit()
- * elsewhere we'll always run our trace2 exit handler.
- */
- exit(result);
- }
++ /* Not exit(3), but a wrapper calling our common_exit() */
++ exit(result);
++}
+
++/* We wrap exit() to call common_exit() in git-compat-util.h */
+int common_exit(const char *file, int line, int code)
+{
-+ /*
+ /*
+- * We define exit() to call trace2_cmd_exit_fl() in
+- * git-compat-util.h. Whether we reach this or exit()
+- * elsewhere we'll always run our trace2 exit handler.
+ * For non-POSIX systems: Take the lowest 8 bits of the "code"
+ * to e.g. turn -1 into 255. On a POSIX system this is
+ * redundant, see exit(3) and wait(2), but as it doesn't harm
+ * anything there we don't need to guard this with an "ifdef".
-+ */
+ */
+- exit(result);
+ code &= 0xff;
+
+ trace2_cmd_exit_fl(file, line, code);
+
+ return code;
-+}
+ }
## git-compat-util.h ##
-@@ git-compat-util.h: static inline int is_missing_file_error(int errno_)
-
- int cmd_main(int, const char **);
-
-+/**
-+ * The exit() function is defined as common_exit() in
-+ * git-compat-util.h.
-+ *
-+ * Intercepting the exit() allows us to hook in at-exit behavior, such
-+ * emitting trace2 logging before calling the real exit().
-+ */
-+int common_exit(const char *file, int line, int code);
-+
- /*
+@@ git-compat-util.h: int cmd_main(int, const char **);
* Intercept all calls to exit() and route them to trace2 to
* optionally emit a message before calling the real exit().
*/
-int trace2_cmd_exit_fl(const char *file, int line, int code);
-#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))
++int common_exit(const char *file, int line, int code);
+#define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
/*
2: 2d0527f86dc ! 2: 033f373acaa usage.c: add a non-fatal bug() function to go with BUG()
@@ Commit message
flexible. As the tests and documentation here show we'll catch missing
BUG_if_bug() invocations in our exit() wrapper.
- I'd previously proposed this as part of another series[1], in that
- use-case we ended thinking a BUG() would be better (and eventually
- 96e41f58fe1 (fsck: report invalid object type-path combinations,
- 2021-10-01) ended up with neither). Here though we'll end up
- converting various existing code that was already doing what we're
- doing better with this new API.
-
- 1. https://lore.kernel.org/git/YGRXomWwRYPdXZi3@coredump.intra.peff.net/
-
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Documentation/technical/api-error-handling.txt ##
@@ Documentation/technical/api-error-handling.txt
+arguments as `BUG()` itself. Calling `BUG_if_bug()` explicitly isn't
+necessary, but ensures that we die as soon as possible.
++
++If you know you had prior calls to `bug()` then calling `BUG()` itself
++is equivalent to calling `BUG_if_bug()`, the latter being a wrapper
++calling `BUG()` if we've set a flag indicating that we've called
++`bug()`.
+++
+This is for the convenience of APIs who'd like to potentially report
+more than one "bug", such as the optbug() validation in
+parse-options.c.
@@ common-main.c: int main(int argc, const char **argv)
+{
+ if (!bug_called_must_BUG)
+ return;
-+
-+ /* BUG_vfl() calls exit(), which calls us again */
-+ bug_called_must_BUG = 0;
+ BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
+}
+
+ /* We wrap exit() to call common_exit() in git-compat-util.h */
int common_exit(const char *file, int line, int code)
{
- /*
@@ common-main.c: int common_exit(const char *file, int line, int code)
*/
code &= 0xff;
@@ git-compat-util.h: static inline int regexec_buf(const regex_t *preg, const char
+void bug_fl(const char *file, int line, const char *fmt, ...);
+#define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define BUG_if_bug(...) do { \
-+ if (bug_called_must_BUG) { \
-+ bug_called_must_BUG = 0; \
++ if (bug_called_must_BUG) \
+ BUG_fl(__FILE__, __LINE__, __VA_ARGS__); \
-+ } \
+} while (0)
#ifdef __APPLE__
@@ t/helper/test-trace2.c: static int ut_007bug(int argc, const char **argv)
+ /* The BUG_if_bug(...) isn't here, but we'll spot bug() calls on exit()! */
+ return 0;
+}
++
++static int ut_010bug_BUG(int argc, const char **argv)
++{
++ bug("a bug message");
++ BUG("a BUG message");
++}
+
/*
* Usage:
@@ t/helper/test-trace2.c: static struct unit_test ut_table[] = {
+ { ut_007BUG, "007bug", "" },
+ { ut_008bug, "008bug", "" },
+ { ut_009bug_BUG, "009bug_BUG","" },
++ { ut_010bug_BUG, "010bug_BUG","" },
};
/* clang-format on */
@@ t/t0210-trace2-normal.sh: test_expect_success 'BUG messages are written to trace
+ EOF
+ test_cmp expect actual
+'
++
++test_expect_success 'bug messages followed by BUG() are written to trace2' '
++ test_when_finished "rm trace.normal actual expect" &&
++ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
++ test-tool trace2 010bug_BUG 2>err &&
++ cat >expect <<-\EOF &&
++ a bug message
++ a BUG message
++ EOF
++ sed "s/^.*: //" <err >actual &&
++ test_cmp expect actual &&
++
++ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
++ cat >expect <<-EOF &&
++ version $V
++ start _EXE_ trace2 010bug_BUG
++ cmd_name trace2 (trace2)
++ error a bug message
++ error a BUG message
++ exit elapsed:_TIME_ code:99
++ atexit elapsed:_TIME_ code:99
++ EOF
++ test_cmp expect actual
++'
+
sane_unset GIT_TRACE2_BRIEF
@@ usage.c: void warning(const char *warn, ...)
if (in_bug)
abort();
-@@ usage.c: NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
+@@ usage.c: static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
+ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
+ {
+ va_list ap;
++
++ bug_called_must_BUG = 0;
++
+ va_start(ap, fmt);
+ BUG_vfl(file, line, fmt, ap);
va_end(ap);
}
3: 4a7089fbbf2 = 3: 3d8a8e95f4d parse-options.c: use new bug() API for optbug()
4: 47d384d0ae5 ! 4: 58eafa2e04a parse-options.c: use optbug() instead of BUG() "opts" check
@@ Commit message
## parse-options.c ##
@@ parse-options.c: static void parse_options_check(const struct option *opts)
- optbug(opts, "should not accept an argument");
break;
case OPTION_CALLBACK:
-- if (!opts->callback && !opts->ll_callback)
+ if (!opts->callback && !opts->ll_callback)
- BUG("OPTION_CALLBACK needs one callback");
- if (opts->callback && opts->ll_callback)
- BUG("OPTION_CALLBACK can't have two callbacks");
-+ if (!opts->callback && !opts->ll_callback) {
+ optbug(opts, "OPTION_CALLBACK needs one callback");
-+ break;
-+ }
-+ if (opts->callback && opts->ll_callback) {
++ else if (opts->callback && opts->ll_callback)
+ optbug(opts, "OPTION_CALLBACK can't have two callbacks");
-+ break;
-+ }
break;
case OPTION_LOWLEVEL_CALLBACK:
-- if (!opts->ll_callback)
+ if (!opts->ll_callback)
- BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
-- if (opts->callback)
-- BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
-+ if (!opts->ll_callback) {
+ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
-+ break;
-+ }
-+ if (opts->callback) {
+ if (opts->callback)
+- BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
-+ break;
-+ }
break;
case OPTION_ALIAS:
- BUG("OPT_ALIAS() should not remain at this point. "
5: fe5c3926675 = 5: 05fb94aee6e receive-pack: use bug() and BUG_if_bug()
6: cbbe0276966 ! 6: 754a66be365 cache-tree.c: use bug() and BUG_if_bug()
@@ Commit message
set of messages via trace2, before this we'd only log the one BUG()
invocation.
+ We don't replace the last "BUG()" invocation with "BUG_if_bug()", as
+ in this case we're sure that we called bug() earlier, so there's no
+ need to make it a conditional.
+
While we're at it let's replace "There" with "there" in the message,
i.e. not start a message with a capital letter, per the
CodingGuidelines.
@@ cache-tree.c: struct tree* write_in_core_index_as_tree(struct repository *repo)
+ bug("%d %.*s", ce_stage(ce),
+ (int)ce_namelen(ce), ce->name);
}
-- BUG("unmerged index entries when writing inmemory index");
-+ bug("unmerged index entries when writing inmemory index");
+ BUG("unmerged index entries when writing inmemory index");
}
-
- return lookup_tree(repo, &index_state->cache_tree->oid);
--
2.36.1.1103.gb3ecdfb3e6a
^ permalink raw reply [flat|nested] 49+ messages in thread* [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
@ 2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason
2022-06-03 1:19 ` Junio C Hamano
2022-06-07 20:09 ` Josh Steadmon
2022-06-02 12:25 ` [PATCH v3 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
6 siblings, 2 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Change the exit() wrapper added in ee4512ed481 (trace2: create new
combined trace facility, 2019-02-22) so that we'll split up the trace2
logging concerns from wanting to wrap the "exit()" function itself for
other purposes.
This makes more sense structurally, as we won't seem to conflate
non-trace2 behavior with the trace2 code. I'd previously added an
explanation for this in 368b5843158 (common-main.c: call exit(), don't
return, 2021-12-07), that comment is being adjusted here.
Now the only thing we'll do if we're not using trace2 is to truncate
the "code" argument to the lowest 8 bits.
We only need to do that truncation on non-POSIX systems, but in
ee4512ed481 that "if defined(__MINGW32__)" code added in
47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
2009-07-05) was made to run everywhere. It might be good for clarify
to narrow that down by an "ifdef" again, but I'm not certain that in
the interim we haven't had some other non-POSIX systems rely the
behavior. On a POSIX system taking the lowest 8 bits is implicit, see
exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
1. https://man7.org/linux/man-pages/man3/exit.3.html
2. https://man7.org/linux/man-pages/man2/wait.2.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
common-main.c | 20 ++++++++++++++++----
git-compat-util.h | 4 ++--
trace2.c | 8 ++------
trace2.h | 8 +-------
4 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/common-main.c b/common-main.c
index 29fb7452f8a..b6124dd2c2c 100644
--- a/common-main.c
+++ b/common-main.c
@@ -55,10 +55,22 @@ int main(int argc, const char **argv)
result = cmd_main(argc, argv);
+ /* Not exit(3), but a wrapper calling our common_exit() */
+ exit(result);
+}
+
+/* We wrap exit() to call common_exit() in git-compat-util.h */
+int common_exit(const char *file, int line, int code)
+{
/*
- * We define exit() to call trace2_cmd_exit_fl() in
- * git-compat-util.h. Whether we reach this or exit()
- * elsewhere we'll always run our trace2 exit handler.
+ * For non-POSIX systems: Take the lowest 8 bits of the "code"
+ * to e.g. turn -1 into 255. On a POSIX system this is
+ * redundant, see exit(3) and wait(2), but as it doesn't harm
+ * anything there we don't need to guard this with an "ifdef".
*/
- exit(result);
+ code &= 0xff;
+
+ trace2_cmd_exit_fl(file, line, code);
+
+ return code;
}
diff --git a/git-compat-util.h b/git-compat-util.h
index 96293b6c43a..1227ff80ca7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1451,8 +1451,8 @@ int cmd_main(int, const char **);
* Intercept all calls to exit() and route them to trace2 to
* optionally emit a message before calling the real exit().
*/
-int trace2_cmd_exit_fl(const char *file, int line, int code);
-#define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))
+int common_exit(const char *file, int line, int code);
+#define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
/*
* You can mark a stack variable with UNLEAK(var) to avoid it being
diff --git a/trace2.c b/trace2.c
index e01cf77f1a8..0c0a11e07d5 100644
--- a/trace2.c
+++ b/trace2.c
@@ -202,17 +202,15 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv)
argv);
}
-int trace2_cmd_exit_fl(const char *file, int line, int code)
+void trace2_cmd_exit_fl(const char *file, int line, int code)
{
struct tr2_tgt *tgt_j;
int j;
uint64_t us_now;
uint64_t us_elapsed_absolute;
- code &= 0xff;
-
if (!trace2_enabled)
- return code;
+ return;
trace_git_fsync_stats();
trace2_collect_process_info(TRACE2_PROCESS_INFO_EXIT);
@@ -226,8 +224,6 @@ int trace2_cmd_exit_fl(const char *file, int line, int code)
if (tgt_j->pfn_exit_fl)
tgt_j->pfn_exit_fl(file, line, us_elapsed_absolute,
code);
-
- return code;
}
void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
diff --git a/trace2.h b/trace2.h
index 1b109f57d0a..88d906ea830 100644
--- a/trace2.h
+++ b/trace2.h
@@ -101,14 +101,8 @@ void trace2_cmd_start_fl(const char *file, int line, const char **argv);
/*
* Emit an 'exit' event.
- *
- * Write the exit-code that will be passed to exit() or returned
- * from main().
- *
- * Use this prior to actually calling exit().
- * See "#define exit()" in git-compat-util.h
*/
-int trace2_cmd_exit_fl(const char *file, int line, int code);
+void trace2_cmd_exit_fl(const char *file, int line, int code);
#define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))
--
2.36.1.1103.gb3ecdfb3e6a
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
2022-06-02 12:25 ` [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
@ 2022-06-03 1:19 ` Junio C Hamano
2022-06-07 20:09 ` Josh Steadmon
1 sibling, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-03 1:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
Glen Choo, Andrei Rybak, Emily Shaffer
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Subject: Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
.o, not .c?
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
2022-06-02 12:25 ` [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
2022-06-03 1:19 ` Junio C Hamano
@ 2022-06-07 20:09 ` Josh Steadmon
2022-06-07 21:12 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 49+ messages in thread
From: Josh Steadmon @ 2022-06-07 20:09 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Glen Choo, Andrei Rybak, Emily Shaffer
On 2022.06.02 14:25, Ævar Arnfjörð Bjarmason wrote:
> Change the exit() wrapper added in ee4512ed481 (trace2: create new
> combined trace facility, 2019-02-22) so that we'll split up the trace2
> logging concerns from wanting to wrap the "exit()" function itself for
> other purposes.
>
> This makes more sense structurally, as we won't seem to conflate
> non-trace2 behavior with the trace2 code. I'd previously added an
> explanation for this in 368b5843158 (common-main.c: call exit(), don't
> return, 2021-12-07), that comment is being adjusted here.
>
> Now the only thing we'll do if we're not using trace2 is to truncate
> the "code" argument to the lowest 8 bits.
>
> We only need to do that truncation on non-POSIX systems, but in
> ee4512ed481 that "if defined(__MINGW32__)" code added in
> 47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
> 2009-07-05) was made to run everywhere. It might be good for clarify
> to narrow that down by an "ifdef" again, but I'm not certain that in
> the interim we haven't had some other non-POSIX systems rely the
> behavior. On a POSIX system taking the lowest 8 bits is implicit, see
> exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
>
> 1. https://man7.org/linux/man-pages/man3/exit.3.html
> 2. https://man7.org/linux/man-pages/man2/wait.2.html
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> common-main.c | 20 ++++++++++++++++----
> git-compat-util.h | 4 ++--
> trace2.c | 8 ++------
> trace2.h | 8 +-------
> 4 files changed, 21 insertions(+), 19 deletions(-)
Just realized that this unexpectedly breaks the fuzzer builds:
$ git checkout ab/bug-if-bug
$ make CC=clang CXX=clang++ \
CFLAGS="-fsanitize=fuzzer-no-link,address" \
LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
fuzz-all
[...]
LINK fuzz-pack-headers
LINK fuzz-pack-idx
/usr/bin/ld: archive.o: in function `parse_archive_args':
archive.c:(.text.parse_archive_args[parse_archive_args]+0x2cb8): undefined reference to `common_exit'
[...]
The issue is that we don't link the fuzzers against common-main.o
because the fuzzing engine provides its own main(). Would it be too much
of a pain to move this out to a new common-exit.c file?
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
2022-06-07 20:09 ` Josh Steadmon
@ 2022-06-07 21:12 ` Ævar Arnfjörð Bjarmason
2022-06-07 22:05 ` Josh Steadmon
0 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-07 21:12 UTC (permalink / raw)
To: Josh Steadmon
Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Glen Choo, Andrei Rybak, Emily Shaffer
On Tue, Jun 07 2022, Josh Steadmon wrote:
> On 2022.06.02 14:25, Ævar Arnfjörð Bjarmason wrote:
>> Change the exit() wrapper added in ee4512ed481 (trace2: create new
>> combined trace facility, 2019-02-22) so that we'll split up the trace2
>> logging concerns from wanting to wrap the "exit()" function itself for
>> other purposes.
>>
>> This makes more sense structurally, as we won't seem to conflate
>> non-trace2 behavior with the trace2 code. I'd previously added an
>> explanation for this in 368b5843158 (common-main.c: call exit(), don't
>> return, 2021-12-07), that comment is being adjusted here.
>>
>> Now the only thing we'll do if we're not using trace2 is to truncate
>> the "code" argument to the lowest 8 bits.
>>
>> We only need to do that truncation on non-POSIX systems, but in
>> ee4512ed481 that "if defined(__MINGW32__)" code added in
>> 47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
>> 2009-07-05) was made to run everywhere. It might be good for clarify
>> to narrow that down by an "ifdef" again, but I'm not certain that in
>> the interim we haven't had some other non-POSIX systems rely the
>> behavior. On a POSIX system taking the lowest 8 bits is implicit, see
>> exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
>>
>> 1. https://man7.org/linux/man-pages/man3/exit.3.html
>> 2. https://man7.org/linux/man-pages/man2/wait.2.html
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> common-main.c | 20 ++++++++++++++++----
>> git-compat-util.h | 4 ++--
>> trace2.c | 8 ++------
>> trace2.h | 8 +-------
>> 4 files changed, 21 insertions(+), 19 deletions(-)
>
> Just realized that this unexpectedly breaks the fuzzer builds:
>
> $ git checkout ab/bug-if-bug
> $ make CC=clang CXX=clang++ \
> CFLAGS="-fsanitize=fuzzer-no-link,address" \
> LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
> fuzz-all
>
> [...]
> LINK fuzz-pack-headers
> LINK fuzz-pack-idx
> /usr/bin/ld: archive.o: in function `parse_archive_args':
> archive.c:(.text.parse_archive_args[parse_archive_args]+0x2cb8): undefined reference to `common_exit'
> [...]
Hrm, that's a pain, sorry about that.
> The issue is that we don't link the fuzzers against common-main.o
> because the fuzzing engine provides its own main(). Would it be too much
> of a pain to move this out to a new common-exit.c file?
I could do that, I actually did that in a WIP copy, but figured it was
too much boilerplate to pass the list.
But why it is that we can't just link to common-main.o? Yeah the linker
error, but we already depend on modern clang here, so can't we just use
-Wl,--allow-multiple-definition?
This works for me with my local clang & GNU ld:
make CC=clang CXX=clang++ \
CFLAGS="-fsanitize=fuzzer-no-link,address" \
FUZZ_CXXFLAGS="-fsanitize=fuzzer-no-link,address -Wl,--allow-multiple-definition" \
LIB_FUZZING_ENGINE="common-main.o -fsanitize=fuzzer" fuzz-all
It seems to me that the $(FUZZ_PROGRAMS) rule is mostly trying to work
around that by size, i.e. re-declaring most of $(OBJECTS).
But maybe it's a no-go for some reason, maybe due to OSX ld?
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c
2022-06-07 21:12 ` Ævar Arnfjörð Bjarmason
@ 2022-06-07 22:05 ` Josh Steadmon
0 siblings, 0 replies; 49+ messages in thread
From: Josh Steadmon @ 2022-06-07 22:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Glen Choo, Andrei Rybak, Emily Shaffer
On 2022.06.07 23:12, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Jun 07 2022, Josh Steadmon wrote:
>
> > On 2022.06.02 14:25, Ævar Arnfjörð Bjarmason wrote:
> >> Change the exit() wrapper added in ee4512ed481 (trace2: create new
> >> combined trace facility, 2019-02-22) so that we'll split up the trace2
> >> logging concerns from wanting to wrap the "exit()" function itself for
> >> other purposes.
> >>
> >> This makes more sense structurally, as we won't seem to conflate
> >> non-trace2 behavior with the trace2 code. I'd previously added an
> >> explanation for this in 368b5843158 (common-main.c: call exit(), don't
> >> return, 2021-12-07), that comment is being adjusted here.
> >>
> >> Now the only thing we'll do if we're not using trace2 is to truncate
> >> the "code" argument to the lowest 8 bits.
> >>
> >> We only need to do that truncation on non-POSIX systems, but in
> >> ee4512ed481 that "if defined(__MINGW32__)" code added in
> >> 47e3de0e796 (MinGW: truncate exit()'s argument to lowest 8 bits,
> >> 2009-07-05) was made to run everywhere. It might be good for clarify
> >> to narrow that down by an "ifdef" again, but I'm not certain that in
> >> the interim we haven't had some other non-POSIX systems rely the
> >> behavior. On a POSIX system taking the lowest 8 bits is implicit, see
> >> exit(3)[1] and wait(2)[2]. Let's leave a comment about that instead.
> >>
> >> 1. https://man7.org/linux/man-pages/man3/exit.3.html
> >> 2. https://man7.org/linux/man-pages/man2/wait.2.html
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >> common-main.c | 20 ++++++++++++++++----
> >> git-compat-util.h | 4 ++--
> >> trace2.c | 8 ++------
> >> trace2.h | 8 +-------
> >> 4 files changed, 21 insertions(+), 19 deletions(-)
> >
> > Just realized that this unexpectedly breaks the fuzzer builds:
> >
> > $ git checkout ab/bug-if-bug
> > $ make CC=clang CXX=clang++ \
> > CFLAGS="-fsanitize=fuzzer-no-link,address" \
> > LIB_FUZZING_ENGINE="-fsanitize=fuzzer" \
> > fuzz-all
> >
> > [...]
> > LINK fuzz-pack-headers
> > LINK fuzz-pack-idx
> > /usr/bin/ld: archive.o: in function `parse_archive_args':
> > archive.c:(.text.parse_archive_args[parse_archive_args]+0x2cb8): undefined reference to `common_exit'
> > [...]
>
> Hrm, that's a pain, sorry about that.
>
> > The issue is that we don't link the fuzzers against common-main.o
> > because the fuzzing engine provides its own main(). Would it be too much
> > of a pain to move this out to a new common-exit.c file?
>
> I could do that, I actually did that in a WIP copy, but figured it was
> too much boilerplate to pass the list.
>
> But why it is that we can't just link to common-main.o? Yeah the linker
> error, but we already depend on modern clang here, so can't we just use
> -Wl,--allow-multiple-definition?
>
> This works for me with my local clang & GNU ld:
>
> make CC=clang CXX=clang++ \
> CFLAGS="-fsanitize=fuzzer-no-link,address" \
> FUZZ_CXXFLAGS="-fsanitize=fuzzer-no-link,address -Wl,--allow-multiple-definition" \
> LIB_FUZZING_ENGINE="common-main.o -fsanitize=fuzzer" fuzz-all
>
> It seems to me that the $(FUZZ_PROGRAMS) rule is mostly trying to work
> around that by size, i.e. re-declaring most of $(OBJECTS).
>
> But maybe it's a no-go for some reason, maybe due to OSX ld?
Ah yeah, that would probably work. The main concern is that we want
these to run via OSS-Fuzz[1], but I believe I can send a change to our
build script there to add that flag. Thanks for the suggestion!
[1]: https://google.github.io/oss-fuzz/
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 2/6] usage.c: add a non-fatal bug() function to go with BUG()
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
@ 2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 3/6] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Add a bug() function to use in cases where we'd like to indicate a
runtime BUG(), but would like to defer the BUG() call because we're
possibly accumulating more bug() callers to exhaustively indicate what
went wrong.
We already have this sort of facility in various parts of the
codebase, just in the form of ad-hoc re-inventions of the
functionality that this new API provides. E.g. this will be used to
replace optbug() in parse-options.c, and the 'error("BUG:[...]' we do
in a loop in builtin/receive-pack.c.
Unlike the code this replaces we'll log to trace2 with this new bug()
function (as with other usage.c functions, including BUG()), we'll
also be able to avoid calls to xstrfmt() in some cases, as the bug()
function itself accepts variadic sprintf()-like arguments.
Any caller to bug() can follow up such calls with BUG_if_bug(),
which will BUG() out (i.e. abort()) if there were any preceding calls
to bug(), callers can also decide not to call BUG_if_bug() and leave
the resulting BUG() invocation until exit() time. There are currently
no bug() API users that don't call BUG_if_bug() themselves after a
for-loop, but allowing for not calling BUG_if_bug() keeps the API
flexible. As the tests and documentation here show we'll catch missing
BUG_if_bug() invocations in our exit() wrapper.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
.../technical/api-error-handling.txt | 24 +++++-
Documentation/technical/api-trace2.txt | 4 +-
common-main.c | 8 ++
git-compat-util.h | 10 +++
t/helper/test-trace2.c | 29 ++++++-
t/t0210-trace2-normal.sh | 76 +++++++++++++++++++
usage.c | 33 ++++++--
7 files changed, 174 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt
index 8be4f4d0d6a..70bf1d3e522 100644
--- a/Documentation/technical/api-error-handling.txt
+++ b/Documentation/technical/api-error-handling.txt
@@ -1,12 +1,34 @@
Error reporting in git
======================
-`BUG`, `die`, `usage`, `error`, and `warning` report errors of
+`BUG`, `bug`, `die`, `usage`, `error`, and `warning` report errors of
various kinds.
- `BUG` is for failed internal assertions that should never happen,
i.e. a bug in git itself.
+- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but
+ prints a "BUG" message instead of calling `abort()`.
++
+A call to `bug()` will then result in a "real" call to the `BUG()`
+function, either explicitly by invoking `BUG_if_bug()` after call(s)
+to `bug()`, or implicitly at `exit()` time where we'll check if we
+encountered any outstanding `bug()` invocations.
++
+If there were no prior calls to `bug()` before invoking `BUG_if_bug()`
+the latter is a NOOP. The `BUG_if_bug()` function takes the same
+arguments as `BUG()` itself. Calling `BUG_if_bug()` explicitly isn't
+necessary, but ensures that we die as soon as possible.
++
+If you know you had prior calls to `bug()` then calling `BUG()` itself
+is equivalent to calling `BUG_if_bug()`, the latter being a wrapper
+calling `BUG()` if we've set a flag indicating that we've called
+`bug()`.
++
+This is for the convenience of APIs who'd like to potentially report
+more than one "bug", such as the optbug() validation in
+parse-options.c.
+
- `die` is for fatal application errors. It prints a message to
the user and exits with status 128.
diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
index f4a8a690878..77a150b30ee 100644
--- a/Documentation/technical/api-trace2.txt
+++ b/Documentation/technical/api-trace2.txt
@@ -465,8 +465,8 @@ completed.)
------------
`"error"`::
- This event is emitted when one of the `BUG()`, `error()`, `die()`,
- `warning()`, or `usage()` functions are called.
+ This event is emitted when one of the `BUG()`, `bug()`, `error()`,
+ `die()`, `warning()`, or `usage()` functions are called.
+
------------
{
diff --git a/common-main.c b/common-main.c
index b6124dd2c2c..c531372f3ff 100644
--- a/common-main.c
+++ b/common-main.c
@@ -59,6 +59,13 @@ int main(int argc, const char **argv)
exit(result);
}
+static void check_bug_if_BUG(void)
+{
+ if (!bug_called_must_BUG)
+ return;
+ BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
+}
+
/* We wrap exit() to call common_exit() in git-compat-util.h */
int common_exit(const char *file, int line, int code)
{
@@ -70,6 +77,7 @@ int common_exit(const char *file, int line, int code)
*/
code &= 0xff;
+ check_bug_if_BUG();
trace2_cmd_exit_fl(file, line, code);
return code;
diff --git a/git-compat-util.h b/git-compat-util.h
index 1227ff80ca7..ce007102f76 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1320,9 +1320,19 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
/* usage.c: only to be used for testing BUG() implementation (see test-tool) */
extern int BUG_exit_code;
+/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */
+extern int bug_called_must_BUG;
+
__attribute__((format (printf, 3, 4))) NORETURN
void BUG_fl(const char *file, int line, const char *fmt, ...);
#define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
+__attribute__((format (printf, 3, 4)))
+void bug_fl(const char *file, int line, const char *fmt, ...);
+#define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
+#define BUG_if_bug(...) do { \
+ if (bug_called_must_BUG) \
+ BUG_fl(__FILE__, __LINE__, __VA_ARGS__); \
+} while (0)
#ifdef __APPLE__
#define FSYNC_METHOD_DEFAULT FSYNC_METHOD_WRITEOUT_ONLY
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index 59b124bb5f1..180c7f53f31 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -198,7 +198,7 @@ static int ut_006data(int argc, const char **argv)
return 0;
}
-static int ut_007bug(int argc, const char **argv)
+static int ut_007BUG(int argc, const char **argv)
{
/*
* Exercise BUG() to ensure that the message is printed to trace2.
@@ -206,6 +206,28 @@ static int ut_007bug(int argc, const char **argv)
BUG("the bug message");
}
+static int ut_008bug(int argc, const char **argv)
+{
+ bug("a bug message");
+ bug("another bug message");
+ BUG_if_bug("an explicit BUG_if_bug() following bug() call(s) is nice, but not required");
+ return 0;
+}
+
+static int ut_009bug_BUG(int argc, const char **argv)
+{
+ bug("a bug message");
+ bug("another bug message");
+ /* The BUG_if_bug(...) isn't here, but we'll spot bug() calls on exit()! */
+ return 0;
+}
+
+static int ut_010bug_BUG(int argc, const char **argv)
+{
+ bug("a bug message");
+ BUG("a BUG message");
+}
+
/*
* Usage:
* test-tool trace2 <ut_name_1> <ut_usage_1>
@@ -222,7 +244,10 @@ static struct unit_test ut_table[] = {
{ ut_004child, "004child", "[<child_command_line>]" },
{ ut_005exec, "005exec", "<git_command_args>" },
{ ut_006data, "006data", "[<category> <key> <value>]+" },
- { ut_007bug, "007bug", "" },
+ { ut_007BUG, "007bug", "" },
+ { ut_008bug, "008bug", "" },
+ { ut_009bug_BUG, "009bug_BUG","" },
+ { ut_010bug_BUG, "010bug_BUG","" },
};
/* clang-format on */
diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh
index 37c359bd5a2..80e76a4695e 100755
--- a/t/t0210-trace2-normal.sh
+++ b/t/t0210-trace2-normal.sh
@@ -168,6 +168,82 @@ test_expect_success 'BUG messages are written to trace2' '
test_cmp expect actual
'
+test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '
+ test_when_finished "rm trace.normal actual expect" &&
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
+ test-tool trace2 008bug 2>err &&
+ cat >expect <<-\EOF &&
+ a bug message
+ another bug message
+ an explicit BUG_if_bug() following bug() call(s) is nice, but not required
+ EOF
+ sed "s/^.*: //" <err >actual &&
+ test_cmp expect actual &&
+
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
+ cat >expect <<-EOF &&
+ version $V
+ start _EXE_ trace2 008bug
+ cmd_name trace2 (trace2)
+ error a bug message
+ error another bug message
+ error an explicit BUG_if_bug() following bug() call(s) is nice, but not required
+ exit elapsed:_TIME_ code:99
+ atexit elapsed:_TIME_ code:99
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'bug messages without explicit BUG_if_bug() are written to trace2' '
+ test_when_finished "rm trace.normal actual expect" &&
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
+ test-tool trace2 009bug_BUG 2>err &&
+ cat >expect <<-\EOF &&
+ a bug message
+ another bug message
+ had bug() call(s) in this process without explicit BUG_if_bug()
+ EOF
+ sed "s/^.*: //" <err >actual &&
+ test_cmp expect actual &&
+
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
+ cat >expect <<-EOF &&
+ version $V
+ start _EXE_ trace2 009bug_BUG
+ cmd_name trace2 (trace2)
+ error a bug message
+ error another bug message
+ error on exit(): had bug() call(s) in this process without explicit BUG_if_bug()
+ exit elapsed:_TIME_ code:99
+ atexit elapsed:_TIME_ code:99
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'bug messages followed by BUG() are written to trace2' '
+ test_when_finished "rm trace.normal actual expect" &&
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
+ test-tool trace2 010bug_BUG 2>err &&
+ cat >expect <<-\EOF &&
+ a bug message
+ a BUG message
+ EOF
+ sed "s/^.*: //" <err >actual &&
+ test_cmp expect actual &&
+
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
+ cat >expect <<-EOF &&
+ version $V
+ start _EXE_ trace2 010bug_BUG
+ cmd_name trace2 (trace2)
+ error a bug message
+ error a BUG message
+ exit elapsed:_TIME_ code:99
+ atexit elapsed:_TIME_ code:99
+ EOF
+ test_cmp expect actual
+'
+
sane_unset GIT_TRACE2_BRIEF
# Now test without environment variables and get all Trace2 settings
diff --git a/usage.c b/usage.c
index b738dd178b3..79900d0287f 100644
--- a/usage.c
+++ b/usage.c
@@ -290,18 +290,24 @@ void warning(const char *warn, ...)
/* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
int BUG_exit_code;
-static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)
+static void BUG_vfl_common(const char *file, int line, const char *fmt,
+ va_list params)
{
char prefix[256];
- va_list params_copy;
- static int in_bug;
-
- va_copy(params_copy, params);
/* truncation via snprintf is OK here */
snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
vreportf(prefix, fmt, params);
+}
+
+static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)
+{
+ va_list params_copy;
+ static int in_bug;
+
+ va_copy(params_copy, params);
+ BUG_vfl_common(file, line, fmt, params);
if (in_bug)
abort();
@@ -317,11 +323,28 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
{
va_list ap;
+
+ bug_called_must_BUG = 0;
+
va_start(ap, fmt);
BUG_vfl(file, line, fmt, ap);
va_end(ap);
}
+int bug_called_must_BUG;
+void bug_fl(const char *file, int line, const char *fmt, ...)
+{
+ va_list ap, cp;
+
+ bug_called_must_BUG = 1;
+
+ va_copy(cp, ap);
+ va_start(ap, fmt);
+ BUG_vfl_common(file, line, fmt, ap);
+ va_end(ap);
+ trace2_cmd_error_va(fmt, cp);
+}
+
#ifdef SUPPRESS_ANNOTATED_LEAKS
void unleak_memory(const void *ptr, size_t len)
{
--
2.36.1.1103.gb3ecdfb3e6a
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v3 3/6] parse-options.c: use new bug() API for optbug()
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 1/6] common-main.o: move non-trace2 exit() behavior out of trace2.c Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 2/6] usage.c: add a non-fatal bug() function to go with BUG() Ævar Arnfjörð Bjarmason
@ 2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 4/6] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
When we run into bugs in parse-options.c usage it's good to be able to
note all the issues we ran into before dying. This use-case is why we
have the optbug() function introduced in 1e5ce570ca3 (parse-options:
clearer reporting of API misuse, 2010-12-02)
Let's change this code to use the new bug() API introduced in the
preceding commit, which cuts down on the verbosity of
parse_options_check().
There are existing uses of BUG() in adjacent code that should have
been using optbug() that aren't being changed here. That'll be done in
a subsequent commit. This only changes the optbug() callers.
Since this will invoke BUG() the previous exit(128) code will be
changed, but in this case that's what we want, i.e. to have
encountering a BUG() return the specific "BUG" exit code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
parse-options.c | 36 +++++++++++++++++-------------------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 6e57744fd22..78b46ae9698 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -14,15 +14,15 @@ enum opt_parsed {
OPT_UNSET = 1<<1,
};
-static int optbug(const struct option *opt, const char *reason)
+static void optbug(const struct option *opt, const char *reason)
{
- if (opt->long_name) {
- if (opt->short_name)
- return error("BUG: switch '%c' (--%s) %s",
- opt->short_name, opt->long_name, reason);
- return error("BUG: option '%s' %s", opt->long_name, reason);
- }
- return error("BUG: switch '%c' %s", opt->short_name, reason);
+ if (opt->long_name && opt->short_name)
+ bug("switch '%c' (--%s) %s", opt->short_name,
+ opt->long_name, reason);
+ else if (opt->long_name)
+ bug("option '%s' %s", opt->long_name, reason);
+ else
+ bug("switch '%c' %s", opt->short_name, reason);
}
static const char *optname(const struct option *opt, enum opt_parsed flags)
@@ -441,28 +441,27 @@ static void check_typos(const char *arg, const struct option *options)
static void parse_options_check(const struct option *opts)
{
- int err = 0;
char short_opts[128];
memset(short_opts, '\0', sizeof(short_opts));
for (; opts->type != OPTION_END; opts++) {
if ((opts->flags & PARSE_OPT_LASTARG_DEFAULT) &&
(opts->flags & PARSE_OPT_OPTARG))
- err |= optbug(opts, "uses incompatible flags "
- "LASTARG_DEFAULT and OPTARG");
+ optbug(opts, "uses incompatible flags "
+ "LASTARG_DEFAULT and OPTARG");
if (opts->short_name) {
if (0x7F <= opts->short_name)
- err |= optbug(opts, "invalid short name");
+ optbug(opts, "invalid short name");
else if (short_opts[opts->short_name]++)
- err |= optbug(opts, "short name already used");
+ optbug(opts, "short name already used");
}
if (opts->flags & PARSE_OPT_NODASH &&
((opts->flags & PARSE_OPT_OPTARG) ||
!(opts->flags & PARSE_OPT_NOARG) ||
!(opts->flags & PARSE_OPT_NONEG) ||
opts->long_name))
- err |= optbug(opts, "uses feature "
- "not supported for dashless options");
+ optbug(opts, "uses feature "
+ "not supported for dashless options");
switch (opts->type) {
case OPTION_COUNTUP:
case OPTION_BIT:
@@ -471,7 +470,7 @@ static void parse_options_check(const struct option *opts)
case OPTION_NUMBER:
if ((opts->flags & PARSE_OPT_OPTARG) ||
!(opts->flags & PARSE_OPT_NOARG))
- err |= optbug(opts, "should not accept an argument");
+ optbug(opts, "should not accept an argument");
break;
case OPTION_CALLBACK:
if (!opts->callback && !opts->ll_callback)
@@ -494,10 +493,9 @@ static void parse_options_check(const struct option *opts)
}
if (opts->argh &&
strcspn(opts->argh, " _") != strlen(opts->argh))
- err |= optbug(opts, "multi-word argh should use dash to separate words");
+ optbug(opts, "multi-word argh should use dash to separate words");
}
- if (err)
- exit(128);
+ BUG_if_bug("invalid 'struct option'");
}
static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
--
2.36.1.1103.gb3ecdfb3e6a
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v3 4/6] parse-options.c: use optbug() instead of BUG() "opts" check
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-06-02 12:25 ` [PATCH v3 3/6] parse-options.c: use new bug() API for optbug() Ævar Arnfjörð Bjarmason
@ 2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 5/6] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Change the assertions added in bf3ff338a25 (parse-options: stop
abusing 'callback' for lowlevel callbacks, 2019-01-27) to use optbug()
instead of BUG(). At this point we're looping over individual options,
so if we encounter any issues we'd like to report the offending option.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
parse-options.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 78b46ae9698..edf55d3ef5d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -474,20 +474,21 @@ static void parse_options_check(const struct option *opts)
break;
case OPTION_CALLBACK:
if (!opts->callback && !opts->ll_callback)
- BUG("OPTION_CALLBACK needs one callback");
- if (opts->callback && opts->ll_callback)
- BUG("OPTION_CALLBACK can't have two callbacks");
+ optbug(opts, "OPTION_CALLBACK needs one callback");
+ else if (opts->callback && opts->ll_callback)
+ optbug(opts, "OPTION_CALLBACK can't have two callbacks");
break;
case OPTION_LOWLEVEL_CALLBACK:
if (!opts->ll_callback)
- BUG("OPTION_LOWLEVEL_CALLBACK needs a callback");
+ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs a callback");
if (opts->callback)
- BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
+ optbug(opts, "OPTION_LOWLEVEL_CALLBACK needs no high level callback");
break;
case OPTION_ALIAS:
- BUG("OPT_ALIAS() should not remain at this point. "
- "Are you using parse_options_step() directly?\n"
- "That case is not supported yet.");
+ optbug(opts, "OPT_ALIAS() should not remain at this point. "
+ "Are you using parse_options_step() directly?\n"
+ "That case is not supported yet.");
+ break;
default:
; /* ok. (usually accepts an argument) */
}
--
2.36.1.1103.gb3ecdfb3e6a
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v3 5/6] receive-pack: use bug() and BUG_if_bug()
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-06-02 12:25 ` [PATCH v3 4/6] parse-options.c: use optbug() instead of BUG() "opts" check Ævar Arnfjörð Bjarmason
@ 2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason
2022-06-02 12:25 ` [PATCH v3 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
2022-06-02 16:56 ` [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug() Glen Choo
6 siblings, 0 replies; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Amend code added in a6a84319686 (receive-pack.c: shorten the
execute_commands loop over all commands, 2015-01-07) and amended to
hard die in b6a4788586d (receive-pack.c: die instead of error in case
of possible future bug, 2015-01-07) to use the new bug() function
instead.
Let's also rename the warn_if_*() function that code is in to
BUG_if_*(), its name became outdated in b6a4788586d.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/receive-pack.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index daa153d0446..4a62327dee9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1810,21 +1810,17 @@ static int should_process_cmd(struct command *cmd)
return !cmd->error_string && !cmd->skip_update;
}
-static void warn_if_skipped_connectivity_check(struct command *commands,
+static void BUG_if_skipped_connectivity_check(struct command *commands,
struct shallow_info *si)
{
struct command *cmd;
- int checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd->next) {
- if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {
- error("BUG: connectivity check has not been run on ref %s",
- cmd->ref_name);
- checked_connectivity = 0;
- }
+ if (should_process_cmd(cmd) && si->shallow_ref[cmd->index])
+ bug("connectivity check has not been run on ref %s",
+ cmd->ref_name);
}
- if (!checked_connectivity)
- BUG("connectivity check skipped???");
+ BUG_if_bug("connectivity check skipped???");
}
static void execute_commands_non_atomic(struct command *commands,
@@ -2005,7 +2001,7 @@ static void execute_commands(struct command *commands,
execute_commands_non_atomic(commands, si);
if (shallow_update)
- warn_if_skipped_connectivity_check(commands, si);
+ BUG_if_skipped_connectivity_check(commands, si);
}
static struct command **queue_command(struct command **tail,
--
2.36.1.1103.gb3ecdfb3e6a
^ permalink raw reply related [flat|nested] 49+ messages in thread* [PATCH v3 6/6] cache-tree.c: use bug() and BUG_if_bug()
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-06-02 12:25 ` [PATCH v3 5/6] receive-pack: use bug() and BUG_if_bug() Ævar Arnfjörð Bjarmason
@ 2022-06-02 12:25 ` Ævar Arnfjörð Bjarmason
2022-06-02 19:54 ` Junio C Hamano
2022-06-02 16:56 ` [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug() Glen Choo
6 siblings, 1 reply; 49+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-02 12:25 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Glen Choo, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Change "BUG" output originally added in a97e4075a16 (Keep
rename/rename conflicts of intermediate merges while doing recursive
merge, 2007-03-31), and later made to say it was a "BUG" in
19c6a4f8369 (merge-recursive: do not return NULL only to cause
segfault, 2010-01-21) to use the new bug() function.
This gets the same job done with slightly less code, as we won't need
to prefix lines with "BUG: ". More importantly we'll now log the full
set of messages via trace2, before this we'd only log the one BUG()
invocation.
We don't replace the last "BUG()" invocation with "BUG_if_bug()", as
in this case we're sure that we called bug() earlier, so there's no
need to make it a conditional.
While we're at it let's replace "There" with "there" in the message,
i.e. not start a message with a capital letter, per the
CodingGuidelines.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
cache-tree.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/cache-tree.c b/cache-tree.c
index 6752f69d515..63953bf7772 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -692,12 +692,12 @@ struct tree* write_in_core_index_as_tree(struct repository *repo) {
ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL);
if (ret == WRITE_TREE_UNMERGED_INDEX) {
int i;
- fprintf(stderr, "BUG: There are unmerged index entries:\n");
+ bug("there are unmerged index entries:");
for (i = 0; i < index_state->cache_nr; i++) {
const struct cache_entry *ce = index_state->cache[i];
if (ce_stage(ce))
- fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
- (int)ce_namelen(ce), ce->name);
+ bug("%d %.*s", ce_stage(ce),
+ (int)ce_namelen(ce), ce->name);
}
BUG("unmerged index entries when writing inmemory index");
}
--
2.36.1.1103.gb3ecdfb3e6a
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [PATCH v3 6/6] cache-tree.c: use bug() and BUG_if_bug()
2022-06-02 12:25 ` [PATCH v3 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-06-02 19:54 ` Junio C Hamano
0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2022-06-02 19:54 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Bagas Sanjaya, Abhradeep Chakraborty, Josh Steadmon,
Glen Choo, Andrei Rybak, Emily Shaffer
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Change "BUG" output originally added in a97e4075a16 (Keep
> rename/rename conflicts of intermediate merges while doing recursive
> merge, 2007-03-31), and later made to say it was a "BUG" in
> 19c6a4f8369 (merge-recursive: do not return NULL only to cause
> segfault, 2010-01-21) to use the new bug() function.
>
> This gets the same job done with slightly less code, as we won't need
> to prefix lines with "BUG: ". More importantly we'll now log the full
> set of messages via trace2, before this we'd only log the one BUG()
> invocation.
>
> We don't replace the last "BUG()" invocation with "BUG_if_bug()", as
> in this case we're sure that we called bug() earlier, so there's no
> need to make it a conditional.
>
> While we're at it let's replace "There" with "there" in the message,
> i.e. not start a message with a capital letter, per the
> CodingGuidelines.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> cache-tree.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 6752f69d515..63953bf7772 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -692,12 +692,12 @@ struct tree* write_in_core_index_as_tree(struct repository *repo) {
> ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL);
> if (ret == WRITE_TREE_UNMERGED_INDEX) {
> int i;
> - fprintf(stderr, "BUG: There are unmerged index entries:\n");
> + bug("there are unmerged index entries:");
> for (i = 0; i < index_state->cache_nr; i++) {
> const struct cache_entry *ce = index_state->cache[i];
> if (ce_stage(ce))
> - fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce),
> - (int)ce_namelen(ce), ce->name);
> + bug("%d %.*s", ce_stage(ce),
> + (int)ce_namelen(ce), ce->name);
> }
> BUG("unmerged index entries when writing inmemory index");
"git grep inmemory" shows no hits on the non-word. We say "in-core"
often, and it is even in the name of the function this new messages
is added ;-)
Will tweak as this is a new message.
Thanks.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3 0/6] usage API: add and use a bug() + BUG_if_bug()
2022-06-02 12:25 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2022-06-02 12:25 ` [PATCH v3 6/6] cache-tree.c: " Ævar Arnfjörð Bjarmason
@ 2022-06-02 16:56 ` Glen Choo
6 siblings, 0 replies; 49+ messages in thread
From: Glen Choo @ 2022-06-02 16:56 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Bagas Sanjaya, Abhradeep Chakraborty,
Josh Steadmon, Andrei Rybak, Emily Shaffer,
Ævar Arnfjörð Bjarmason
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> This series adds a bug() (lower-case) function to go along with
> BUG(). As seen in 3-5/6 this makes it much easier to handle the cases
> such as parse-options.c where we'd like to call BUG(), but would like
> to first exhaustively accumulate the N issues we spot before doing so,
> and not merely BUG() out on the first one.
>
> Changes since v2[1]:
>
> * Fix up commentary in 1/6 to address Junio's suggestions, and
> de-duplicate previously added commentary (just the one existing
> comment in git-compat-util.h sufficed).
>
> * The BUG() function itself now clears bug_called_must_BUG, as
> suggested by Glen. This simplifies things, and allows "BUG()" to be
> called after a sequence of "BUG()" instead of "BUG_if_bug()". That
> pattern is now used in 6/6.
>
> I pondered adding a message that BUG() would emit in that case,
> i.e. to say that we'd previously called bug() and were now calling
> BUG(), but left it. I think the result in 6/6 is better as a
> result.
>
> * Used "else if" in 4/6 and got rid of "break" to exhaustively log
> errors.
>
> * Got rid of a "below the fold" part of a commit message.
>
> 1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20220531T164806Z-avarab@gmail.com/
I didn't look too closely at patch 1/6 (since I'm not too familiar with
the trace2 stuff), but everything else looks great :)
(not sure if this is enough to give a Reviewed-By)
^ permalink raw reply [flat|nested] 49+ messages in thread