[PATCH 0/3] Add a static analysis job to prevent assertions with side effects

[PATCH 0/3] Add a static analysis job to prevent assertions with side effects

[Elijah Newren via GitGitGadget]

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

Elijah Newren (3):
  git-compat-util: introduce BUG_IF_NOT() macro
  ci: add build checking for side-effects in assert() calls
  treewide: replace assert() with BUG_IF_NOT() 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             |  7 +++++++
 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, 40 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-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1881
-- 
gitgitgadget

[PATCH 1/3] git-compat-util: introduce BUG_IF_NOT() macro

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Create a BUG_IF_NOT() 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
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 <newren@gmail.com>
---
 git-compat-util.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index e123288e8f1..c3415ad7e0a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1460,6 +1460,7 @@ 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.")
 __attribute__((format (printf, 3, 4)))
 void bug_fl(const char *file, int line, const char *fmt, ...);
 #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

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

[PATCH 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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 91b77993c78..1a945945fab 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));
+	BUG_IF_NOT(!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 46e78c3ffa6..3db7a911f81 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 ==
+	BUG_IF_NOT(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 &&
+			BUG_IF_NOT(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 884ccf99a58..ab888689ae4 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));
+	BUG_IF_NOT(!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 726e41a0475..8ef4813eb63 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));
+	BUG_IF_NOT(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 7cc6b305281..4d2fa6d7374 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));
+	BUG_IF_NOT(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 da42b4be0cc..173286110ea 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());
+	BUG_IF_NOT(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());
+	BUG_IF_NOT(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 ad0ab75c8d4..98a7657b398 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 ||
+		BUG_IF_NOT(!(opts->signoff || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
 			 opts->committer_date_is_author_date ||
 			 opts->ignore_date));
-- 
gitgitgadget

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

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

A few style guides:

 - doing multiple echo into the same descriptor is easier to read if
   you have redirection near the beginning, i.e.

	echo >&2 "message one"
	echo >&2 "message two that may way be longer than the previous"
        ehco >&2 "message three"

 - multi-line pipelines are easier to follow without backslash by
   ending the previous line with a '|', i.e.

	grep ... file |
	sed -e ... |
	while read file line
	do
		...

 - I thought our "one indent one tab" standard extends to shell
   scripts as well?

> 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 */

Cute.  As this checking assert is in void context, the optimizing
compiler knows that the entire thing can be optimized away ONLY IF
it can somehow prove that (expr) has no side effect.  And if it does
not optimize it away, you will hit an error from the linker, saying
that the undefined variable is being used.

This requires a fairly good optimizing compiler that can peek into
(as in "inline") what is in expr to notice, so it cannot be free of
false positive, but at least when the optimization works as expected,
it is provably (modulo optimizer bugs) side-effect free.

Is this something we can use in our project?  I am just double
checking.

Thanks.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

On 2025-03-14 at 01:06:21, Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > 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 */
> 
> Cute.  As this checking assert is in void context, the optimizing
> compiler knows that the entire thing can be optimized away ONLY IF
> it can somehow prove that (expr) has no side effect.  And if it does
> not optimize it away, you will hit an error from the linker, saying
> that the undefined variable is being used.

I agree this is very clever.

> This requires a fairly good optimizing compiler that can peek into
> (as in "inline") what is in expr to notice, so it cannot be free of
> false positive, but at least when the optimization works as expected,
> it is provably (modulo optimizer bugs) side-effect free.
> 
> Is this something we can use in our project?  I am just double
> checking.

I believe it's valid in C99.  Certainly some compiler might be bad at
optimizing, or a user may have compiled with -O0, but this is run in CI,
where we have known good compilers and can control the optimization
flags.  I doubt GCC, Clang, or MSVC will have problems here, and since
this is not on by default, users using something less capable (the Tiny
C Compiler, maybe?) or a vendor compiler won't even see it.

Was there some other case that you were concerned about?
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> Is this something we can use in our project?  I am just double
>> checking.
>
> I believe it's valid in C99.  Certainly some compiler might be bad at
> optimizing, or a user may have compiled with -O0, but this is run in CI,
> where we have known good compilers and can control the optimization
> flags.  I doubt GCC, Clang, or MSVC will have problems here, and since
> this is not on by default, users using something less capable (the Tiny
> C Compiler, maybe?) or a vendor compiler won't even see it.
>
> Was there some other case that you were concerned about?

Licensing, mostly, as clever things we see are not necessarily home
grown.  I know the patch came with DCO sign-off, but it does not
hurt to double check.

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

[Elijah Newren]

On Thu, Mar 13, 2025 at 6:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> >> Is this something we can use in our project?  I am just double
> >> checking.
> >
> > I believe it's valid in C99.  Certainly some compiler might be bad at
> > optimizing, or a user may have compiled with -O0, but this is run in CI,
> > where we have known good compilers and can control the optimization
> > flags.  I doubt GCC, Clang, or MSVC will have problems here, and since
> > this is not on by default, users using something less capable (the Tiny
> > C Compiler, maybe?) or a vendor compiler won't even see it.
> >
> > Was there some other case that you were concerned about?
>
> Licensing, mostly, as clever things we see are not necessarily home
> grown.  I know the patch came with DCO sign-off, but it does not
> hurt to double check.

These two lines:

> +extern int not_supposed_to_survive;
> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))

