usage.c: add a non-fatal bug() function to go with BUG()
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ævar Arnfjörð Bjarmason committed
Jun 2, 2022 at 14:25 UTC
0cc05b044fd690f37565262a5b09f60c203c5218
7 files changed
+174
-10
Documentation/technical/api-error-handling.txt
+23
-1
@@ -1,12 +1,34 @@
1
Error reporting in git
2
======================
3
4
-`BUG`, `die`, `usage`, `error`, and `warning` report errors of
4
+`BUG`, `bug`, `die`, `usage`, `error`, and `warning` report errors of
5
various kinds.
6
7
- `BUG` is for failed internal assertions that should never happen,
8
i.e. a bug in git itself.
9
10
+- `bug` (lower-case, not `BUG`) is supposed to be used like `BUG` but
11
+ prints a "BUG" message instead of calling `abort()`.
12
++
13
+A call to `bug()` will then result in a "real" call to the `BUG()`
14
+function, either explicitly by invoking `BUG_if_bug()` after call(s)
15
+to `bug()`, or implicitly at `exit()` time where we'll check if we
16
+encountered any outstanding `bug()` invocations.
17
++
18
+If there were no prior calls to `bug()` before invoking `BUG_if_bug()`
19
+the latter is a NOOP. The `BUG_if_bug()` function takes the same
20
+arguments as `BUG()` itself. Calling `BUG_if_bug()` explicitly isn't
21
+necessary, but ensures that we die as soon as possible.
22
++
23
+If you know you had prior calls to `bug()` then calling `BUG()` itself
24
+is equivalent to calling `BUG_if_bug()`, the latter being a wrapper
25
+calling `BUG()` if we've set a flag indicating that we've called
26
+`bug()`.
27
++
28
+This is for the convenience of APIs who'd like to potentially report
29
+more than one "bug", such as the optbug() validation in
30
+parse-options.c.
31
+
32
- `die` is for fatal application errors. It prints a message to
33
the user and exits with status 128.
34
Documentation/technical/api-trace2.txt
+2
-2
@@ -465,8 +465,8 @@ completed.)
465
------------
466
467
`"error"`::
468
- This event is emitted when one of the `BUG()`, `error()`, `die()`,
469
- `warning()`, or `usage()` functions are called.
468
+ This event is emitted when one of the `BUG()`, `bug()`, `error()`,
469
+ `die()`, `warning()`, or `usage()` functions are called.
470
+
471
------------
472
{
common-main.c
+8
@@ -59,6 +59,13 @@ int main(int argc, const char **argv)
59
exit(result);
60
}
61
62
+static void check_bug_if_BUG(void)
63
+{
64
+ if (!bug_called_must_BUG)
65
+ return;
66
+ BUG("on exit(): had bug() call(s) in this process without explicit BUG_if_bug()");
67
+}
68
+
69
/* We wrap exit() to call common_exit() in git-compat-util.h */
70
int common_exit(const char *file, int line, int code)
71
{
@@ -70,6 +77,7 @@ int common_exit(const char *file, int line, int code)
77
*/
78
code &= 0xff;
79
80
+ check_bug_if_BUG();
81
trace2_cmd_exit_fl(file, line, code);
82
83
return code;
git-compat-util.h
+10
@@ -1320,9 +1320,19 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
1320
/* usage.c: only to be used for testing BUG() implementation (see test-tool) */
1321
extern int BUG_exit_code;
1322
1323
+/* usage.c: if bug() is called we should have a BUG_if_bug() afterwards */
1324
+extern int bug_called_must_BUG;
1325
+
1326
__attribute__((format (printf, 3, 4))) NORETURN
1327
void BUG_fl(const char *file, int line, const char *fmt, ...);
1328
#define BUG(...) BUG_fl(__FILE__, __LINE__, __VA_ARGS__)
1329
+__attribute__((format (printf, 3, 4)))
1330
+void bug_fl(const char *file, int line, const char *fmt, ...);
1331
+#define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
1332
+#define BUG_if_bug(...) do { \
1333
+ if (bug_called_must_BUG) \
1334
+ BUG_fl(__FILE__, __LINE__, __VA_ARGS__); \
1335
+} while (0)
1336
1337
#ifdef __APPLE__
1338
#define FSYNC_METHOD_DEFAULT FSYNC_METHOD_WRITEOUT_ONLY
t/helper/test-trace2.c
+27
-2
@@ -198,7 +198,7 @@ static int ut_006data(int argc, const char **argv)
198
return 0;
199
}
200
201
-static int ut_007bug(int argc, const char **argv)
201
+static int ut_007BUG(int argc, const char **argv)
202
{
203
/*
204
* Exercise BUG() to ensure that the message is printed to trace2.
@@ -206,6 +206,28 @@ static int ut_007bug(int argc, const char **argv)
206
BUG("the bug message");
207
}
208
209
+static int ut_008bug(int argc, const char **argv)
210
+{
211
+ bug("a bug message");
212
+ bug("another bug message");
213
+ BUG_if_bug("an explicit BUG_if_bug() following bug() call(s) is nice, but not required");
214
+ return 0;
215
+}
216
+
217
+static int ut_009bug_BUG(int argc, const char **argv)
218
+{
219
+ bug("a bug message");
220
+ bug("another bug message");
221
+ /* The BUG_if_bug(...) isn't here, but we'll spot bug() calls on exit()! */
222
+ return 0;
223
+}
224
+
225
+static int ut_010bug_BUG(int argc, const char **argv)
226
+{
227
+ bug("a bug message");
228
+ BUG("a BUG message");
229
+}
230
+
231
/*
232
* Usage:
233
* test-tool trace2 <ut_name_1> <ut_usage_1>
@@ -222,7 +244,10 @@ static struct unit_test ut_table[] = {
244
{ ut_004child, "004child", "[<child_command_line>]" },
245
{ ut_005exec, "005exec", "<git_command_args>" },
246
{ ut_006data, "006data", "[<category> <key> <value>]+" },
225
- { ut_007bug, "007bug", "" },
247
+ { ut_007BUG, "007bug", "" },
248
+ { ut_008bug, "008bug", "" },
249
+ { ut_009bug_BUG, "009bug_BUG","" },
250
+ { ut_010bug_BUG, "010bug_BUG","" },
251
};
252
/* clang-format on */
253
t/t0210-trace2-normal.sh
+76
@@ -168,6 +168,82 @@ test_expect_success 'BUG messages are written to trace2' '
168
test_cmp expect actual
169
'
170
171
+test_expect_success 'bug messages with BUG_if_bug() are written to trace2' '
172
+ test_when_finished "rm trace.normal actual expect" &&
173
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
174
+ test-tool trace2 008bug 2>err &&
175
+ cat >expect <<-\EOF &&
176
+ a bug message
177
+ another bug message
178
+ an explicit BUG_if_bug() following bug() call(s) is nice, but not required
179
+ EOF
180
+ sed "s/^.*: //" <err >actual &&
181
+ test_cmp expect actual &&
182
+
183
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
184
+ cat >expect <<-EOF &&
185
+ version $V
186
+ start _EXE_ trace2 008bug
187
+ cmd_name trace2 (trace2)
188
+ error a bug message
189
+ error another bug message
190
+ error an explicit BUG_if_bug() following bug() call(s) is nice, but not required
191
+ exit elapsed:_TIME_ code:99
192
+ atexit elapsed:_TIME_ code:99
193
+ EOF
194
+ test_cmp expect actual
195
+'
196
+
197
+test_expect_success 'bug messages without explicit BUG_if_bug() are written to trace2' '
198
+ test_when_finished "rm trace.normal actual expect" &&
199
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
200
+ test-tool trace2 009bug_BUG 2>err &&
201
+ cat >expect <<-\EOF &&
202
+ a bug message
203
+ another bug message
204
+ had bug() call(s) in this process without explicit BUG_if_bug()
205
+ EOF
206
+ sed "s/^.*: //" <err >actual &&
207
+ test_cmp expect actual &&
208
+
209
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
210
+ cat >expect <<-EOF &&
211
+ version $V
212
+ start _EXE_ trace2 009bug_BUG
213
+ cmd_name trace2 (trace2)
214
+ error a bug message
215
+ error another bug message
216
+ error on exit(): had bug() call(s) in this process without explicit BUG_if_bug()
217
+ exit elapsed:_TIME_ code:99
218
+ atexit elapsed:_TIME_ code:99
219
+ EOF
220
+ test_cmp expect actual
221
+'
222
+
223
+test_expect_success 'bug messages followed by BUG() are written to trace2' '
224
+ test_when_finished "rm trace.normal actual expect" &&
225
+ test_expect_code 99 env GIT_TRACE2="$(pwd)/trace.normal" \
226
+ test-tool trace2 010bug_BUG 2>err &&
227
+ cat >expect <<-\EOF &&
228
+ a bug message
229
+ a BUG message
230
+ EOF
231
+ sed "s/^.*: //" <err >actual &&
232
+ test_cmp expect actual &&
233
+
234
+ perl "$TEST_DIRECTORY/t0210/scrub_normal.perl" <trace.normal >actual &&
235
+ cat >expect <<-EOF &&
236
+ version $V
237
+ start _EXE_ trace2 010bug_BUG
238
+ cmd_name trace2 (trace2)
239
+ error a bug message
240
+ error a BUG message
241
+ exit elapsed:_TIME_ code:99
242
+ atexit elapsed:_TIME_ code:99
243
+ EOF
244
+ test_cmp expect actual
245
+'
246
+
247
sane_unset GIT_TRACE2_BRIEF
248
249
# Now test without environment variables and get all Trace2 settings
usage.c
+28
-5
@@ -290,18 +290,24 @@ void warning(const char *warn, ...)
290
/* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
291
int BUG_exit_code;
292
293
-static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)
293
+static void BUG_vfl_common(const char *file, int line, const char *fmt,
294
+ va_list params)
295
{
296
char prefix[256];
296
- va_list params_copy;
297
- static int in_bug;
298
-
299
- va_copy(params_copy, params);
297
298
/* truncation via snprintf is OK here */
299
snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
300
301
vreportf(prefix, fmt, params);
302
+}
303
+
304
+static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)
305
+{
306
+ va_list params_copy;
307
+ static int in_bug;
308
+
309
+ va_copy(params_copy, params);
310
+ BUG_vfl_common(file, line, fmt, params);
311
312
if (in_bug)
313
abort();
@@ -317,11 +323,28 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis
323
NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
324
{
325
va_list ap;
326
+
327
+ bug_called_must_BUG = 0;
328
+
329
va_start(ap, fmt);
330
BUG_vfl(file, line, fmt, ap);
331
va_end(ap);
332
}
333
334
+int bug_called_must_BUG;
335
+void bug_fl(const char *file, int line, const char *fmt, ...)
336
+{
337
+ va_list ap, cp;
338
+
339
+ bug_called_must_BUG = 1;
340
+
341
+ va_copy(cp, ap);
342
+ va_start(ap, fmt);
343
+ BUG_vfl_common(file, line, fmt, ap);
344
+ va_end(ap);
345
+ trace2_cmd_error_va(fmt, cp);
346
+}
347
+
348
#ifdef SUPPRESS_ANNOTATED_LEAKS
349
void unleak_memory(const void *ptr, size_t len)
350
{