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 <defraine@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Acked-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Mar 19, 2025 at 16:22 UTC 85e4f762c224c52708919a10c9fa5b79e7949564
4 files changed +30
Makefile
+4
@@ -2261,6 +2261,10 @@ ifdef WITH_BREAKING_CHANGES
2261 BASIC_CFLAGS += -DWITH_BREAKING_CHANGES
2262 endif
2263
2264 +ifdef CHECK_ASSERTION_SIDE_EFFECTS
2265 + BASIC_CFLAGS += -DCHECK_ASSERTION_SIDE_EFFECTS
2266 +endif
2267 +
2268 ifdef INCLUDE_LIBGIT_RS
2269 # Enable symbol hiding in contrib/libgit-sys/libgitpub.a without making
2270 # us rebuild the whole tree every time we run a Rust build.
ci/check-unsafe-assertions.sh new
+18
@@ -0,0 +1,18 @@
1 +#!/bin/sh
2 +
3 +make CHECK_ASSERTION_SIDE_EFFECTS=1 >compiler_output 2>compiler_error
4 +if test $? != 0
5 +then
6 + echo >&2 "ERROR: The compiler could not verify the following assert()"
7 + echo >&2 " calls are free of side-effects. Please replace with"
8 + echo >&2 " ASSERT() calls."
9 + grep undefined.reference.to..not_supposed_to_survive compiler_error |
10 + sed -e s/:[^:]*$// | sort | uniq | tr ':' ' ' |
11 + while read f l
12 + do
13 + printf "${f}:${l}\n "
14 + awk -v start="$l" 'NR >= start { print; if (/\);/) exit }' $f
15 + done
16 + exit 1
17 +fi
18 +rm compiler_output compiler_error
ci/run-static-analysis.sh
+2
@@ -31,4 +31,6 @@ exit 1
31
32 make check-pot
33
34 +${0%/*}/check-unsafe-assertions.sh
35 +
36 save_good_tree
git-compat-util.h
+6
@@ -1585,4 +1585,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)
1585 ((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))
1586 #endif /* !__GNUC__ */
1587
1588 +#ifdef CHECK_ASSERTION_SIDE_EFFECTS
1589 +#undef assert
1590 +extern int not_supposed_to_survive;
1591 +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))
1592 +#endif /* CHECK_ASSERTION_SIDE_EFFECTS */
1593 +
1594 #endif