From d22ff3e3f97dfa1b88e78b81d85d9bc66c6a74d1 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 13 Mar 2025 13:44:43 -0700 Subject: [PATCH 1/3] git-compat-util: introduce ASSERT() macro Create a ASSERT() macro which is similar to assert(), but will not be compiled out when NDEBUG is defined, and is thus safe to use even if its argument has side-effects. We will use this new macro in a subsequent commit to convert a few existing assert() invocations to ASSERT(). In particular, we'll convert the handful of invocations which cannot be proven to be free of side effects with a simple compiler/linker hack. Signed-off-by: Elijah Newren --- git-compat-util.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index e123288e8f1393..d7f3407128c062 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1460,6 +1460,8 @@ 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__) +/* ASSERT: like assert(), but won't be compiled out with NDEBUG */ +#define ASSERT(a) if (!(a)) BUG("Assertion `" #a "' failed.") __attribute__((format (printf, 3, 4))) void bug_fl(const char *file, int line, const char *fmt, ...); #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__) From 223d8c0ca2a9f78ae9e281773037cc5673f0637a Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 13 Mar 2025 13:33:07 -0700 Subject: [PATCH 2/3] ci: add build checking for side-effects in assert() calls It is a big no-no to have side-effects in an assertion, because if the assert() is compiled out, you don't get that side-effect, leading to the code behaving differently. That can be a large headache to debug. We have roughly 566 assert() calls in our codebase (my grep might have picked up things that aren't actually assert() calls, but most appeared to be). All but 9 of them can be determined by gcc to be free of side effects with a clever redefine of assert() provided by Bruno De Fraine (from https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects), who upon request has graciously placed his two-liner into the public domain without warranty of any kind. The current 9 assert() calls flagged by this clever redefinition of assert() appear to me to be free of side effects as well, but are too complicated for a compiler/linker to figure that since each assertion involves some kind of function call. Add a CI job which will find and report these possibly problematic assertions, and have the job suggest to the user that they replace these with ASSERT() calls. Example output from running: ``` ERROR: The compiler could not verify the following assert() calls are free of side-effects. Please replace with ASSERT() calls. /home/newren/floss/git/diffcore-rename.c:1409 assert(!dir_rename_count || strmap_empty(dir_rename_count)); /home/newren/floss/git/merge-ort.c:1645 assert(renames->deferred[side].trivial_merges_okay && !strset_contains(&renames->deferred[side].target_dirs, path)); /home/newren/floss/git/merge-ort.c:794 assert(omittable_hint == (!starts_with(type_short_descriptions[type], "CONFLICT") && !starts_with(type_short_descriptions[type], "ERROR")) || type == CONFLICT_DIR_RENAME_SUGGESTED); /home/newren/floss/git/merge-recursive.c:1200 assert(!merge_remote_util(commit)); /home/newren/floss/git/object-file.c:2709 assert(would_convert_to_git_filter_fd(istate, path)); /home/newren/floss/git/parallel-checkout.c:280 assert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca)); /home/newren/floss/git/scalar.c:244 assert(have_fsmonitor_support()); /home/newren/floss/git/scalar.c:254 assert(have_fsmonitor_support()); /home/newren/floss/git/sequencer.c:4968 assert(!(opts->signoff || opts->no_commit || opts->record_origin || should_edit(opts) || opts->committer_date_is_author_date || opts->ignore_date)); ``` Note that if there are possibly problematic assertions, not necessarily all of them will be shown in a single run, because the compiler errors may include something like "ld: ... more undefined references to `not_supposed_to_survive' follow" instead of listing each individually. But in such cases, once you clean up a few that are shown in your first run, subsequent runs will show (some of) the ones that remain, allowing you to iteratively remove them all. Helped-by: Bruno De Fraine Signed-off-by: Elijah Newren --- Makefile | 4 ++++ ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++ ci/run-static-analysis.sh | 2 ++ git-compat-util.h | 6 ++++++ 4 files changed, 30 insertions(+) create mode 100755 ci/check-unsafe-assertions.sh diff --git a/Makefile b/Makefile index 7315507381e146..57774912f18e5a 100644 --- a/Makefile +++ b/Makefile @@ -2261,6 +2261,10 @@ ifdef WITH_BREAKING_CHANGES BASIC_CFLAGS += -DWITH_BREAKING_CHANGES endif +ifdef CHECK_ASSERTION_SIDE_EFFECTS + BASIC_CFLAGS += -DCHECK_ASSERTION_SIDE_EFFECTS +endif + ifdef INCLUDE_LIBGIT_RS # Enable symbol hiding in contrib/libgit-sys/libgitpub.a without making # us rebuild the whole tree every time we run a Rust build. diff --git a/ci/check-unsafe-assertions.sh b/ci/check-unsafe-assertions.sh new file mode 100755 index 00000000000000..233bd9dfbc41c2 --- /dev/null +++ b/ci/check-unsafe-assertions.sh @@ -0,0 +1,18 @@ +#!/bin/sh + +make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error +if test $? != 0 +then + echo >&2 "ERROR: The compiler could not verify the following assert()" + echo >&2 " calls are free of side-effects. Please replace with" + echo >&2 " ASSERT() calls." + grep undefined.reference.to..not_supposed_to_survive compiler_error | + sed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' | + while read f l + do + printf "${f}:${l}\n " + awk -v start="$l" 'NR >= start { print; if (/\);/) exit }' $f + done + exit 1 +fi +rm compiler_output compiler_error diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh index 0d51e5ce0e7cb4..ae714e020ae782 100755 --- a/ci/run-static-analysis.sh +++ b/ci/run-static-analysis.sh @@ -31,4 +31,6 @@ exit 1 make check-pot +${0%/*}/check-unsafe-assertions.sh + save_good_tree diff --git a/git-compat-util.h b/git-compat-util.h index d7f3407128c062..5891efaeb18890 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1585,4 +1585,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr)) #endif /* !__GNUC__ */ +#ifdef CHECK_ASSERTION_SIDE_EFFECTS +#undef assert +extern int not_supposed_to_survive; +#define assert(expr) ((void)(not_supposed_to_survive || (expr))) +#endif /* CHECK_ASSERTION_SIDE_EFFECTS */ + #endif From 82b7344e96683efdac5dfc559a023d81ea0084d0 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 13 Mar 2025 13:58:13 -0700 Subject: [PATCH 3/3] treewide: replace assert() with ASSERT() in special cases When the compiler/linker cannot verify that an assert() invocation is free of side effects for us (e.g. because the assertion includes some kind of function call), replace the use of assert() with ASSERT(). Signed-off-by: Elijah Newren --- diffcore-rename.c | 2 +- merge-ort.c | 4 ++-- merge-recursive.c | 2 +- object-file.c | 2 +- parallel-checkout.c | 2 +- scalar.c | 4 ++-- sequencer.c | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/diffcore-rename.c b/diffcore-rename.c index 91b77993c7827f..624304f0416746 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -1406,7 +1406,7 @@ void diffcore_rename_extended(struct diff_options *options, trace2_region_enter("diff", "setup", options->repo); info.setup = 0; - assert(!dir_rename_count || strmap_empty(dir_rename_count)); + ASSERT(!dir_rename_count || strmap_empty(dir_rename_count)); want_copies = (detect_rename == DIFF_DETECT_COPY); if (dirs_removed && (break_idx || want_copies)) BUG("dirs_removed incompatible with break/copy detection"); diff --git a/merge-ort.c b/merge-ort.c index 46e78c3ffa68e6..d484f16cf2bf2c 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -791,7 +791,7 @@ static void path_msg(struct merge_options *opt, struct strbuf tmp = STRBUF_INIT; /* Sanity checks */ - assert(omittable_hint == + ASSERT(omittable_hint == (!starts_with(type_short_descriptions[type], "CONFLICT") && !starts_with(type_short_descriptions[type], "ERROR")) || type == CONFLICT_DIR_RENAME_SUGGESTED); @@ -1642,7 +1642,7 @@ static int handle_deferred_entries(struct merge_options *opt, ci = strmap_get(&opt->priv->paths, path); VERIFY_CI(ci); - assert(renames->deferred[side].trivial_merges_okay && + ASSERT(renames->deferred[side].trivial_merges_okay && !strset_contains(&renames->deferred[side].target_dirs, path)); resolve_trivial_directory_merge(ci, side); diff --git a/merge-recursive.c b/merge-recursive.c index 884ccf99a58d3b..4fbbece922c8cf 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1197,7 +1197,7 @@ static void print_commit(struct repository *repo, struct commit *commit) struct pretty_print_context ctx = {0}; ctx.date_mode.type = DATE_NORMAL; /* FIXME: Merge this with output_commit_title() */ - assert(!merge_remote_util(commit)); + ASSERT(!merge_remote_util(commit)); repo_format_commit_message(repo, commit, " %h: %m %s", &sb, &ctx); fprintf(stderr, "%s\n", sb.buf); strbuf_release(&sb); diff --git a/object-file.c b/object-file.c index 726e41a0475b43..4fb3cd9dcb9baf 100644 --- a/object-file.c +++ b/object-file.c @@ -2706,7 +2706,7 @@ static int index_stream_convert_blob(struct index_state *istate, struct strbuf sbuf = STRBUF_INIT; assert(path); - assert(would_convert_to_git_filter_fd(istate, path)); + ASSERT(would_convert_to_git_filter_fd(istate, path)); convert_to_git_filter_fd(istate, path, fd, &sbuf, get_conv_flags(flags)); diff --git a/parallel-checkout.c b/parallel-checkout.c index 7cc6b3052819ac..57c2dcaa8f60a1 100644 --- a/parallel-checkout.c +++ b/parallel-checkout.c @@ -277,7 +277,7 @@ static int write_pc_item_to_fd(struct parallel_checkout_item *pc_item, int fd, ssize_t wrote; /* Sanity check */ - assert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca)); + ASSERT(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca)); filter = get_stream_filter_ca(&pc_item->ca, &pc_item->ce->oid); if (filter) { diff --git a/scalar.c b/scalar.c index da42b4be0cc9b6..d359f08bb8e23e 100644 --- a/scalar.c +++ b/scalar.c @@ -241,7 +241,7 @@ static int add_or_remove_enlistment(int add) static int start_fsmonitor_daemon(void) { - assert(have_fsmonitor_support()); + ASSERT(have_fsmonitor_support()); if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) return run_git("fsmonitor--daemon", "start", NULL); @@ -251,7 +251,7 @@ static int start_fsmonitor_daemon(void) static int stop_fsmonitor_daemon(void) { - assert(have_fsmonitor_support()); + ASSERT(have_fsmonitor_support()); if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING) return run_git("fsmonitor--daemon", "stop", NULL); diff --git a/sequencer.c b/sequencer.c index ad0ab75c8d4dd7..c625a39111e11b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4965,7 +4965,7 @@ static int pick_commits(struct repository *r, ctx->reflog_message = sequencer_reflog_action(opts); if (opts->allow_ff) - assert(!(opts->signoff || opts->no_commit || + ASSERT(!(opts->signoff || opts->no_commit || opts->record_origin || should_edit(opts) || opts->committer_date_is_author_date || opts->ignore_date));