diff options
author | Junio C Hamano <gitster@pobox.com> | 2025-04-08 11:43:12 -0700 |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2025-04-08 11:43:12 -0700 |
commit | b97b360c514acd0f5a148524a85bcdb583dbe914 (patch) | |
tree | dc16900efcc7576ec5211f9c340e18bd01dd9020 | |
parent | 9d22ac51228304102deb62f30c3ecba6377e1237 (diff) | |
parent | 5633aa3af1282cad5161174f17867399e58b2a54 (diff) |
Merge branch 'en/assert-wo-side-effects'
Ensure what we write in assert() does not have side effects,
and introduce ASSERT() macro to mark those that cannot be
mechanically checked for lack of side effects.
* en/assert-wo-side-effects:
treewide: replace assert() with ASSERT() in special cases
ci: add build checking for side-effects in assert() calls
git-compat-util: introduce ASSERT() macro
-rw-r--r-- | Makefile | 4 | ||||
-rwxr-xr-x | ci/check-unsafe-assertions.sh | 18 | ||||
-rwxr-xr-x | ci/run-static-analysis.sh | 2 | ||||
-rw-r--r-- | diffcore-rename.c | 2 | ||||
-rw-r--r-- | git-compat-util.h | 9 | ||||
-rw-r--r-- | merge-ort.c | 4 | ||||
-rw-r--r-- | merge-recursive.c | 2 | ||||
-rw-r--r-- | object-file.c | 2 | ||||
-rw-r--r-- | parallel-checkout.c | 2 | ||||
-rw-r--r-- | scalar.c | 4 | ||||
-rw-r--r-- | sequencer.c | 2 |
11 files changed, 42 insertions, 9 deletions
@@ -2263,6 +2263,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 0000000000..233bd9dfbc --- /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 0d51e5ce0e..ae714e020a 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/diffcore-rename.c b/diffcore-rename.c index 5002e896aa..8077283fc7 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/git-compat-util.h b/git-compat-util.h index cf733b38ac..f8562996ed 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__) @@ -1592,4 +1594,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) */ #define NOT_CONSTANT(expr) ((expr) || false_but_the_compiler_does_not_know_it_) extern int false_but_the_compiler_does_not_know_it_; + +#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 diff --git a/merge-ort.c b/merge-ort.c index 2b7d86aa4e..c41f466577 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 884ccf99a5..4fbbece922 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 726e41a047..4fb3cd9dcb 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 7cc6b30528..57c2dcaa8f 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) { @@ -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 ad0ab75c8d..c625a39111 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)); |