, which serve as the core trick, I had used elsewhere before.  Doing
some searches, it looks like those likely came from
https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects.
And it appears that StackOverflow is CC-BY-SA-3.0 or -4.0.  Doh,
sorry.  Anyone got a clever alternative?

The rest of the patch was all written by me.

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> Licensing, mostly, as clever things we see are not necessarily home
>> grown.  I know the patch came with DCO sign-off, but it does not
>> hurt to double check.
>
> These two lines:
>
>> +extern int not_supposed_to_survive;
>> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))
>
> , which serve as the core trick, I had used elsewhere before.

It may be arguable that it is too small to be copyrightable and
there is no other way to express the idea behind that check, but
in any case ...

> Anyone got a clever alternative?

... as I cannot unsee your patch, I cannot be the one who comes up
with a clever alternative, if we are worried about licensing with
what you posted X-<.

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

[Elijah Newren]

On Fri, Mar 14, 2025 at 10:29 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> >> Licensing, mostly, as clever things we see are not necessarily home
> >> grown.  I know the patch came with DCO sign-off, but it does not
> >> hurt to double check.
> >
> > These two lines:
> >
> >> +extern int not_supposed_to_survive;
> >> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))
> >
> > , which serve as the core trick, I had used elsewhere before.
>
> It may be arguable that it is too small to be copyrightable and
> there is no other way to express the idea behind that check, but
> in any case ...

That's what I had been assuming, but then you, brian, and Taylor all
pointed out how clever it was making me think otherwise.

> > Anyone got a clever alternative?
>
> ... as I cannot unsee your patch, I cannot be the one who comes up
> with a clever alternative, if we are worried about licensing with
> what you posted X-<.

Turns out we don't need an alternative.  I contacted the author, who
responded and placed the two-liner into the public domain with no
warranty of any kind.  I'll send a re-roll with an updated commit
message.

[PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects

[Elijah Newren via GitGitGadget]

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

Elijah Newren (3):
  git-compat-util: introduce BUG_IF_NOT() macro
  ci: add build checking for side-effects in assert() calls
  treewide: replace assert() with BUG_IF_NOT() 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             |  7 +++++++
 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, 40 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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1881

Range-diff vs v1:

 1:  109060ccb86 = 1:  109060ccb86 git-compat-util: introduce BUG_IF_NOT() macro
 2:  80dcc2ba3aa ! 2:  58cb8f6a160 ci: add build checking for side-effects in assert() calls
     @@ Commit message
          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.
     +    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:
      
     @@ Commit message
          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>
      
       ## Makefile ##
 3:  4c668039bb7 = 3:  20c763f2951 treewide: replace assert() with BUG_IF_NOT() in special cases

-- 
gitgitgadget

[PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Create a BUG_IF_NOT() 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
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 <newren@gmail.com>
---
 git-compat-util.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index e123288e8f1..c3415ad7e0a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1460,6 +1460,7 @@ 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.")
 __attribute__((format (printf, 3, 4)))
 void bug_fl(const char *file, int line, const char *fmt, ...);
 #define bug(...) bug_fl(__FILE__, __LINE__, __VA_ARGS__)
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

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() 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 <defraine@gmail.com>
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

[PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 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 91b77993c78..1a945945fab 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));
+	BUG_IF_NOT(!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 46e78c3ffa6..3db7a911f81 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 ==
+	BUG_IF_NOT(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 &&
+			BUG_IF_NOT(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 884ccf99a58..ab888689ae4 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));
+	BUG_IF_NOT(!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 726e41a0475..8ef4813eb63 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));
+	BUG_IF_NOT(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 7cc6b305281..4d2fa6d7374 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));
+	BUG_IF_NOT(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 da42b4be0cc..173286110ea 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());
+	BUG_IF_NOT(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());
+	BUG_IF_NOT(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 ad0ab75c8d4..98a7657b398 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 ||
+		BUG_IF_NOT(!(opts->signoff || opts->no_commit ||
 			 opts->record_origin || should_edit(opts) ||
 			 opts->committer_date_is_author_date ||
 			 opts->ignore_date));
-- 
gitgitgadget

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

[Elijah Newren]

On Sat, Mar 15, 2025 at 11:38 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Fri, Mar 14, 2025 at 10:29 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Elijah Newren <newren@gmail.com> writes:
> >
> > >> Licensing, mostly, as clever things we see are not necessarily home
> > >> grown.  I know the patch came with DCO sign-off, but it does not
> > >> hurt to double check.
> > >
> > > These two lines:
> > >
> > >> +extern int not_supposed_to_survive;
> > >> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))
> > >
> > > , which serve as the core trick, I had used elsewhere before.
> >
> > It may be arguable that it is too small to be copyrightable and
> > there is no other way to express the idea behind that check, but
> > in any case ...
>
> That's what I had been assuming, but then you, brian, and Taylor all
> pointed out how clever it was making me think otherwise.
>
> > > Anyone got a clever alternative?
> >
> > ... as I cannot unsee your patch, I cannot be the one who comes up
> > with a clever alternative, if we are worried about licensing with
> > what you posted X-<.
>
> Turns out we don't need an alternative.  I contacted the author, who
> responded and placed the two-liner into the public domain with no
> warranty of any kind.  I'll send a re-roll with an updated commit
> message.

And of course, in my excitement to send a re-roll addressing that
issue, I totally spaced fixing the style issues you pointed out.  I'll
wait a couple days for any comments and then send a re-roll with those
fixes.

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

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> >> +extern int not_supposed_to_survive;
>> >> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))
>> >
>> > , which serve as the core trick, I had used elsewhere before.
>>
>> It may be arguable that it is too small to be copyrightable and
>> there is no other way to express the idea behind that check, but
>> in any case ...
>
> That's what I had been assuming, but then you, brian, and Taylor all
> pointed out how clever it was making me think otherwise.

Heh, cleverness lies in the idea, not the expression, and copyright
is about expression.

> Turns out we don't need an alternative.  I contacted the author, who
> responded and placed the two-liner into the public domain with no
> warranty of any kind.  I'll send a re-roll with an updated commit
> message.

Wonderful.  That is the best solution.

Thanks.

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

[Taylor Blau]

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

