{"thread":{"id":"57924","subject":"[RFC PATCH 2/3] refs API: rename \"abort\" callback to avoid macro clash","startedAt":"2022-05-26T00:31:43Z","lastAt":"2022-06-09T15:23:52Z","messageCount":16,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Bagas Sanjaya","Josh Steadmon","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"456136","messageId":"RFC-patch-2.3-6d21c43a4a9-20220525T234908Z-avarab@gmail.com","threadId":"57924","inReplyTo":"RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com","subject":"[RFC PATCH 2/3] refs API: rename \"abort\" callback to avoid macro clash","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-26T00:30:43Z","receivedAt":"2022-05-26T00:31:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In a subsequent commit we'll add an abort() macro to wrap the abort(3)\nfunction, unfortunately due to C syntax semantics having such a macro\nwould be a compilation error as long as we're invoking callbacks named\n\"abort()\".\n\nSo let's rename the callback \"abort\" callback added in\n3bc581b9406 (refs: introduce an iterator interface, 2016-06-18) to\n\"end\", which is the alternate naming that commit itself discusses,\ni.e. it says:\n\n    * ref_iterator_advance(): move to the next reference in the iteration\n    * ref_iterator_abort(): end the iteration before it is exhausted\n    * ref_iterator_peel(): peel the reference currently being looked at\n\nJust as \"peel=peel we can change that \"abort=end\" pair to be\n\"end=end\", let's leave \"advance=move\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n refs/debug.c          | 4 ++--\n refs/files-backend.c  | 4 ++--\n refs/iterator.c       | 8 ++++----\n refs/packed-backend.c | 2 +-\n refs/ref-cache.c      | 2 +-\n refs/refs-internal.h  | 2 +-\n 6 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex eed8bc94b04..3364a7fb261 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -214,7 +214,7 @@ static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)\n {\n \tstruct debug_ref_iterator *diter =\n \t\t(struct debug_ref_iterator *)ref_iterator;\n-\tint res = diter->iter->vtable->abort(diter->iter);\n+\tint res = diter->iter->vtable->end(diter->iter);\n \ttrace_printf_key(&trace_refs, \"iterator_abort: %d\\n\", res);\n \treturn res;\n }\n@@ -222,7 +222,7 @@ static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable debug_ref_iterator_vtable = {\n \t.advance = debug_ref_iterator_advance,\n \t.peel = debug_ref_iterator_peel,\n-\t.abort = debug_ref_iterator_abort,\n+\t.end = debug_ref_iterator_abort,\n };\n \n static struct ref_iterator *\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 8db7882aacb..3f0dde4ae10 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -824,7 +824,7 @@ static int files_ref_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable files_ref_iterator_vtable = {\n \t.advance = files_ref_iterator_advance,\n \t.peel = files_ref_iterator_peel,\n-\t.abort = files_ref_iterator_abort,\n+\t.end = files_ref_iterator_abort,\n };\n \n static struct ref_iterator *files_ref_iterator_begin(\n@@ -2224,7 +2224,7 @@ static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable files_reflog_iterator_vtable = {\n \t.advance = files_reflog_iterator_advance,\n \t.peel = files_reflog_iterator_peel,\n-\t.abort = files_reflog_iterator_abort,\n+\t.end = files_reflog_iterator_abort,\n };\n \n static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex b2e56bae1c6..f4a9546adb6 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -21,7 +21,7 @@ int ref_iterator_peel(struct ref_iterator *ref_iterator,\n \n int ref_iterator_abort(struct ref_iterator *ref_iterator)\n {\n-\treturn ref_iterator->vtable->abort(ref_iterator);\n+\treturn ref_iterator->vtable->end(ref_iterator);\n }\n \n void base_ref_iterator_init(struct ref_iterator *iter,\n@@ -66,7 +66,7 @@ static int empty_ref_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable empty_ref_iterator_vtable = {\n \t.advance = empty_ref_iterator_advance,\n \t.peel = empty_ref_iterator_peel,\n-\t.abort = empty_ref_iterator_abort,\n+\t.end = empty_ref_iterator_abort,\n };\n \n struct ref_iterator *empty_ref_iterator_begin(void)\n@@ -203,7 +203,7 @@ static int merge_ref_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable merge_ref_iterator_vtable = {\n \t.advance = merge_ref_iterator_advance,\n \t.peel = merge_ref_iterator_peel,\n-\t.abort = merge_ref_iterator_abort,\n+\t.end = merge_ref_iterator_abort,\n };\n \n struct ref_iterator *merge_ref_iterator_begin(\n@@ -380,7 +380,7 @@ static int prefix_ref_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable prefix_ref_iterator_vtable = {\n \t.advance = prefix_ref_iterator_advance,\n \t.peel = prefix_ref_iterator_peel,\n-\t.abort = prefix_ref_iterator_abort,\n+\t.end = prefix_ref_iterator_abort,\n };\n \n struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 97b68377673..ad23f734b0f 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -913,7 +913,7 @@ static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable packed_ref_iterator_vtable = {\n \t.advance = packed_ref_iterator_advance,\n \t.peel = packed_ref_iterator_peel,\n-\t.abort = packed_ref_iterator_abort\n+\t.end = packed_ref_iterator_abort\n };\n \n static struct ref_iterator *packed_ref_iterator_begin(\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 32afd8a40b0..b4acaac2fb5 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -458,7 +458,7 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\n static struct ref_iterator_vtable cache_ref_iterator_vtable = {\n \t.advance = cache_ref_iterator_advance,\n \t.peel = cache_ref_iterator_peel,\n-\t.abort = cache_ref_iterator_abort\n+\t.end = cache_ref_iterator_abort\n };\n \n struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 69f93b0e2ac..7f462273fba 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -478,7 +478,7 @@ typedef int ref_iterator_abort_fn(struct ref_iterator *ref_iterator);\n struct ref_iterator_vtable {\n \tref_iterator_advance_fn *advance;\n \tref_iterator_peel_fn *peel;\n-\tref_iterator_abort_fn *abort;\n+\tref_iterator_abort_fn *end;\n };\n \n /*\n-- \n2.36.1.1046.g586767a6996\n\n"},{"id":"456137","messageId":"RFC-patch-1.3-78431bdc8f0-20220525T234908Z-avarab@gmail.com","threadId":"57924","inReplyTo":"RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com","subject":"[RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-26T00:30:42Z","receivedAt":"2022-05-26T00:31:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the BUG() function invoked be \"test-tool\" to be the \"real\" one,\ninstead of one that avoids producing core files. In\na86303cb5d5 (test-tool: help verifying BUG() code paths, 2018-05-02)\nto test the (then recently added) BUG() function we faked up the\nabort() in favor of an exit with code 99.\n\nHowever, in doing so we've been fooling ourselves when it comes to\nwhat trace2 events we log. The events tested for in\n0a9dde4a04c (usage: trace2 BUG() invocations, 2021-02-05) are not the\nreal ones, but those that we emit only from the \"test-tool\".\n\nLet's just stop faking it, and call abort(). As a86303cb5d5 notes this\nwill produce core files on some OS's, but as the default behavior for\nthat is to dump them in the current directory they'll be placed in the\ntrash directory that we'll shortly me \"rm -rf\"-ing.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThere's a CI problem with the test_must_BUG wrapper here on Windows:\nhttps://github.com/avar/git/runs/6602059183?check_suite_focus=true#step:6:1361\n\n t/helper/test-tool.c           |  1 -\n t/t0210-trace2-normal.sh       |  4 +---\n t/t1406-submodule-ref-store.sh | 10 +++++-----\n t/test-lib-functions.sh        | 13 +++++++++++++\n usage.c                        |  5 -----\n 5 files changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 0424f7adf5d..99a10f294f5 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -110,7 +110,6 @@ int cmd_main(int argc, const char **argv)\n \t\tOPT_END()\n \t};\n \n-\tBUG_exit_code = 99;\n \targc = parse_options(argc, argv, NULL, options, test_tool_usage,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION |\n \t\t\t     PARSE_OPT_KEEP_ARGV0);\ndiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\nindex 37c359bd5a2..910a6af8058 100755\n--- a/t/t0210-trace2-normal.sh\n+++ b/t/t0210-trace2-normal.sh\n@@ -155,15 +155,13 @@ test_expect_success 'normal stream, error event' '\n \n test_expect_success 'BUG messages are written to trace2' '\n \ttest_when_finished \"rm trace.normal actual expect\" &&\n-\ttest_must_fail env GIT_TRACE2=\"$(pwd)/trace.normal\" test-tool trace2 007bug &&\n+\ttest_must_BUG env GIT_TRACE2=\"$(pwd)/trace.normal\" test-tool trace2 007bug &&\n \tperl \"$TEST_DIRECTORY/t0210/scrub_normal.perl\" <trace.normal >actual &&\n \tcat >expect <<-EOF &&\n \t\tversion $V\n \t\tstart _EXE_ trace2 007bug\n \t\tcmd_name trace2 (trace2)\n \t\terror the bug message\n-\t\texit elapsed:_TIME_ code:99\n-\t\tatexit elapsed:_TIME_ code:99\n \tEOF\n \ttest_cmp expect actual\n '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex e6a7f7334b6..6f9a16b7355 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -25,15 +25,15 @@ test_expect_success 'pack_refs() not allowed' '\n '\n \n test_expect_success 'create_symref() not allowed' '\n-\ttest_must_fail $RUN create-symref FOO refs/heads/main nothing\n+\ttest_must_BUG $RUN create-symref FOO refs/heads/main nothing\n '\n \n test_expect_success 'delete_refs() not allowed' '\n-\ttest_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag\n+\ttest_must_BUG $RUN delete-refs 0 nothing FOO refs/tags/new-tag\n '\n \n test_expect_success 'rename_refs() not allowed' '\n-\ttest_must_fail $RUN rename-ref refs/heads/main refs/heads/new-main\n+\ttest_must_BUG $RUN rename-ref refs/heads/main refs/heads/new-main\n '\n \n test_expect_success 'for_each_ref(refs/heads/)' '\n@@ -89,11 +89,11 @@ test_expect_success 'reflog_exists(HEAD)' '\n '\n \n test_expect_success 'delete_reflog() not allowed' '\n-\ttest_must_fail $RUN delete-reflog HEAD\n+\ttest_must_BUG $RUN delete-reflog HEAD\n '\n \n test_expect_success 'create-reflog() not allowed' '\n-\ttest_must_fail $RUN create-reflog HEAD\n+\ttest_must_BUG $RUN create-reflog HEAD\n '\n \n test_done\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 93c03380d44..61f1f03c099 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1167,6 +1167,9 @@ test_must_fail () {\n \tthen\n \t\techo >&4 \"test_must_fail: command succeeded: $*\"\n \t\treturn 1\n+\telif test_match_signal 6 $exit_code && list_contains \"$_test_ok\" sigabrt\n+\tthen\n+\t\treturn 0\n \telif test_match_signal 13 $exit_code && list_contains \"$_test_ok\" sigpipe\n \tthen\n \t\treturn 0\n@@ -1186,6 +1189,16 @@ test_must_fail () {\n \treturn 0\n } 7>&2 2>&4\n \n+# The test_must_BUG() function is a wrapper for test_must_fail which\n+# checks that we BUG() out.\n+#\n+# Currently this checks that we exit with abort(), but in the future\n+# we might e.g. check the trace2 logging itself, or otherwise make\n+# sure that we used BUG() in particular.\n+test_must_BUG () {\n+\ttest_must_fail ok=sigabrt \"$@\"\n+} 7>&2 2>&4\n+\n # Similar to test_must_fail, but tolerates success, too.  This is\n # meant to be used in contexts like:\n #\ndiff --git a/usage.c b/usage.c\nindex b738dd178b3..bf868b5be1f 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -287,9 +287,6 @@ void warning(const char *warn, ...)\n \tva_end(params);\n }\n \n-/* Only set this, ever, from t/helper/, when verifying that bugs are caught. */\n-int BUG_exit_code;\n-\n static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_list params)\n {\n \tchar prefix[256];\n@@ -309,8 +306,6 @@ static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, va_lis\n \n \ttrace2_cmd_error_va(fmt, params_copy);\n \n-\tif (BUG_exit_code)\n-\t\texit(BUG_exit_code);\n \tabort();\n }\n \n-- \n2.36.1.1046.g586767a6996\n\n"},{"id":"456138","messageId":"RFC-patch-3.3-b099a3a4a96-20220525T234908Z-avarab@gmail.com","threadId":"57924","inReplyTo":"RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com","subject":"[RFC PATCH 3/3] trace2: emit \"signal\" events after calling BUG()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-26T00:30:44Z","receivedAt":"2022-05-26T00:31:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a wrapper for the abort(3) function to correspond to the existing\nwrapper for exit(3). See ee4512ed481 (trace2: create new combined\ntrace facility, 2019-02-22) for the introduction of the exit(3)\nwrapper. See also the more recent 368b5843158 (common-main.c: call\nexit(), don't return, 2021-12-07) discussing how the exit() wrapper\nmacro it and the trace2 machinery interact.\n\nAs reported (off the ML) by Josh Steadmon we have \"start\" events that\nare not followed by corresponding \"exit\" events. I first considered\nlogging an \"exit\" here, but that would be incorrect, we didn't call\nexit, on abort() we do the equivalent of:\n\n\tkill(getpid(), SIGABRT);\n\nSo let's instead update the documentation to note that those\ninterested in seeing a marker for process exit need to be looking for\neither an \"exit\" or a \"signal\" event.\n\nWe then need to add a *_fl() function similar to the existing\n\"tr2main_signal_handler()\" function. Until now we'd only been emitting\nthese \"signal\" events from our own signal handlers.\n\nReported-by: Josh Steadmon <steadmon@google.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/technical/api-trace2.txt | 13 +++++++++----\n git-compat-util.h                      |  9 +++++++++\n t/t0210-trace2-normal.sh               |  1 +\n trace2.c                               | 19 +++++++++++++++++++\n trace2.h                               |  8 ++++++++\n trace2/tr2_tgt.h                       |  3 +++\n trace2/tr2_tgt_event.c                 | 11 +++++++++--\n trace2/tr2_tgt_normal.c                | 11 +++++++++--\n trace2/tr2_tgt_perf.c                  | 11 +++++++++--\n 9 files changed, 76 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt\nindex f4a8a690878..55ecfd8c30c 100644\n--- a/Documentation/technical/api-trace2.txt\n+++ b/Documentation/technical/api-trace2.txt\n@@ -421,7 +421,9 @@ only present on the \"start\" and \"atexit\" events.\n ------------\n \n `\"exit\"`::\n-\tThis event is emitted when git calls `exit()`.\n+\tThis event is emitted when git calls `exit()`. This event will\n+\tbe produced for all regular ending of the git process, but it\n+\tmight also exit via a \"signal\".\n +\n ------------\n {\n@@ -435,7 +437,7 @@ only present on the \"start\" and \"atexit\" events.\n `\"atexit\"`::\n \tThis event is emitted by the Trace2 `atexit` routine during\n \tfinal shutdown.  It should be the last event emitted by the\n-\tprocess.\n+\tprocess, unless it was aborted (see \"signal\").\n +\n (The elapsed time reported here is greater than the time reported in\n the \"exit\" event because it runs after all other atexit tasks have\n@@ -452,8 +454,11 @@ completed.)\n \n `\"signal\"`::\n \tThis event is emitted when the program is terminated by a user\n-\tsignal.  Depending on the platform, the signal event may\n-\tprevent the \"atexit\" event from being generated.\n+\tsignal, which includes git itself calling abort(3). Depending\n+\ton the platform, the signal event may prevent the \"exit\"\n+\tand/or \"atexit\" events from being generated. E.g. if BUG() was\n+\tinvoked we'll emit an \"error\" event followed by a \"signal\"\n+\tevent, and nothing else.\n +\n ------------\n {\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 58fd813bd01..ea3177b2771 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1403,6 +1403,15 @@ int cmd_main(int, const char **);\n int trace2_cmd_exit_fl(const char *file, int line, int code);\n #define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n \n+/*\n+ * Intercept calls to abort() to log that we aborted.\n+ */\n+void trace2_cmd_signal_fl(const char *file, int line, int signo);\n+#define abort() do { \\\n+\ttrace2_cmd_signal_fl(__FILE__, __LINE__, SIGABRT); \\\n+\tabort(); \\\n+} while (0)\n+\n /*\n  * You can mark a stack variable with UNLEAK(var) to avoid it being\n  * reported as a leak by tools like LSAN or valgrind. The argument\ndiff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\nindex 910a6af8058..25fffdba80e 100755\n--- a/t/t0210-trace2-normal.sh\n+++ b/t/t0210-trace2-normal.sh\n@@ -162,6 +162,7 @@ test_expect_success 'BUG messages are written to trace2' '\n \t\tstart _EXE_ trace2 007bug\n \t\tcmd_name trace2 (trace2)\n \t\terror the bug message\n+\t\tsignal elapsed:_TIME_ code:6\n \tEOF\n \ttest_cmp expect actual\n '\ndiff --git a/trace2.c b/trace2.c\nindex e01cf77f1a8..250b2424bfa 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -230,6 +230,25 @@ int trace2_cmd_exit_fl(const char *file, int line, int code)\n \treturn code;\n }\n \n+void trace2_cmd_signal_fl(const char *file, int line, int signo)\n+{\n+\tstruct tr2_tgt *tgt_j;\n+\tint j;\n+\tuint64_t us_now;\n+\tuint64_t us_elapsed_absolute;\n+\n+\tif (!trace2_enabled)\n+\t\treturn;\n+\n+\tus_now = getnanotime() / 1000;\n+\tus_elapsed_absolute = tr2tls_absolute_elapsed(us_now);\n+\n+\tfor_each_wanted_builtin (j, tgt_j)\n+\t\tif (tgt_j->pfn_signal_fl)\n+\t\t\ttgt_j->pfn_signal_fl(file, line, us_elapsed_absolute,\n+\t\t\t\t\t     signo);\n+}\n+\n void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,\n \t\t\t    va_list ap)\n {\ndiff --git a/trace2.h b/trace2.h\nindex 1b109f57d0a..4f456f2fc62 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -112,6 +112,14 @@ int trace2_cmd_exit_fl(const char *file, int line, int code);\n \n #define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n \n+/*\n+ * Emit a 'signal' event. Used to log that we called abort() e.g. via\n+ * the BUG() function.\n+ */\n+void trace2_cmd_signal_fl(const char *file, int line, int signo);\n+\n+#define trace2_cmd_signal(signo) (trace2_cmd_signal_fl(__FILE__, __LINE__, (signo)))\n+\n /*\n  * Emit an 'error' event.\n  *\ndiff --git a/trace2/tr2_tgt.h b/trace2/tr2_tgt.h\nindex 65f94e15748..fbc33650c5d 100644\n--- a/trace2/tr2_tgt.h\n+++ b/trace2/tr2_tgt.h\n@@ -20,6 +20,8 @@ typedef void(tr2_tgt_evt_start_fl_t)(const char *file, int line,\n typedef void(tr2_tgt_evt_exit_fl_t)(const char *file, int line,\n \t\t\t\t    uint64_t us_elapsed_absolute, int code);\n typedef void(tr2_tgt_evt_signal_t)(uint64_t us_elapsed_absolute, int signo);\n+typedef void(tr2_tgt_evt_signal_fl_t)(const char *file, int line,\n+\t\t\t\t      uint64_t us_elapsed_absolute, int signo);\n typedef void(tr2_tgt_evt_atexit_t)(uint64_t us_elapsed_absolute, int code);\n \n typedef void(tr2_tgt_evt_error_va_fl_t)(const char *file, int line,\n@@ -111,6 +113,7 @@ struct tr2_tgt {\n \ttr2_tgt_evt_start_fl_t                  *pfn_start_fl;\n \ttr2_tgt_evt_exit_fl_t                   *pfn_exit_fl;\n \ttr2_tgt_evt_signal_t                    *pfn_signal;\n+\ttr2_tgt_evt_signal_fl_t                 *pfn_signal_fl;\n \ttr2_tgt_evt_atexit_t                    *pfn_atexit;\n \ttr2_tgt_evt_error_va_fl_t               *pfn_error_va_fl;\n \ttr2_tgt_evt_command_path_fl_t           *pfn_command_path_fl;\ndiff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\nindex c5c8cfbbaa0..df947378a51 100644\n--- a/trace2/tr2_tgt_event.c\n+++ b/trace2/tr2_tgt_event.c\n@@ -177,14 +177,15 @@ static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n \tjw_release(&jw);\n }\n \n-static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n+static void fn_signal_fl(const char *file, int line,\n+\t\t\t uint64_t us_elapsed_absolute, int signo)\n {\n \tconst char *event_name = \"signal\";\n \tstruct json_writer jw = JSON_WRITER_INIT;\n \tdouble t_abs = (double)us_elapsed_absolute / 1000000.0;\n \n \tjw_object_begin(&jw, 0);\n-\tevent_fmt_prepare(event_name, __FILE__, __LINE__, NULL, &jw);\n+\tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n \tjw_object_double(&jw, \"t_abs\", 6, t_abs);\n \tjw_object_intmax(&jw, \"signo\", signo);\n \tjw_end(&jw);\n@@ -193,6 +194,11 @@ static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n \tjw_release(&jw);\n }\n \n+static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n+{\n+\tfn_signal_fl(__FILE__, __LINE__, us_elapsed_absolute, signo);\n+}\n+\n static void fn_atexit(uint64_t us_elapsed_absolute, int code)\n {\n \tconst char *event_name = \"atexit\";\n@@ -624,6 +630,7 @@ struct tr2_tgt tr2_tgt_event = {\n \t.pfn_start_fl = fn_start_fl,\n \t.pfn_exit_fl = fn_exit_fl,\n \t.pfn_signal = fn_signal,\n+\t.pfn_signal_fl = fn_signal_fl,\n \t.pfn_atexit = fn_atexit,\n \t.pfn_error_va_fl = fn_error_va_fl,\n \t.pfn_command_path_fl = fn_command_path_fl,\ndiff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\nindex c42fbade7f0..8e2bc5027fe 100644\n--- a/trace2/tr2_tgt_normal.c\n+++ b/trace2/tr2_tgt_normal.c\n@@ -105,17 +105,23 @@ static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n \tstrbuf_release(&buf_payload);\n }\n \n-static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n+static void fn_signal_fl(const char *file, int line,\n+\t\t\t uint64_t us_elapsed_absolute, int signo)\n {\n \tstruct strbuf buf_payload = STRBUF_INIT;\n \tdouble elapsed = (double)us_elapsed_absolute / 1000000.0;\n \n \tstrbuf_addf(&buf_payload, \"signal elapsed:%.6f code:%d\", elapsed,\n \t\t    signo);\n-\tnormal_io_write_fl(__FILE__, __LINE__, &buf_payload);\n+\tnormal_io_write_fl(file, line, &buf_payload);\n \tstrbuf_release(&buf_payload);\n }\n \n+static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n+{\n+\tfn_signal_fl(__FILE__, __LINE__, us_elapsed_absolute, signo);\n+}\n+\n static void fn_atexit(uint64_t us_elapsed_absolute, int code)\n {\n \tstruct strbuf buf_payload = STRBUF_INIT;\n@@ -336,6 +342,7 @@ struct tr2_tgt tr2_tgt_normal = {\n \t.pfn_start_fl = fn_start_fl,\n \t.pfn_exit_fl = fn_exit_fl,\n \t.pfn_signal = fn_signal,\n+\t.pfn_signal_fl = fn_signal_fl,\n \t.pfn_atexit = fn_atexit,\n \t.pfn_error_va_fl = fn_error_va_fl,\n \t.pfn_command_path_fl = fn_command_path_fl,\ndiff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\nindex a1eff8bea31..4ce18d3f12e 100644\n--- a/trace2/tr2_tgt_perf.c\n+++ b/trace2/tr2_tgt_perf.c\n@@ -193,18 +193,24 @@ static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n \tstrbuf_release(&buf_payload);\n }\n \n-static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n+static void fn_signal_fl(const char *file, int line,\n+\t\t\t uint64_t us_elapsed_absolute, int signo)\n {\n \tconst char *event_name = \"signal\";\n \tstruct strbuf buf_payload = STRBUF_INIT;\n \n \tstrbuf_addf(&buf_payload, \"signo:%d\", signo);\n \n-\tperf_io_write_fl(__FILE__, __LINE__, event_name, NULL,\n+\tperf_io_write_fl(file, line, event_name, NULL,\n \t\t\t &us_elapsed_absolute, NULL, NULL, &buf_payload);\n \tstrbuf_release(&buf_payload);\n }\n \n+static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n+{\n+\tfn_signal_fl(__FILE__, __LINE__, us_elapsed_absolute, signo);\n+}\n+\n static void fn_atexit(uint64_t us_elapsed_absolute, int code)\n {\n \tconst char *event_name = \"atexit\";\n@@ -560,6 +566,7 @@ struct tr2_tgt tr2_tgt_perf = {\n \t.pfn_start_fl = fn_start_fl,\n \t.pfn_exit_fl = fn_exit_fl,\n \t.pfn_signal = fn_signal,\n+\t.pfn_signal_fl = fn_signal_fl,\n \t.pfn_atexit = fn_atexit,\n \t.pfn_error_va_fl = fn_error_va_fl,\n \t.pfn_command_path_fl = fn_command_path_fl,\n-- \n2.36.1.1046.g586767a6996\n\n"},{"id":"456140","messageId":"RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com","threadId":"57924","inReplyTo":null,"subject":"[RFC PATCH 0/3] trace2: log \"signal\" end events if we invoke BUG()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-26T00:30:41Z","receivedAt":"2022-05-26T00:31:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Josh Steadmon noted in the Review Club tonight/today/yesterday\n(depending on the TZ) that he'd encountered trace2 event streams\nwithout an \"exit\" event, which as discussed can be seen from usage.c\nis due to our invoking of abort() there.\n\nThis RFC series aims to rectify that, first we back out of test-tool\nspecific behavior for BUG() in 1/3, and in 3/3 define abort() to be a\nwrapper macro like what we do for exit() already.\n\nI'm not sure about the direction of emitting such a \"signal\" event,\nshould we just \"fake it\" and emit an \"exit\" event? Probably not, but\nI'm not confident in that. For one is the exit code (as in what a\nshell would get from $?) when we're killed with SIGABRT guaranteed to\nbe portable?\n\nBut hopefully this gets the ball rolling on a good fix for this by\nstarting a discussion about what we should be doing here.\n\nOne alternate way of dealing with this would be to first split up the\n\"error\" event so that we don't emit \"error\" for all of BUG(), die(),\nerror() and warning(), and to instead split those up into \"BUG\",\n\"die\", \"error\", \"warning\".\n\nOnce we did that we could pretty much declare that the current\nbehavior before this series is a feature, and that anyone parsing\ntrace2 output needs to deal with the stream potentially ending in a\n\"BUG\" event.\n\nAny such event parser will need to deal with at least:\n\n    \"start\" -> [\"exit\" | \"signal\"] (with this series)\n\nSo perhaps it's OK to simply say that consumers should expect?\n\n   # Or \"error\" now, but then you can't distinguish \"BUG()\" from the\n   # rest.\n    \"start\" -> [\"exit\" | \"BUG\"]\n\nI think I like that less, but it's worth pointing out as an\nalternative. The implementation would certainly be smaller than this\nproposed RFC.\n\nÆvar Arnfjörð Bjarmason (3):\n  test-tool: don't fake up BUG() exits as code 99\n  refs API: rename \"abort\" callback to avoid macro clash\n  trace2: emit \"signal\" events after calling BUG()\n\n Documentation/technical/api-trace2.txt | 13 +++++++++----\n git-compat-util.h                      |  9 +++++++++\n refs/debug.c                           |  4 ++--\n refs/files-backend.c                   |  4 ++--\n refs/iterator.c                        |  8 ++++----\n refs/packed-backend.c                  |  2 +-\n refs/ref-cache.c                       |  2 +-\n refs/refs-internal.h                   |  2 +-\n t/helper/test-tool.c                   |  1 -\n t/t0210-trace2-normal.sh               |  5 ++---\n t/t1406-submodule-ref-store.sh         | 10 +++++-----\n t/test-lib-functions.sh                | 13 +++++++++++++\n trace2.c                               | 19 +++++++++++++++++++\n trace2.h                               |  8 ++++++++\n trace2/tr2_tgt.h                       |  3 +++\n trace2/tr2_tgt_event.c                 | 11 +++++++++--\n trace2/tr2_tgt_normal.c                | 11 +++++++++--\n trace2/tr2_tgt_perf.c                  | 11 +++++++++--\n usage.c                                |  5 -----\n 19 files changed, 106 insertions(+), 35 deletions(-)\n\n-- \n2.36.1.1046.g586767a6996\n\n"},{"id":"456143","messageId":"xmqq8rqp3x9z.fsf@gitster.g","threadId":"57924","inReplyTo":"RFC-cover-0.3-00000000000-20220525T234908Z-avarab@gmail.com","subject":"Re: [RFC PATCH 0/3] trace2: log \"signal\" end events if we invoke BUG()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-26T01:20:08Z","receivedAt":"2022-05-26T01:20:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> (depending on the TZ) that he'd encountered trace2 event streams\n> without an \"exit\" event, which as discussed can be seen from usage.c\n> is due to our invoking of abort() there.\n\nJosh, is this related to the \"in rare cases\" thing we discussed on\nthe \"run-command: don't spam trace2_child_exit()\" thread?\n\nhttps://lore.kernel.org/git/4616d09ffa632bd2c9e308a713c4bdf2a1328c3c.1651179450.git.steadmon@google.com/\n\n"},{"id":"456145","messageId":"Yo7uIG6JOx2lHOqE@debian.me","threadId":"57924","inReplyTo":"RFC-patch-3.3-b099a3a4a96-20220525T234908Z-avarab@gmail.com","subject":"Re: [RFC PATCH 3/3] trace2: emit \"signal\" events after calling BUG()","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2022-05-26T03:04:00Z","receivedAt":"2022-05-26T03:04:12Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On Thu, May 26, 2022 at 02:30:44AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>  `\"exit\"`::\n> -\tThis event is emitted when git calls `exit()`.\n> +\tThis event is emitted when git calls `exit()`. This event will\n> +\tbe produced for all regular ending of the git process, but it\n> +\tmight also exit via a \"signal\".\n>  +\n\nThe second 'this' can be elided, thus says \"This event is emitted ...\nand will be produced ...\".\n\n>  ------------\n>  {\n> @@ -435,7 +437,7 @@ only present on the \"start\" and \"atexit\" events.\n>  `\"atexit\"`::\n>  \tThis event is emitted by the Trace2 `atexit` routine during\n>  \tfinal shutdown.  It should be the last event emitted by the\n> -\tprocess.\n> +\tprocess, unless it was aborted (see \"signal\").\n>  +\n\nLooks OK.\n\n> @@ -452,8 +454,11 @@ completed.)\n>  \n>  `\"signal\"`::\n>  \tThis event is emitted when the program is terminated by a user\n> -\tsignal.  Depending on the platform, the signal event may\n> -\tprevent the \"atexit\" event from being generated.\n> +\tsignal, which includes git itself calling abort(3). Depending\n> +\ton the platform, the signal event may prevent the \"exit\"\n> +\tand/or \"atexit\" events from being generated. E.g. if BUG() was\n> +\tinvoked we'll emit an \"error\" event followed by a \"signal\"\n> +\tevent, and nothing else.\n>  +\n\nSo in case of BUG() trigger, there may not be exit event due to user\nsignal right? I'm expecting system (not user) signal in that case.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"456419","messageId":"YpZXj/VzMTNPSA1H@google.com","threadId":"57924","inReplyTo":"xmqq8rqp3x9z.fsf@gitster.g","subject":"Re: [RFC PATCH 0/3] trace2: log \"signal\" end events if we invoke BUG()","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-05-31T17:59:43Z","receivedAt":"2022-05-31T18:00:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.05.25 18:20, Junio C Hamano wrote:\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> \n> > (depending on the TZ) that he'd encountered trace2 event streams\n> > without an \"exit\" event, which as discussed can be seen from usage.c\n> > is due to our invoking of abort() there.\n> \n> Josh, is this related to the \"in rare cases\" thing we discussed on\n> the \"run-command: don't spam trace2_child_exit()\" thread?\n> \n> https://lore.kernel.org/git/4616d09ffa632bd2c9e308a713c4bdf2a1328c3c.1651179450.git.steadmon@google.com/\n> \n\nNo, this is a much more frequent issue than the repeated \"child_exit\"\nevents from that thread. Any time Git exits without calling our exit()\nwrapper, we don't log any \"exit\" events (and presumably skip \"atexit\"\nas well).\n"},{"id":"456420","messageId":"YpZbfM+DvMoUWDSH@google.com","threadId":"57924","inReplyTo":"RFC-patch-3.3-b099a3a4a96-20220525T234908Z-avarab@gmail.com","subject":"Re: [RFC PATCH 3/3] trace2: emit \"signal\" events after calling BUG()","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-05-31T18:16:28Z","receivedAt":"2022-05-31T18:16:39Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.05.26 02:30, Ævar Arnfjörð Bjarmason wrote:\n> Add a wrapper for the abort(3) function to correspond to the existing\n> wrapper for exit(3). See ee4512ed481 (trace2: create new combined\n> trace facility, 2019-02-22) for the introduction of the exit(3)\n> wrapper. See also the more recent 368b5843158 (common-main.c: call\n> exit(), don't return, 2021-12-07) discussing how the exit() wrapper\n> macro it and the trace2 machinery interact.\n> \n> As reported (off the ML) by Josh Steadmon we have \"start\" events that\n> are not followed by corresponding \"exit\" events. I first considered\n> logging an \"exit\" here, but that would be incorrect, we didn't call\n> exit, on abort() we do the equivalent of:\n> \n> \tkill(getpid(), SIGABRT);\n> \n> So let's instead update the documentation to note that those\n> interested in seeing a marker for process exit need to be looking for\n> either an \"exit\" or a \"signal\" event.\n> \n> We then need to add a *_fl() function similar to the existing\n> \"tr2main_signal_handler()\" function. Until now we'd only been emitting\n> these \"signal\" events from our own signal handlers.\n> \n> Reported-by: Josh Steadmon <steadmon@google.com>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  Documentation/technical/api-trace2.txt | 13 +++++++++----\n>  git-compat-util.h                      |  9 +++++++++\n>  t/t0210-trace2-normal.sh               |  1 +\n>  trace2.c                               | 19 +++++++++++++++++++\n>  trace2.h                               |  8 ++++++++\n>  trace2/tr2_tgt.h                       |  3 +++\n>  trace2/tr2_tgt_event.c                 | 11 +++++++++--\n>  trace2/tr2_tgt_normal.c                | 11 +++++++++--\n>  trace2/tr2_tgt_perf.c                  | 11 +++++++++--\n>  9 files changed, 76 insertions(+), 10 deletions(-)\n> \n> diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt\n> index f4a8a690878..55ecfd8c30c 100644\n> --- a/Documentation/technical/api-trace2.txt\n> +++ b/Documentation/technical/api-trace2.txt\n> @@ -421,7 +421,9 @@ only present on the \"start\" and \"atexit\" events.\n>  ------------\n>  \n>  `\"exit\"`::\n> -\tThis event is emitted when git calls `exit()`.\n> +\tThis event is emitted when git calls `exit()`. This event will\n> +\tbe produced for all regular ending of the git process, but it\n> +\tmight also exit via a \"signal\".\n>  +\n>  ------------\n>  {\n> @@ -435,7 +437,7 @@ only present on the \"start\" and \"atexit\" events.\n>  `\"atexit\"`::\n>  \tThis event is emitted by the Trace2 `atexit` routine during\n>  \tfinal shutdown.  It should be the last event emitted by the\n> -\tprocess.\n> +\tprocess, unless it was aborted (see \"signal\").\n>  +\n>  (The elapsed time reported here is greater than the time reported in\n>  the \"exit\" event because it runs after all other atexit tasks have\n> @@ -452,8 +454,11 @@ completed.)\n>  \n>  `\"signal\"`::\n>  \tThis event is emitted when the program is terminated by a user\n> -\tsignal.  Depending on the platform, the signal event may\n> -\tprevent the \"atexit\" event from being generated.\n> +\tsignal, which includes git itself calling abort(3). Depending\n> +\ton the platform, the signal event may prevent the \"exit\"\n> +\tand/or \"atexit\" events from being generated. E.g. if BUG() was\n> +\tinvoked we'll emit an \"error\" event followed by a \"signal\"\n> +\tevent, and nothing else.\n>  +\n>  ------------\n>  {\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 58fd813bd01..ea3177b2771 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1403,6 +1403,15 @@ int cmd_main(int, const char **);\n>  int trace2_cmd_exit_fl(const char *file, int line, int code);\n>  #define exit(code) exit(trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n>  \n> +/*\n> + * Intercept calls to abort() to log that we aborted.\n> + */\n> +void trace2_cmd_signal_fl(const char *file, int line, int signo);\n> +#define abort() do { \\\n> +\ttrace2_cmd_signal_fl(__FILE__, __LINE__, SIGABRT); \\\n> +\tabort(); \\\n> +} while (0)\n> +\n>  /*\n>   * You can mark a stack variable with UNLEAK(var) to avoid it being\n>   * reported as a leak by tools like LSAN or valgrind. The argument\n> diff --git a/t/t0210-trace2-normal.sh b/t/t0210-trace2-normal.sh\n> index 910a6af8058..25fffdba80e 100755\n> --- a/t/t0210-trace2-normal.sh\n> +++ b/t/t0210-trace2-normal.sh\n> @@ -162,6 +162,7 @@ test_expect_success 'BUG messages are written to trace2' '\n>  \t\tstart _EXE_ trace2 007bug\n>  \t\tcmd_name trace2 (trace2)\n>  \t\terror the bug message\n> +\t\tsignal elapsed:_TIME_ code:6\n>  \tEOF\n>  \ttest_cmp expect actual\n>  '\n> diff --git a/trace2.c b/trace2.c\n> index e01cf77f1a8..250b2424bfa 100644\n> --- a/trace2.c\n> +++ b/trace2.c\n> @@ -230,6 +230,25 @@ int trace2_cmd_exit_fl(const char *file, int line, int code)\n>  \treturn code;\n>  }\n>  \n> +void trace2_cmd_signal_fl(const char *file, int line, int signo)\n> +{\n> +\tstruct tr2_tgt *tgt_j;\n> +\tint j;\n> +\tuint64_t us_now;\n> +\tuint64_t us_elapsed_absolute;\n> +\n> +\tif (!trace2_enabled)\n> +\t\treturn;\n> +\n> +\tus_now = getnanotime() / 1000;\n> +\tus_elapsed_absolute = tr2tls_absolute_elapsed(us_now);\n> +\n> +\tfor_each_wanted_builtin (j, tgt_j)\n> +\t\tif (tgt_j->pfn_signal_fl)\n> +\t\t\ttgt_j->pfn_signal_fl(file, line, us_elapsed_absolute,\n> +\t\t\t\t\t     signo);\n> +}\n> +\n>  void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,\n>  \t\t\t    va_list ap)\n>  {\n> diff --git a/trace2.h b/trace2.h\n> index 1b109f57d0a..4f456f2fc62 100644\n> --- a/trace2.h\n> +++ b/trace2.h\n> @@ -112,6 +112,14 @@ int trace2_cmd_exit_fl(const char *file, int line, int code);\n>  \n>  #define trace2_cmd_exit(code) (trace2_cmd_exit_fl(__FILE__, __LINE__, (code)))\n>  \n> +/*\n> + * Emit a 'signal' event. Used to log that we called abort() e.g. via\n> + * the BUG() function.\n> + */\n> +void trace2_cmd_signal_fl(const char *file, int line, int signo);\n> +\n> +#define trace2_cmd_signal(signo) (trace2_cmd_signal_fl(__FILE__, __LINE__, (signo)))\n> +\n>  /*\n>   * Emit an 'error' event.\n>   *\n> diff --git a/trace2/tr2_tgt.h b/trace2/tr2_tgt.h\n> index 65f94e15748..fbc33650c5d 100644\n> --- a/trace2/tr2_tgt.h\n> +++ b/trace2/tr2_tgt.h\n> @@ -20,6 +20,8 @@ typedef void(tr2_tgt_evt_start_fl_t)(const char *file, int line,\n>  typedef void(tr2_tgt_evt_exit_fl_t)(const char *file, int line,\n>  \t\t\t\t    uint64_t us_elapsed_absolute, int code);\n>  typedef void(tr2_tgt_evt_signal_t)(uint64_t us_elapsed_absolute, int signo);\n> +typedef void(tr2_tgt_evt_signal_fl_t)(const char *file, int line,\n> +\t\t\t\t      uint64_t us_elapsed_absolute, int signo);\n>  typedef void(tr2_tgt_evt_atexit_t)(uint64_t us_elapsed_absolute, int code);\n>  \n>  typedef void(tr2_tgt_evt_error_va_fl_t)(const char *file, int line,\n> @@ -111,6 +113,7 @@ struct tr2_tgt {\n>  \ttr2_tgt_evt_start_fl_t                  *pfn_start_fl;\n>  \ttr2_tgt_evt_exit_fl_t                   *pfn_exit_fl;\n>  \ttr2_tgt_evt_signal_t                    *pfn_signal;\n> +\ttr2_tgt_evt_signal_fl_t                 *pfn_signal_fl;\n>  \ttr2_tgt_evt_atexit_t                    *pfn_atexit;\n>  \ttr2_tgt_evt_error_va_fl_t               *pfn_error_va_fl;\n>  \ttr2_tgt_evt_command_path_fl_t           *pfn_command_path_fl;\n> diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\n> index c5c8cfbbaa0..df947378a51 100644\n> --- a/trace2/tr2_tgt_event.c\n> +++ b/trace2/tr2_tgt_event.c\n> @@ -177,14 +177,15 @@ static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n>  \tjw_release(&jw);\n>  }\n>  \n> -static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n> +static void fn_signal_fl(const char *file, int line,\n> +\t\t\t uint64_t us_elapsed_absolute, int signo)\n>  {\n>  \tconst char *event_name = \"signal\";\n>  \tstruct json_writer jw = JSON_WRITER_INIT;\n>  \tdouble t_abs = (double)us_elapsed_absolute / 1000000.0;\n>  \n>  \tjw_object_begin(&jw, 0);\n> -\tevent_fmt_prepare(event_name, __FILE__, __LINE__, NULL, &jw);\n> +\tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n>  \tjw_object_double(&jw, \"t_abs\", 6, t_abs);\n>  \tjw_object_intmax(&jw, \"signo\", signo);\n>  \tjw_end(&jw);\n> @@ -193,6 +194,11 @@ static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n>  \tjw_release(&jw);\n>  }\n>  \n> +static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n> +{\n> +\tfn_signal_fl(__FILE__, __LINE__, us_elapsed_absolute, signo);\n> +}\n> +\n\nI believe the only caller of this would be\ntrace2.c:tr2main_signal_handler(), so could we just switch that one to\nuse pfn_signal_fl and get rid of pfn_signal altogether?\n\n\n>  static void fn_atexit(uint64_t us_elapsed_absolute, int code)\n>  {\n>  \tconst char *event_name = \"atexit\";\n> @@ -624,6 +630,7 @@ struct tr2_tgt tr2_tgt_event = {\n>  \t.pfn_start_fl = fn_start_fl,\n>  \t.pfn_exit_fl = fn_exit_fl,\n>  \t.pfn_signal = fn_signal,\n> +\t.pfn_signal_fl = fn_signal_fl,\n>  \t.pfn_atexit = fn_atexit,\n>  \t.pfn_error_va_fl = fn_error_va_fl,\n>  \t.pfn_command_path_fl = fn_command_path_fl,\n> diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\n> index c42fbade7f0..8e2bc5027fe 100644\n> --- a/trace2/tr2_tgt_normal.c\n> +++ b/trace2/tr2_tgt_normal.c\n> @@ -105,17 +105,23 @@ static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n>  \tstrbuf_release(&buf_payload);\n>  }\n>  \n> -static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n> +static void fn_signal_fl(const char *file, int line,\n> +\t\t\t uint64_t us_elapsed_absolute, int signo)\n>  {\n>  \tstruct strbuf buf_payload = STRBUF_INIT;\n>  \tdouble elapsed = (double)us_elapsed_absolute / 1000000.0;\n>  \n>  \tstrbuf_addf(&buf_payload, \"signal elapsed:%.6f code:%d\", elapsed,\n>  \t\t    signo);\n> -\tnormal_io_write_fl(__FILE__, __LINE__, &buf_payload);\n> +\tnormal_io_write_fl(file, line, &buf_payload);\n>  \tstrbuf_release(&buf_payload);\n>  }\n>  \n> +static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n> +{\n> +\tfn_signal_fl(__FILE__, __LINE__, us_elapsed_absolute, signo);\n> +}\n> +\n>  static void fn_atexit(uint64_t us_elapsed_absolute, int code)\n>  {\n>  \tstruct strbuf buf_payload = STRBUF_INIT;\n> @@ -336,6 +342,7 @@ struct tr2_tgt tr2_tgt_normal = {\n>  \t.pfn_start_fl = fn_start_fl,\n>  \t.pfn_exit_fl = fn_exit_fl,\n>  \t.pfn_signal = fn_signal,\n> +\t.pfn_signal_fl = fn_signal_fl,\n>  \t.pfn_atexit = fn_atexit,\n>  \t.pfn_error_va_fl = fn_error_va_fl,\n>  \t.pfn_command_path_fl = fn_command_path_fl,\n> diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\n> index a1eff8bea31..4ce18d3f12e 100644\n> --- a/trace2/tr2_tgt_perf.c\n> +++ b/trace2/tr2_tgt_perf.c\n> @@ -193,18 +193,24 @@ static void fn_exit_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n>  \tstrbuf_release(&buf_payload);\n>  }\n>  \n> -static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n> +static void fn_signal_fl(const char *file, int line,\n> +\t\t\t uint64_t us_elapsed_absolute, int signo)\n>  {\n>  \tconst char *event_name = \"signal\";\n>  \tstruct strbuf buf_payload = STRBUF_INIT;\n>  \n>  \tstrbuf_addf(&buf_payload, \"signo:%d\", signo);\n>  \n> -\tperf_io_write_fl(__FILE__, __LINE__, event_name, NULL,\n> +\tperf_io_write_fl(file, line, event_name, NULL,\n>  \t\t\t &us_elapsed_absolute, NULL, NULL, &buf_payload);\n>  \tstrbuf_release(&buf_payload);\n>  }\n>  \n> +static void fn_signal(uint64_t us_elapsed_absolute, int signo)\n> +{\n> +\tfn_signal_fl(__FILE__, __LINE__, us_elapsed_absolute, signo);\n> +}\n> +\n>  static void fn_atexit(uint64_t us_elapsed_absolute, int code)\n>  {\n>  \tconst char *event_name = \"atexit\";\n> @@ -560,6 +566,7 @@ struct tr2_tgt tr2_tgt_perf = {\n>  \t.pfn_start_fl = fn_start_fl,\n>  \t.pfn_exit_fl = fn_exit_fl,\n>  \t.pfn_signal = fn_signal,\n> +\t.pfn_signal_fl = fn_signal_fl,\n>  \t.pfn_atexit = fn_atexit,\n>  \t.pfn_error_va_fl = fn_error_va_fl,\n>  \t.pfn_command_path_fl = fn_command_path_fl,\n> -- \n> 2.36.1.1046.g586767a6996\n> \n"},{"id":"456627","messageId":"xmqqv8theehf.fsf@gitster.g","threadId":"57924","inReplyTo":"RFC-patch-1.3-78431bdc8f0-20220525T234908Z-avarab@gmail.com","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-03T19:25:32Z","receivedAt":"2022-06-03T19:25:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change the BUG() function invoked be \"test-tool\" to be the \"real\" one,\n> instead of one that avoids producing core files. In\n> a86303cb5d5 (test-tool: help verifying BUG() code paths, 2018-05-02)\n> to test the (then recently added) BUG() function we faked up the\n> abort() in favor of an exit with code 99.\n>\n> However, in doing so we've been fooling ourselves when it comes to\n> what trace2 events we log. The events tested for in\n> 0a9dde4a04c (usage: trace2 BUG() invocations, 2021-02-05) are not the\n> real ones, but those that we emit only from the \"test-tool\".\n\nI can fully agree with the above reasoning, i.e. let's test what we\ndo use in production, instead of something nobody uses for real, if\nwe were adding a test for BUG() in vacuum, but why did we have to\n\"fake\" it in the first place?\n\n> Let's just stop faking it, and call abort(). As a86303cb5d5 notes this\n> will produce core files on some OS's, but as the default behavior for\n> that is to dump them in the current directory they'll be placed in the\n> trash directory that we'll shortly me \"rm -rf\"-ing.\n\nAre we sure that the reason no longer applies?  How do we know?  We\nwould want to explain that to future developers in the proposed log\nmessage, I would think.\n\n> +\telif test_match_signal 6 $exit_code && list_contains \"$_test_ok\" sigabrt\n> +\tthen\n> +\t\treturn 0\n>  \telif test_match_signal 13 $exit_code && list_contains \"$_test_ok\" sigpipe\n\nNot a new problem, but these numberings are probably not very\nportable.  I am willing to take this as-is and let people on\nminority platforms complain ;-)\n"},{"id":"456633","messageId":"xmqqczfpe9ua.fsf@gitster.g","threadId":"57924","inReplyTo":"xmqqv8theehf.fsf@gitster.g","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-03T21:05:49Z","receivedAt":"2022-06-03T21:05:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> However, in doing so we've been fooling ourselves when it comes to\n>> what trace2 events we log. The events tested for in\n>> 0a9dde4a04c (usage: trace2 BUG() invocations, 2021-02-05) are not the\n>> real ones, but those that we emit only from the \"test-tool\".\n>\n> I can fully agree with the above reasoning, i.e. let's test what we\n> do use in production, instead of something nobody uses for real, if\n> we were adding a test for BUG() in vacuum, but why did we have to\n> \"fake\" it in the first place?\n> ...\n> Are we sure that the reason no longer applies?  How do we know?  We\n> would want to explain that to future developers in the proposed log\n> message, I would think.\n\nWe can flip it the other way around.  \n\nI do not think I ever saw anybody asked anybody on this list who got\na BUG() message to use the coredump to do something useful.  Don't\nmodern distros ship with \"ulimit -c 0\" these days?\n\nIt might be possible that a better direction is to introduce\nGIT_ABORT_ON_BUG environment or core.abortOnBUG configuration that\nchooses between abort() and exit(99), or something like that, and\nthen we switch to use the latter by default over time?\n"},{"id":"456644","messageId":"220604.867d5xxs90.gmgdl@evledraar.gmail.com","threadId":"57924","inReplyTo":"xmqqv8theehf.fsf@gitster.g","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T23:03:53Z","receivedAt":"2022-06-03T23:05:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 03 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Change the BUG() function invoked be \"test-tool\" to be the \"real\" one,\n>> instead of one that avoids producing core files. In\n>> a86303cb5d5 (test-tool: help verifying BUG() code paths, 2018-05-02)\n>> to test the (then recently added) BUG() function we faked up the\n>> abort() in favor of an exit with code 99.\n>>\n>> However, in doing so we've been fooling ourselves when it comes to\n>> what trace2 events we log. The events tested for in\n>> 0a9dde4a04c (usage: trace2 BUG() invocations, 2021-02-05) are not the\n>> real ones, but those that we emit only from the \"test-tool\".\n>\n> I can fully agree with the above reasoning, i.e. let's test what we\n> do use in production, instead of something nobody uses for real, if\n> we were adding a test for BUG() in vacuum, but why did we have to\n> \"fake\" it in the first place?\n\nPer a86303cb5d5 (test-tool: help verifying BUG() code paths, 2018-05-02)\nI think it was to get rid of coredump clutter outside of trash\ndirectories (which would require some non-standard config), which\ndoesn't seem worth it.\n\n>> Let's just stop faking it, and call abort(). As a86303cb5d5 notes this\n>> will produce core files on some OS's, but as the default behavior for\n>> that is to dump them in the current directory they'll be placed in the\n>> trash directory that we'll shortly me \"rm -rf\"-ing.\n>\n> Are we sure that the reason no longer applies?  How do we know?  We\n> would want to explain that to future developers in the proposed log\n> message, I would think.\n\nI was trying to get the above across, i.e. that the reasoning still\napplies, but that the fakery isn't worth it.\n\n>> +\telif test_match_signal 6 $exit_code && list_contains \"$_test_ok\" sigabrt\n>> +\tthen\n>> +\t\treturn 0\n>>  \telif test_match_signal 13 $exit_code && list_contains \"$_test_ok\" sigpipe\n>\n> Not a new problem, but these numberings are probably not very\n> portable.  I am willing to take this as-is and let people on\n> minority platforms complain ;-)\n\nI'll test it better for portability before a non-RFC.\n"},{"id":"456646","messageId":"220604.8635glxs3g.gmgdl@evledraar.gmail.com","threadId":"57924","inReplyTo":"xmqqczfpe9ua.fsf@gitster.g","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-03T23:05:34Z","receivedAt":"2022-06-03T23:08:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 03 2022, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>> However, in doing so we've been fooling ourselves when it comes to\n>>> what trace2 events we log. The events tested for in\n>>> 0a9dde4a04c (usage: trace2 BUG() invocations, 2021-02-05) are not the\n>>> real ones, but those that we emit only from the \"test-tool\".\n>>\n>> I can fully agree with the above reasoning, i.e. let's test what we\n>> do use in production, instead of something nobody uses for real, if\n>> we were adding a test for BUG() in vacuum, but why did we have to\n>> \"fake\" it in the first place?\n>> ...\n>> Are we sure that the reason no longer applies?  How do we know?  We\n>> would want to explain that to future developers in the proposed log\n>> message, I would think.\n>\n> We can flip it the other way around.  \n>\n> I do not think I ever saw anybody asked anybody on this list who got\n> a BUG() message to use the coredump to do something useful.  Don't\n> modern distros ship with \"ulimit -c 0\" these days?\n\nI think that part of it was just a side-effect of SIGABRT.\n\n> It might be possible that a better direction is to introduce\n> GIT_ABORT_ON_BUG environment or core.abortOnBUG configuration that\n> chooses between abort() and exit(99), or something like that, and\n> then we switch to use the latter by default over time?\n\nI think the reason for abort() was what's covered in raise(3), i.e. it's\na one-stop-shop to getting \"stop it\" behavior both under threading and\nnon-threading, which as e.g. exit(3) discusses wouldn't be thread-safe\nwith it.\n\nBut perhaps that was all premature worrying, we're mostly running\nnon-threaded, and to the extent that we ever BUG() running into exit(3)\nthreading issues is probably the least of our worries.\n\nSo perhaps we should drop abort() entirely, I don't know. These proposed\npatches tried not to do that, but just to log things when we did so.\n"},{"id":"456881","messageId":"YqD100DRVJoZCC+x@coredump.intra.peff.net","threadId":"57924","inReplyTo":"xmqqczfpe9ua.fsf@gitster.g","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-08T19:17:39Z","receivedAt":"2022-06-08T19:17:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 03, 2022 at 02:05:49PM -0700, Junio C Hamano wrote:\n\n> > Are we sure that the reason no longer applies?  How do we know?  We\n> > would want to explain that to future developers in the proposed log\n> > message, I would think.\n> \n> We can flip it the other way around.  \n> \n> I do not think I ever saw anybody asked anybody on this list who got\n> a BUG() message to use the coredump to do something useful.  Don't\n> modern distros ship with \"ulimit -c 0\" these days?\n\nCertainly I have found the coredumps useful, though I would expect most\nGit developers to be able to run under a debugger and stop at BUG()\nanyway. So we could probably live without that, but...\n\n> It might be possible that a better direction is to introduce\n> GIT_ABORT_ON_BUG environment or core.abortOnBUG configuration that\n> chooses between abort() and exit(99), or something like that, and\n> then we switch to use the latter by default over time?\n\nIt really should continue to die with a signal (or an exit code that\npretends to be one) to continue triggering even under test_must_fail().\n\nIMHO the whole \"let's trigger BUG() intentionally via test-tool\" stuff\nin t1406 is misguided. It's literally introducing broken code that is\nnot run in the normal Git binary in order to make sure that it's broken.\nIf that were the only spot, I'd suggest just getting rid of it entirely.\nBut the code in t0210 that checks the handling of trace2 and BUG() is\nprobably worth keeping.\n\nI do agree that an environment variable would be a better selector than\n\"this code is linked against test-tool\". I thought so even back in:\n\n  https://lore.kernel.org/git/20180507090109.GA367@sigill.intra.peff.net/\n\n:) That message also covers the flip-side case discussed earlier in this\nthread: why calling abort() unconditionally in the test suite can be a\npain.\n\n-Peff\n"},{"id":"456898","messageId":"xmqq1qvyrhpc.fsf@gitster.g","threadId":"57924","inReplyTo":"YqD100DRVJoZCC+x@coredump.intra.peff.net","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-08T21:03:27Z","receivedAt":"2022-06-08T21:03:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I do agree that an environment variable would be a better selector than\n> \"this code is linked against test-tool\". I thought so even back in:\n>\n>   https://lore.kernel.org/git/20180507090109.GA367@sigill.intra.peff.net/\n>\n> :) That message also covers the flip-side case discussed earlier in this\n> thread: why calling abort() unconditionally in the test suite can be a\n> pain.\n\nNice.  Thanks.\n"},{"id":"456926","messageId":"220609.86czfitfgz.gmgdl@evledraar.gmail.com","threadId":"57924","inReplyTo":"YqD100DRVJoZCC+x@coredump.intra.peff.net","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-09T08:09:25Z","receivedAt":"2022-06-09T08:21:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 08 2022, Jeff King wrote:\n\n> On Fri, Jun 03, 2022 at 02:05:49PM -0700, Junio C Hamano wrote:\n>\n>> > Are we sure that the reason no longer applies?  How do we know?  We\n>> > would want to explain that to future developers in the proposed log\n>> > message, I would think.\n>> \n>> We can flip it the other way around.  \n>> \n>> I do not think I ever saw anybody asked anybody on this list who got\n>> a BUG() message to use the coredump to do something useful.  Don't\n>> modern distros ship with \"ulimit -c 0\" these days?\n>\n> Certainly I have found the coredumps useful, though I would expect most\n> Git developers to be able to run under a debugger and stop at BUG()\n> anyway. So we could probably live without that, but...\n>\n>> It might be possible that a better direction is to introduce\n>> GIT_ABORT_ON_BUG environment or core.abortOnBUG configuration that\n>> chooses between abort() and exit(99), or something like that, and\n>> then we switch to use the latter by default over time?\n>\n> It really should continue to die with a signal (or an exit code that\n> pretends to be one) to continue triggering even under test_must_fail().\n>\n> IMHO the whole \"let's trigger BUG() intentionally via test-tool\" stuff\n> in t1406 is misguided. It's literally introducing broken code that is\n> not run in the normal Git binary in order to make sure that it's broken.\n\nI haven't looked at that ref code in particular, but in general adding\ncoverage for the case we actually expect to defend against with BUG()\nvia a test-tool is something I think we could use in more cases.\n\nE.g. parse-options.c will BUG() out on various bad options structs,\nthere it makes sense to feed those bad structs in to make sure our\nassertion code is doing what we still expect, but we don't have those\ntests.\n\n> If that were the only spot, I'd suggest just getting rid of it entirely.\n> But the code in t0210 that checks the handling of trace2 and BUG() is\n> probably worth keeping.\n\nYes, we definitely want to test what *actually* happens as far as \n\n> I do agree that an environment variable would be a better selector than\n> \"this code is linked against test-tool\". I thought so even back in:\n>\n>   https://lore.kernel.org/git/20180507090109.GA367@sigill.intra.peff.net/\n>\n> :) That message also covers the flip-side case discussed earlier in this\n> thread: why calling abort() unconditionally in the test suite can be a\n> pain.\n\nI didn't know about that Jenkins case, but in any case I think we can\nprobably stop worrying about this given that we haven't had complaints\nabout ac14de13b22 (t4058: explore duplicate tree entry handling in a bit\nmore detail, 2020-12-11) (although I've noticed in in \"dmesg\" before).\n\nI.e. since v2.31.0 running the test suite has produced at least 2\nsegfaults per run:\n\n    $ (sudo dmesg -c; ./t4058-diff-duplicates.sh) >/dev/null; sudo dmesg | grep segfault\n    [17687265.247252] git[11336]: segfault at 40 ip 0000000000700916 sp 00007ffc10d5dda0 error 4 in git[405000+33d000]\n    [17687265.297027] git[11397]: segfault at 40 ip 0000000000700916 sp 00007ffd7dfd5310 error 4 in git[405000+33d000]\n\nPerhaps those tests aren't valuable, but I think that shows that the\nGIT_BUG_ABORT approach you suggested should probably be tweaked to\ninstead be some test prereq like SEGFAULT_AND_ABORT_OK.\n\nOn the other hand those segfaults in t4058 should probably be converted\nto a BUG()...\n"},{"id":"456946","messageId":"YqIQghPvB/rbkos8@coredump.intra.peff.net","threadId":"57924","inReplyTo":"220609.86czfitfgz.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 1/3] test-tool: don't fake up BUG() exits as code 99","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-09T15:23:46Z","receivedAt":"2022-06-09T15:23:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2022 at 10:09:25AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > IMHO the whole \"let's trigger BUG() intentionally via test-tool\" stuff\n> > in t1406 is misguided. It's literally introducing broken code that is\n> > not run in the normal Git binary in order to make sure that it's broken.\n> \n> I haven't looked at that ref code in particular, but in general adding\n> coverage for the case we actually expect to defend against with BUG()\n> via a test-tool is something I think we could use in more cases.\n> \n> E.g. parse-options.c will BUG() out on various bad options structs,\n> there it makes sense to feed those bad structs in to make sure our\n> assertion code is doing what we still expect, but we don't have those\n> tests.\n\nThese seem like low-value tests to me. Once you've identified a case in\nparse-options.c that should BUG(), it's not very many lines of code to\ndetect it and call the function. But to then create a test case,\nincluding a harness that triggers the internal code in such a way that\nis designed never to happen in the production code, seems much more\nerror prone to me.\n\nI.e., it is very unlikely to me that such a test case will find an\nactual problem, and it carries a lot of scaffolding cost.\n\nMeanwhile the much more likely problem is that a BUG() can be triggered,\nbut we simply don't have test coverage of the right cases. Once you've\nidentified the case, the problem can be demonstrated (and fixed) in the\nproduction code.\n\n> I didn't know about that Jenkins case, but in any case I think we can\n> probably stop worrying about this given that we haven't had complaints\n> about ac14de13b22 (t4058: explore duplicate tree entry handling in a bit\n> more detail, 2020-12-11) (although I've noticed in in \"dmesg\" before).\n\nI didn't notice those ones. It's probable that the CI instance I was\nusing at GitHub changed under the hood. And GitHub is more likely than\nmost people to run git's test suite in a CI environment. :) So yeah, it\nmay not matter that much. I do still think dumping cores around the\nfilesystem (remembering that they do not always go into the trash\ndirectory) and polluting the kernel log are a bit unfriendly. If we can\navoid it easily, it might be worth doing.\n\n> On the other hand those segfaults in t4058 should probably be converted\n> to a BUG()...\n\nAgreed. Those stack-exhaustion ones (which were what originally alerted\nme to the Jenkins issue) are hard to catch.\n\n-Peff\n"}]}