git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/3] ci: add build checking for side-effects in assert() calls

From
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Date
Mar 14, 2025, 00:20 UTC
Message-ID
<80dcc2ba3aa0ef72abe18f8525d571ea39ac6382.1741911652.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1881.git.1741911652.gitgitgadget@gmail.com>
From: Elijah Newren <newren@gmail.com>

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(). The current 9 appear to be free of side effects to me 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 BUG_IF_NOT() 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.
/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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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 7315507381e..57774912f18 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 00000000000..d66091efd22
--- /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 "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
+fi
+rm compiler_output compiler_error
diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index 0d51e5ce0e7..ae714e020ae 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 c3415ad7e0a..0aefd763751 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1584,4 +1584,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
-- 
gitgitgadget
Previous: Elijah Newren via GitGitGadgetNext: Junio C Hamano
Message 3 of 30 in “Add a static analysis job to prevent assertions with side effects”
  1. 0/3 Add a static analysis job to prevent assertions with side effectsElijah Newren via GitGitGadget, Mar 14, 2025
  2. 1/3 git-compat-util: introduce BUG_IF_NOT() macroElijah Newren via GitGitGadget, Mar 14, 2025
  3. 2/3 ci: add build checking for side-effects in assert() callsElijah Newren via GitGitGadget, Mar 14, 2025
  4. Junio C HamanoMar 14, 2025
  5. brian m. carlsonMar 14, 2025
  6. Junio C HamanoMar 14, 2025
  7. Elijah NewrenMar 14, 2025
  8. Junio C HamanoMar 14, 2025
  9. Elijah NewrenMar 16, 2025
  10. Elijah NewrenMar 17, 2025
  11. Junio C HamanoMar 17, 2025
  12. 3/3 treewide: replace assert() with BUG_IF_NOT() in special casesElijah Newren via GitGitGadget, Mar 14, 2025
  13. 0/3 Add a static analysis job to prevent assertions with side effectsElijah Newren via GitGitGadget, Mar 16, 2025
  14. 1/3 git-compat-util: introduce BUG_IF_NOT() macroElijah Newren via GitGitGadget, Mar 16, 2025
  15. Junio C HamanoMar 17, 2025
  16. Taylor BlauMar 17, 2025
  17. 2/3 ci: add build checking for side-effects in assert() callsElijah Newren via GitGitGadget, Mar 16, 2025
  18. Taylor BlauMar 17, 2025
  19. Elijah NewrenMar 19, 2025
  20. Taylor BlauMar 19, 2025
  21. Junio C HamanoMar 17, 2025
  22. 3/3 treewide: replace assert() with BUG_IF_NOT() in special casesElijah Newren via GitGitGadget, Mar 16, 2025
  23. Taylor BlauMar 17, 2025
  24. Taylor BlauMar 17, 2025
  25. 0/3 Add a static analysis job to prevent assertions with side effectsElijah Newren via GitGitGadget, Mar 19, 2025
  26. 1/3 git-compat-util: introduce ASSERT() macroElijah Newren via GitGitGadget, Mar 19, 2025
  27. 2/3 ci: add build checking for side-effects in assert() callsElijah Newren via GitGitGadget, Mar 19, 2025
  28. 3/3 treewide: replace assert() with ASSERT() in special casesElijah Newren via GitGitGadget, Mar 19, 2025
  29. Taylor BlauMar 19, 2025
  30. Taylor BlauMar 19, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.