* [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro
2025-03-16 6:41 ` [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects Elijah Newren via GitGitGadget
@ 2025-03-16 6:42 ` Elijah Newren via GitGitGadget
2025-03-17 22:33 ` Junio C Hamano
2025-03-16 6:42 ` [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls Elijah Newren via GitGitGadget
` (3 subsequent siblings)
4 siblings, 1 reply; 30+ messages in thread
From: Elijah Newren via GitGitGadget @ 2025-03-16 6:42 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Elijah Newren, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro
2025-03-16 6:42 ` [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro Elijah Newren via GitGitGadget
@ 2025-03-17 22:33 ` Junio C Hamano
2025-03-17 22:36 ` Taylor Blau
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2025-03-17 22:33 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, brian m. carlson, Elijah Newren
"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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro
2025-03-17 22:33 ` Junio C Hamano
@ 2025-03-17 22:36 ` Taylor Blau
0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2025-03-17 22:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: Elijah Newren via GitGitGadget, git, brian m. carlson,
Elijah Newren
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls
2025-03-16 6:41 ` [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects Elijah Newren via GitGitGadget
2025-03-16 6:42 ` [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro Elijah Newren via GitGitGadget
@ 2025-03-16 6:42 ` Elijah Newren via GitGitGadget
2025-03-17 22:30 ` Taylor Blau
2025-03-17 22:37 ` Junio C Hamano
2025-03-16 6:42 ` [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases Elijah Newren via GitGitGadget
` (2 subsequent siblings)
4 siblings, 2 replies; 30+ messages in thread
From: Elijah Newren via GitGitGadget @ 2025-03-16 6:42 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Elijah Newren, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls
2025-03-16 6:42 ` [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls Elijah Newren via GitGitGadget
@ 2025-03-17 22:30 ` Taylor Blau
2025-03-19 16:21 ` Elijah Newren
2025-03-17 22:37 ` Junio C Hamano
1 sibling, 1 reply; 30+ messages in thread
From: Taylor Blau @ 2025-03-17 22:30 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, brian m. carlson, Elijah Newren
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls
2025-03-17 22:30 ` Taylor Blau
@ 2025-03-19 16:21 ` Elijah Newren
2025-03-19 22:26 ` Taylor Blau
0 siblings, 1 reply; 30+ messages in thread
From: Elijah Newren @ 2025-03-19 16:21 UTC (permalink / raw)
To: Taylor Blau; +Cc: Elijah Newren via GitGitGadget, git, brian m. carlson
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 :-).
:-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls
2025-03-19 16:21 ` Elijah Newren
@ 2025-03-19 22:26 ` Taylor Blau
0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2025-03-19 22:26 UTC (permalink / raw)
To: Elijah Newren; +Cc: Elijah Newren via GitGitGadget, git, brian m. carlson
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls
2025-03-16 6:42 ` [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls Elijah Newren via GitGitGadget
2025-03-17 22:30 ` Taylor Blau
@ 2025-03-17 22:37 ` Junio C Hamano
1 sibling, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2025-03-17 22:37 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, brian m. carlson, Elijah Newren
"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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases
2025-03-16 6:41 ` [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects Elijah Newren via GitGitGadget
2025-03-16 6:42 ` [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro Elijah Newren via GitGitGadget
2025-03-16 6:42 ` [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls Elijah Newren via GitGitGadget
@ 2025-03-16 6:42 ` Elijah Newren via GitGitGadget
2025-03-17 22:33 ` Taylor Blau
2025-03-17 22:34 ` [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects Taylor Blau
2025-03-19 16:22 ` [PATCH v3 " Elijah Newren via GitGitGadget
4 siblings, 1 reply; 30+ messages in thread
From: Elijah Newren via GitGitGadget @ 2025-03-16 6:42 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Elijah Newren, Elijah Newren, Elijah Newren
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases
2025-03-16 6:42 ` [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases Elijah Newren via GitGitGadget
@ 2025-03-17 22:33 ` Taylor Blau
0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2025-03-17 22:33 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, brian m. carlson, Elijah Newren
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects
2025-03-16 6:41 ` [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects Elijah Newren via GitGitGadget
` (2 preceding siblings ...)
2025-03-16 6:42 ` [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases Elijah Newren via GitGitGadget
@ 2025-03-17 22:34 ` Taylor Blau
2025-03-19 16:22 ` [PATCH v3 " Elijah Newren via GitGitGadget
4 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2025-03-17 22:34 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, brian m. carlson, Elijah Newren
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects
2025-03-16 6:41 ` [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects Elijah Newren via GitGitGadget
` (3 preceding siblings ...)
2025-03-17 22:34 ` [PATCH v2 0/3] Add a static analysis job to prevent assertions with side effects Taylor Blau
@ 2025-03-19 16:22 ` Elijah Newren via GitGitGadget
2025-03-19 16:22 ` [PATCH v3 1/3] git-compat-util: introduce ASSERT() macro Elijah Newren via GitGitGadget
` (3 more replies)
4 siblings, 4 replies; 30+ messages in thread
From: Elijah Newren via GitGitGadget @ 2025-03-19 16:22 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Elijah Newren, Taylor Blau, Elijah Newren
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
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH v3 1/3] git-compat-util: introduce ASSERT() macro
2025-03-19 16:22 ` [PATCH v3 " Elijah Newren via GitGitGadget
@ 2025-03-19 16:22 ` Elijah Newren via GitGitGadget
2025-03-19 16:22 ` [PATCH v3 2/3] ci: add build checking for side-effects in assert() calls Elijah Newren via GitGitGadget
` (2 subsequent siblings)
3 siblings, 0 replies; 30+ messages in thread
From: Elijah Newren via GitGitGadget @ 2025-03-19 16:22 UTC (permalink / raw)
To: git
Cc: brian m. carlson, Elijah Newren, Taylor Blau, Elijah Newren,
Elijah Newren
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v3 2/3] ci: add build checking for side-effects in assert() calls
2025-03-19 16:22 ` [PATCH v3 " Elijah Newren via GitGitGadget
2025-03-19 16:22 ` [PATCH v3 1/3] git-compat-util: introduce ASSERT() macro Elijah Newren via GitGitGadget
@ 2025-03-19 16:22 ` Elijah Newren via GitGitGadget
2025-03-19 16:22 ` [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases Elijah Newren via GitGitGadget
2025-03-19 22:27 ` [PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects Taylor Blau
3 siblings, 0 replies; 30+ messages in thread
From: Elijah Newren via GitGitGadget @ 2025-03-19 16:22 UTC (permalink / raw)
To: git
Cc: brian m. carlson, Elijah Newren, Taylor Blau, Elijah Newren,
Elijah Newren
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases
2025-03-19 16:22 ` [PATCH v3 " Elijah Newren via GitGitGadget
2025-03-19 16:22 ` [PATCH v3 1/3] git-compat-util: introduce ASSERT() macro Elijah Newren via GitGitGadget
2025-03-19 16:22 ` [PATCH v3 2/3] ci: add build checking for side-effects in assert() calls Elijah Newren via GitGitGadget
@ 2025-03-19 16:22 ` Elijah Newren via GitGitGadget
2025-03-19 22:27 ` Taylor Blau
2025-03-19 22:27 ` [PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects Taylor Blau
3 siblings, 1 reply; 30+ messages in thread
From: Elijah Newren via GitGitGadget @ 2025-03-19 16:22 UTC (permalink / raw)
To: git
Cc: brian m. carlson, Elijah Newren, Taylor Blau, Elijah Newren,
Elijah Newren
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases
2025-03-19 16:22 ` [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases Elijah Newren via GitGitGadget
@ 2025-03-19 22:27 ` Taylor Blau
0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2025-03-19 22:27 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, brian m. carlson, Elijah Newren
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v3 0/3] Add a static analysis job to prevent assertions with side effects
2025-03-19 16:22 ` [PATCH v3 " Elijah Newren via GitGitGadget
` (2 preceding siblings ...)
2025-03-19 16:22 ` [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases Elijah Newren via GitGitGadget
@ 2025-03-19 22:27 ` Taylor Blau
3 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2025-03-19 22:27 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, brian m. carlson, Elijah Newren
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
^ permalink raw reply [flat|nested] 30+ messages in thread