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

From: "Elijah Newren via GitGitGadget" <[email-masked]>
Date: Sun, 16 Mar 2025 06:41:59 +0000

We have several hundred assert() invocations in our code base. Some have
suggested that we should add a recommendation in our CodingGuidelines to
avoid their use, because there is a risk that someone might include
something with a side-effect in their assertion, which can lead to a very
difficult to debug problem. However, CodingGuidelines are going to be less
effective at preventing that foot-gun than a CI job which can warn of
assertions that possibly have side-effects. So, let's add a CI job instead.

While it is difficult to perfectly determine whether any expression has side
effects, a simple compiler/linker hack can prove that all but 9 of our
several hundred assert() calls are indeed free from them. While I believe
the remaining 9 are also free of side effects, it's easier to just convert
those 9 to a new macro (which will not be compiled out when NDEBUG is
defined), and instruct any future assertion writers to likewise switch to
that alternative macro if they have a slightly more involved assert()
invocation.

See
https://github.com/newren/git/actions/runs/13845548634/job/38743076293#step:4:1938
for an example of it running in CI and reporting possibly problematic
assertions (sample output also included in the commit message of the middle
commit in this series if you don't have access to view the link; I'm not
sure what the rules on that are).

Elijah Newren (3):
  git-compat-util: introduce BUG_IF_NOT() macro
  ci: add build checking for side-effects in assert() calls
  treewide: replace assert() with BUG_IF_NOT() in special cases

 Makefile                      |  4 ++++
 ci/check-unsafe-assertions.sh | 18 ++++++++++++++++++
 ci/run-static-analysis.sh     |  2 ++
 diffcore-rename.c             |  2 +-
 git-compat-util.h             |  7 +++++++
 merge-ort.c                   |  4 ++--
 merge-recursive.c             |  2 +-
 object-file.c                 |  2 +-
 parallel-checkout.c           |  2 +-
 scalar.c                      |  4 ++--
 sequencer.c                   |  2 +-
 11 files changed, 40 insertions(+), 9 deletions(-)
 create mode 100755 ci/check-unsafe-assertions.sh

base-commit: 4b68faf6b93311254efad80e554780e372deb42f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1881%2Fnewren%2Fassertion-side-effects-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1881/newren/assertion-side-effects-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1881

Range-diff vs v1:

 1:  109060ccb86 = 1:  109060ccb86 git-compat-util: introduce BUG_IF_NOT() macro
 2:  80dcc2ba3aa ! 2:  58cb8f6a160 ci: add build checking for side-effects in assert() calls
     @@ Commit message
          We have roughly 566 assert() calls in our codebase (my grep might have
          picked up things that aren't actually assert() calls, but most appeared
          to be).  All but 9 of them can be determined by gcc to be free of side
     -    effects with a clever redefine of assert().  The current 9 appear to be
     -    free of side effects to me as well, but are too complicated for a
     -    compiler/linker to figure that since each assertion involves some kind
     -    of function call.  Add a CI job which will find and report these
     -    possibly problematic assertions, and have the job suggest to the user
     -    that they replace these with BUG_IF_NOT() calls.
     +    effects with a clever redefine of assert() provided by Bruno De Fraine
     +    (from
     +    https://stackoverflow.com/questions/10593492/catching-assert-with-side-effects),
     +    who upon request has graciously placed his two-liner into the public
     +    domain without warranty of any kind.  The current 9 assert() calls
     +    flagged by this clever redefinition of assert() appear to me to be free
     +    of side effects as well, but are too complicated for a compiler/linker
     +    to figure that since each assertion involves some kind of function call.
     +    Add a CI job which will find and report these possibly problematic
     +    assertions, and have the job suggest to the user that they replace these
     +    with BUG_IF_NOT() calls.
      
          Example output from running:
      
     @@ Commit message
          run, subsequent runs will show (some of) the ones that remain, allowing
          you to iteratively remove them all.
      
     +    Helped-by: Bruno De Fraine <[email-masked]>
          Signed-off-by: Elijah Newren <[email-masked]>
      
       ## Makefile ##
 3:  4c668039bb7 = 3:  20c763f2951 treewide: replace assert() with BUG_IF_NOT() in special cases

-- 
gitgitgadget