Add a static analysis job to prevent assertions with side effects
cover Add a static analysis job to prevent assertions with side effects
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
Signature · 1 line
gitgitgadget
PATCH 1/3 git-compat-util: introduce BUG_IF_NOT() macro
From: Elijah Newren <[email protected]>
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.
--- git-compat-util.h | 1 + 1 file changed, 1 insertion(+)
git-compat-util.h +1 -0
@@ -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__)
Signature · 1 line
gitgitgadget
PATCH 2/3 ci: add build checking for side-effects in assert() calls
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
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.
--- 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
Makefile +4 -0
@@ -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.
ci/check-unsafe-assertions.sh new file +18 -0
@@ -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
ci/run-static-analysis.sh +2 -0
@@ -31,4 +31,6 @@ exit 1 make check-pot +${0%/*}/check-unsafe-assertions.sh + save_good_tree
git-compat-util.h +6 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls
ci/check-unsafe-assertions.sh line 11
Quoted 9 lines · Elijah Newren via GitGitGadget
ci/check-unsafe-assertions.sh new file +9
-0+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?
git-compat-util.h static inline void *container_of_or_null_offset(void *ptr, size_t offset) · line 1591
Quoted 13 lines
git-compat-util.h +5
-0@@ -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
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Elijah Newren via GitGitGadget
git-compat-util.h +0
-0@@ -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__ */
Quoted 11 lines
git-compat-util.h static inline void *container_of_or_null_offset(void *ptr, size_t offset) · line 1591
Quoted 5 lines
git-compat-util.h +5
-0+#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.
Quoted 7 lines
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?
Signature · 2 lines
brian m. carlson (they/them or he/him) Toronto, Ontario, CA
Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls
Quoted 11 lines · brian m. carlson
Quoted 2 lines
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
Quoted 13 lines · Junio C Hamano
Quoted 10 lines · brian m. carlson
Quoted 2 lines
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.
Quoted 5 lines
Quoted 1 line
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:
git-compat-util.h static inline void *container_of_or_null_offset(void *ptr, size_t offset) · line 1590
Quoted 2 lines
git-compat-util.h +2
-0+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
Quoted 10 lines · Elijah Newren
Quoted 3 lines
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:
git-compat-util.h static inline void *container_of_or_null_offset(void *ptr, size_t offset) · line 1590
Quoted 2 lines
git-compat-util.h +2
-0+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 ...
Quoted 1 line
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
Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Elijah Newren
Quoted 3 lines
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:
Quoted 8 lines
Quoted 4 lines
git-compat-util.h static inline void *container_of_or_null_offset(void *ptr, size_t offset) · line 1590
Quoted 2 lines
git-compat-util.h +2
-0+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.
Quoted 5 lines
Quoted 1 line
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.
Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls
Quoted 25 lines · Elijah Newren
Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Elijah Newren
Quoted 3 lines
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:
Quoted 8 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+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.
Quoted 2 lines
Quoted 1 line
Anyone got a clever alternative?
Quoted 8 lines
Quoted 3 lines
... 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
In reply to Elijah NewrenQuoted 11 lines · Elijah Newren
Quoted 8 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+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.
Quoted 4 lines
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.
PATCH 3/3 treewide: replace assert() with BUG_IF_NOT() in special cases
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
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().
--- 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(-)
diffcore-rename.c +1 -1
@@ -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" );
merge-ort.c +2 -2
@@ -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);
merge-recursive.c +1 -1
@@ -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);
object-file.c +1 -1
@@ -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));
parallel-checkout.c +1 -1
@@ -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) {
scalar.c +2 -2
@@ -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 );
sequencer.c +1 -1
@@ -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));
Signature · 1 line
gitgitgadget
v2 cover Add a static analysis job to prevent assertions with side effects
In reply to Elijah Newren via GitGitGadgetWe 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 <[email protected]>
## Makefile ##
3: 4c668039bb7 = 3: 20c763f2951 treewide: replace assert() with BUG_IF_NOT() in special cases
Signature · 1 line
gitgitgadget
PATCH v2 1/3 git-compat-util: introduce BUG_IF_NOT() macro
From: Elijah Newren <[email protected]>
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.
--- git-compat-util.h | 1 + 1 file changed, 1 insertion(+)
git-compat-util.h +1 -0
@@ -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__)
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/3] git-compat-util: introduce BUG_IF_NOT() macro
Quoted 5 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
Create a
BUG_IF_NOT()macro which is similar toassert(), 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 1/3] git-compat-util: introduce BUG_IF_NOT() macro
Quoted 14 lines · Junio C Hamano
Quoted 5 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
Create a
BUG_IF_NOT()macro which is similar toassert(), 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 ofBUG()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.
Quoted 6 lines
ASSERT(),safer_assert(),safe_assert(),sane_assert()?The last one is in line with
safe_istest()that is used on sane_ctype[] andsane_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
PATCH v2 2/3 ci: add build checking for side-effects in assert() calls
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
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.
--- 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
Makefile +4 -0
@@ -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.
ci/check-unsafe-assertions.sh new file +18 -0
@@ -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
ci/run-static-analysis.sh +2 -0
@@ -31,4 +31,6 @@ exit 1 make check-pot +${0%/*}/check-unsafe-assertions.sh + save_good_tree
git-compat-util.h +6 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls
Quoted 6 lines · Elijah Newren via GitGitGadget
We have roughly 566
assert()calls in our codebase (my grep might have picked up things that aren't actuallyassert()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 ofassert()provided by Bruno De Fraine (from
https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),
Quoted 8 lines
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 ofassert()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 withBUG_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 2/3] ci: add build checking for side-effects in assert() calls
Quoted 26 lines · Taylor Blau
Quoted 6 lines · Elijah Newren via GitGitGadget
We have roughly 566
assert()calls in our codebase (my grep might have picked up things that aren't actuallyassert()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 ofassert()provided by Bruno De Fraine (from
https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),Quoted 8 lines
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 ofassert()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 withBUG_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/CodingGuidelinesas a follow-up to this series. I was thinking of a scenario where someone either writes a side-effectingassert(), or a non-side-effecting one that is too complicated to prove otherwise.
Quoted 5 lines
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 byci/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.
Quoted 3 lines
But that may bring us into an
assert()versusBUG_IF_NOT()debate, which may be somewhat counterproductive, so I'm just as happy if you did nothing here :-).
:-)
Re: [PATCH v2 2/3] ci: add build checking for side-effects in assert() calls
Quoted 12 lines · Elijah Newren
Quoted 11 lines
I wonder if it might be useful to explain this in
Documentation/CodingGuidelinesas a follow-up to this series. I was thinking of a scenario where someone either writes a side-effectingassert(), 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 byci/check-unsafe-assertions.sh.
Quoted 9 lines
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 v2 2/3] ci: add build checking for side-effects in assert() calls
In reply to Elijah Newren via GitGitGadgetQuoted 6 lines · Elijah Newren via GitGitGadget
... 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.
Quoted 7 lines
ci/check-unsafe-assertions.sh new file +1
-0@@ -0,0 +1,18 @@ +#!/bin/sh
ci/check-unsafe-assertions.sh line 11
Quoted 10 lines
ci/check-unsafe-assertions.sh new file +10
-0+ +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.
PATCH v2 3/3 treewide: replace assert() with BUG_IF_NOT() in special cases
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
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().
--- 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(-)
diffcore-rename.c +1 -1
@@ -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" );
merge-ort.c +2 -2
@@ -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);
merge-recursive.c +1 -1
@@ -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);
object-file.c +1 -1
@@ -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));
parallel-checkout.c +1 -1
@@ -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) {
scalar.c +2 -2
@@ -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 );
sequencer.c +1 -1
@@ -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));
Signature · 1 line
gitgitgadget
Re: [PATCH v2 3/3] treewide: replace assert() with BUG_IF_NOT() in special cases
Quoted 3 lines · Elijah Newren via GitGitGadget
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 ofassert()withBUG_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
Quoted 4 lines · Elijah Newren via GitGitGadget
Elijah Newren (3):
git-compat-util: introduceBUG_IF_NOT()macro
ci: add build checking for side-effects inassert()calls
treewide: replaceassert()withBUG_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 ;-).
Thanks,
Taylor
v3 cover Add a static analysis job to prevent assertions with side effects
In reply to Elijah Newren via GitGitGadgetWe 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 <[email protected]>
## 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 <[email protected]>
## 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 <[email protected]>
@@ 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));Signature · 1 line
gitgitgadget
PATCH v3 1/3 git-compat-util: introduce ASSERT() macro
From: Elijah Newren <[email protected]>
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.
--- git-compat-util.h | 2 ++ 1 file changed, 2 insertions(+)
git-compat-util.h +2 -0
@@ -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__)
Signature · 1 line
gitgitgadget
PATCH v3 2/3 ci: add build checking for side-effects in assert() calls
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
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.
--- 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
Makefile +4 -0
@@ -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.
ci/check-unsafe-assertions.sh new file +18 -0
@@ -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
ci/run-static-analysis.sh +2 -0
@@ -31,4 +31,6 @@ exit 1 make check-pot +${0%/*}/check-unsafe-assertions.sh + save_good_tree
git-compat-util.h +6 -0
@@ -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
Signature · 1 line
gitgitgadget
PATCH v3 3/3 treewide: replace assert() with ASSERT() in special cases
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
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().
--- 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(-)
diffcore-rename.c +1 -1
@@ -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" );
merge-ort.c +2 -2
@@ -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);
merge-recursive.c +1 -1
@@ -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);
object-file.c +1 -1
@@ -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));
parallel-checkout.c +1 -1
@@ -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) {
scalar.c +2 -2
@@ -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 );
sequencer.c +1 -1
@@ -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));
Signature · 1 line
gitgitgadget
Re: [PATCH v3 3/3] treewide: replace assert() with ASSERT() in special cases
Quoted 5 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
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 ofassert()withASSERT().
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
Quoted 14 lines · Elijah Newren via GitGitGadget
Changes since v1:
* Tweaked commit message for patch 2 Changes since v2:
* RenameBUT_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 2Elijah Newren (3):
git-compat-util: introduceASSERT()macro
ci: add build checking for side-effects inassert()calls
treewide: replaceassert()withASSERT()in special cases
Quoted 13 lines
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