Re: [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases

[Taylor Blau]

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

Nice. I guess since this is split out into its own patch, we wouldn't be
able to cleanly run CI on the previous commit, but I think that's fine,
since we don't treat CI as being nearly as precious as being able to
'make' anywhere in history.

You didn't realign any multi-line assert() statements, but I actually
think that's preferable in this case to demonstrate that the patch does
not change the behavior of these assertions.

Thanks,
Taylor

Re: [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> Create a BUG_IF_NOT() 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.

If this is meant to be "similar to" assert, let's not call it
BUG_IF_NOT().  The point of BUG() is that the developer can mark the
problem with something more than just a conditional, and it feels
funny to call a facility that lacks that central feature with a name
with BUG in it.

ASSERT(), safer_assert(), safe_assert(), sane_assert()?  

The last one is in line with safe_istest() that is used on
sane_ctype[] and sane_qsort(), with the intention to allow
developers to write right code more easily than using the plain
vanilla C.

Thanks.

Re: [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects

[Taylor Blau]

On Sun, Mar 16, 2025 at 06:41:59AM +0000, Elijah Newren via GitGitGadget wrote:
> Elijah Newren (3):
>   git-compat-util: introduce BUG_IF_NOT() macro
>   ci: add build checking for side-effects in assert() calls
>   treewide: replace assert() with BUG_IF_NOT() in special cases

Nice, this version looks great to me. I left a couple of notes
throughout, but they range from "could be done later" to idle
commentary. Thanks for working on this, and I'm sorry to have sent you
down such a rabbit hole ;-).

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro

[Taylor Blau]

On Mon, Mar 17, 2025 at 03:33:50PM -0700, Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Create a BUG_IF_NOT() 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.
>
> If this is meant to be "similar to" assert, let's not call it
> BUG_IF_NOT().  The point of BUG() is that the developer can mark the
> problem with something more than just a conditional, and it feels
> funny to call a facility that lacks that central feature with a name
> with BUG in it.
>
> ASSERT(), safer_assert(), safe_assert(), sane_assert()?
>
> The last one is in line with safe_istest() that is used on
> sane_ctype[] and sane_qsort(), with the intention to allow
> developers to write right code more easily than using the plain
> vanilla C.

For my $.02, I prefer ASSERT() to the other options. It's clear, but
indicates that it's a macro and thus not the same as assert(3). But I
don't have a strong opinion here.

Thanks,
Taylor

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

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> 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.

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

[Elijah Newren]

On Mon, Mar 17, 2025 at 3:30 PM Taylor Blau <me@ttaylorr.com> 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 :-).

:-)

[PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects

[Elijah Newren via GitGitGadget]

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@gmail.com>
      
       ## 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@gmail.com>
      
       ## 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@gmail.com>
      
     @@ 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

[PATCH v3 1/3] git-compat-util: introduce ASSERT() macro

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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 <newren@gmail.com>
---
 git-compat-util.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index e123288e8f1..d7f3407128c 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__)
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

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() 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>
---
 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..233bd9dfbc4
--- /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 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 d7f3407128c..5891efaeb18 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
-- 
gitgitgadget

[PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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 <newren@gmail.com>
---
 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 91b77993c78..624304f0416 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 46e78c3ffa6..d484f16cf2b 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 884ccf99a58..4fbbece922c 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 726e41a0475..4fb3cd9dcb9 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 7cc6b305281..57c2dcaa8f6 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 da42b4be0cc..d359f08bb8e 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 ad0ab75c8d4..c625a39111e 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));
-- 
gitgitgadget

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

[Taylor Blau]

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

Re: [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases

[Taylor Blau]

On Wed, Mar 19, 2025 at 04:22:58PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> 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().

As a nice side-benefit, since our new ASSERT() macro naturally has the
same number of characters as a bog-standard assert(), we don't have to
realign any multi-line assertions below ;-).

Thanks,
Taylor

Re: [PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects

[Taylor Blau]

On Wed, Mar 19, 2025 at 04:22:55PM +0000, Elijah Newren via GitGitGadget wrote:
> 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

Thanks, this version LGTM.

Thanks,
Taylor