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

Commit Message

Elijah Newren March 16, 2025, 6:42 a.m. UTC

From: Elijah Newren <[e-mail]>

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 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.

Helped-by: Bruno De Fraine <[e-mail]>
Signed-off-by: Elijah Newren <[e-mail]>
---
 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

Comments

Taylor Blau March 17, 2025, 10:30 p.m. UTC | #1

On Sun, Mar 16, 2025 at 06:42:01AM +0000, Elijah Newren via GitGitGadget wrote:
> 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 BUG_IF_NOT() calls.

Very nice, and thank you Bruno for placing your very clever assert() in
the public domain :-).

I wonder if it might be useful to explain this in
Documentation/CodingGuidelines as a follow-up to this series. I was
thinking of a scenario where someone either writes a side-effecting
assert(), or a non-side-effecting one that is too complicated to prove
otherwise.

If that person runs 'make test' locally, they might not see any
failures, but then be surprised when CI fails on the new step. It may be
worth mentioning that we have such a check, and that we expect all
assert() statements to be side effect-free, and that developers can
verify this by ci/check-unsafe-assertions.sh.

But that may bring us into an assert() versus BUG_IF_NOT() debate, which
may be somewhat counterproductive, so I'm just as happy if you did
nothing here :-).

Thanks,
Taylor

Junio C Hamano March 17, 2025, 10:37 p.m. UTC | #2

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> ...  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.

Nice.

> 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

Let's lose the unsightly backslash by ending each line with "|"
instead.

Also let's stick to HT indentation, not whitespaces.

Thanks.

Elijah Newren March 19, 2025, 4:21 p.m. UTC | #3

On Mon, Mar 17, 2025 at 3:30 PM Taylor Blau <[e-mail]> wrote:
>
> On Sun, Mar 16, 2025 at 06:42:01AM +0000, Elijah Newren via GitGitGadget wrote:
> > 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 BUG_IF_NOT() calls.
>
> Very nice, and thank you Bruno for placing your very clever assert() in
> the public domain :-).
>
> I wonder if it might be useful to explain this in
> Documentation/CodingGuidelines as a follow-up to this series. I was
> thinking of a scenario where someone either writes a side-effecting
> assert(), or a non-side-effecting one that is too complicated to prove
> otherwise.
>
> If that person runs 'make test' locally, they might not see any
> failures, but then be surprised when CI fails on the new step. It may be
> worth mentioning that we have such a check, and that we expect all
> assert() statements to be side effect-free, and that developers can
> verify this by ci/check-unsafe-assertions.sh.

The same could be said for coccinelle patches, hdr-check, check-pot,
fuzz tests, asan/ubsan, GIT_TEST_SPLIT_INDEX, pedantic build, osx, vs.
windows vs. linux, and perhaps others, which users won't catch on
'make test' locally but can result in failed CI builds and aren't
mentioned in CodingGuidelines.  I usually think of CodingGuidelines as
being the place for documenting things that can't be tested in an
automated fashion, and a brief mention that both cross platform and
additional more thorough but non-default tests can go in
SubmittingPatches.

> But that may bring us into an assert() versus BUG_IF_NOT() debate, which
> may be somewhat counterproductive, so I'm just as happy if you did
> nothing here :-).

:-)

Taylor Blau March 19, 2025, 10:26 p.m. UTC | #4

On Wed, Mar 19, 2025 at 09:21:59AM -0700, Elijah Newren wrote:
> > I wonder if it might be useful to explain this in
> > Documentation/CodingGuidelines as a follow-up to this series. I was
> > thinking of a scenario where someone either writes a side-effecting
> > assert(), or a non-side-effecting one that is too complicated to prove
> > otherwise.
> >
> > If that person runs 'make test' locally, they might not see any
> > failures, but then be surprised when CI fails on the new step. It may be
> > worth mentioning that we have such a check, and that we expect all
> > assert() statements to be side effect-free, and that developers can
> > verify this by ci/check-unsafe-assertions.sh.
>
> The same could be said for coccinelle patches, hdr-check, check-pot,
> fuzz tests, asan/ubsan, GIT_TEST_SPLIT_INDEX, pedantic build, osx, vs.
> windows vs. linux, and perhaps others, which users won't catch on
> 'make test' locally but can result in failed CI builds and aren't
> mentioned in CodingGuidelines.  I usually think of CodingGuidelines as
> being the place for documenting things that can't be tested in an
> automated fashion, and a brief mention that both cross platform and
> additional more thorough but non-default tests can go in
> SubmittingPatches.

Fair enough ;-).

Thanks,
Taylor

Patch

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