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

["Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>]

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