We have several hundred assert() invocations in our code base. Some have suggested that we should add a recommendation in our CodingGuidelines to avoid their use, because there is a risk that someone might include something with a side-effect in their assertion, which can lead to a very difficult to debug problem. However, CodingGuidelines are going to be less effective at preventing that foot-gun than a CI job which can warn of assertions that possibly have side-effects. So, let's add a CI job instead. While it is difficult to perfectly determine whether any expression has side effects, a simple compiler/linker hack can prove that all but 9 of our several hundred assert() calls are indeed free from them. While I believe the remaining 9 are also free of side effects, it's easier to just convert those 9 to a new macro (which will not be compiled out when NDEBUG is defined), and instruct any future assertion writers to likewise switch to that alternative macro if they have a slightly more involved assert() invocation. See https://github.com/newren/git/actions/runs/13845548634/job/38743076293#step:4:1938 for an example of it running in CI and reporting possibly problematic assertions (sample output also included in the commit message of the middle commit in this series if you don't have access to view the link; I'm not sure what the rules on that are). Changes since v1: * Tweaked commit message for patch 2 Changes since v2: * Rename BUT_IF_NOT() -> ASSERT(). Didn't have a strong opinion on the set of alternatives Junio gave, so went with Taylor's small preference. If anyone has a strong preference here, I can pick a different alternative. * Fixed shell style issues (indentation, multi-line pipes, multiple lines with stderr redirects) in patch 2 Elijah Newren (3): git-compat-util: introduce ASSERT() macro ci: add build checking for side-effects in assert() calls treewide: replace assert() with ASSERT() in special cases Makefile | 4 ++++ ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++ ci/run-static-analysis.sh | 2 ++ diffcore-rename.c | 2 +- git-compat-util.h | 8 ++++++++ merge-ort.c | 4 ++-- merge-recursive.c | 2 +- object-file.c | 2 +- parallel-checkout.c | 2 +- scalar.c | 4 ++-- sequencer.c | 2 +- 11 files changed, 41 insertions(+), 9 deletions(-) create mode 100755 ci/check-unsafe-assertions.sh base-commit: 4b68faf6b93311254efad80e554780e372deb42f Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1881%2Fnewren%2Fassertion-side-effects-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1881 Range-diff vs v2: 1: 109060ccb86 ! 1: d22ff3e3f97 git-compat-util: introduce BUG_IF_NOT() macro @@ Metadata Author: Elijah Newren <newren@xxxxxxxxx> ## Commit message ## - git-compat-util: introduce BUG_IF_NOT() macro + git-compat-util: introduce ASSERT() macro - Create a BUG_IF_NOT() macro which is similar to assert(), but will not be + 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 BUG_IF_NOT(). In particular, we'll + 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. @@ git-compat-util.h: 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__) -+#define BUG_IF_NOT(a) if (!(a)) BUG("Assertion `" #a "' failed.") ++/* 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__) 2: 58cb8f6a160 ! 2: 223d8c0ca2a ci: add build checking for side-effects in assert() calls @@ Commit message 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 BUG_IF_NOT() calls. + 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 - BUG_IF_NOT() calls. + 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 @@ ci/check-unsafe-assertions.sh (new) +make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error +if test $? != 0 +then -+ echo "ERROR: The compiler could not verify the following assert()" >&2 -+ echo " calls are free of side-effects. Please replace with" >&2 -+ echo " BUG_IF_NOT() calls." >&2 -+ 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 ++ 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 3: 20c763f2951 ! 3: 82b7344e966 treewide: replace assert() with BUG_IF_NOT() in special cases @@ Metadata Author: Elijah Newren <newren@xxxxxxxxx> ## Commit message ## - treewide: replace assert() with BUG_IF_NOT() in special cases + 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 BUG_IF_NOT(). + kind of function call), replace the use of assert() with ASSERT(). Signed-off-by: Elijah Newren <newren@xxxxxxxxx> @@ diffcore-rename.c: 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)); -+ BUG_IF_NOT(!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"); @@ merge-ort.c: static void path_msg(struct merge_options *opt, /* Sanity checks */ - assert(omittable_hint == -+ BUG_IF_NOT(omittable_hint == ++ ASSERT(omittable_hint == (!starts_with(type_short_descriptions[type], "CONFLICT") && !starts_with(type_short_descriptions[type], "ERROR")) || type == CONFLICT_DIR_RENAME_SUGGESTED); @@ merge-ort.c: static int handle_deferred_entries(struct merge_options *opt, VERIFY_CI(ci); - assert(renames->deferred[side].trivial_merges_okay && -+ BUG_IF_NOT(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); @@ merge-recursive.c: static void print_commit(struct repository *repo, struct comm ctx.date_mode.type = DATE_NORMAL; /* FIXME: Merge this with output_commit_title() */ - assert(!merge_remote_util(commit)); -+ BUG_IF_NOT(!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); @@ object-file.c: static int index_stream_convert_blob(struct index_state *istate, assert(path); - assert(would_convert_to_git_filter_fd(istate, path)); -+ BUG_IF_NOT(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)); @@ parallel-checkout.c: static int write_pc_item_to_fd(struct parallel_checkout_ite /* Sanity check */ - assert(is_eligible_for_parallel_checkout(pc_item->ce, &pc_item->ca)); -+ BUG_IF_NOT(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) { @@ scalar.c: static int add_or_remove_enlistment(int add) static int start_fsmonitor_daemon(void) { - assert(have_fsmonitor_support()); -+ BUG_IF_NOT(have_fsmonitor_support()); ++ ASSERT(have_fsmonitor_support()); if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) return run_git("fsmonitor--daemon", "start", NULL); @@ scalar.c: static int start_fsmonitor_daemon(void) static int stop_fsmonitor_daemon(void) { - assert(have_fsmonitor_support()); -+ BUG_IF_NOT(have_fsmonitor_support()); ++ ASSERT(have_fsmonitor_support()); if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING) return run_git("fsmonitor--daemon", "stop", NULL); @@ sequencer.c: static int pick_commits(struct repository *r, ctx->reflog_message = sequencer_reflog_action(opts); if (opts->allow_ff) - assert(!(opts->signoff || opts->no_commit || -+ BUG_IF_NOT(!(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)); -- gitgitgadget
