Re: [PATCH 2/3] ci: add build checking for side-effects in assert() calls

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2025, 01:06 UTC
Message-ID: <xmqqy0x8l8tu.fsf@gitster.g>
In-Reply-To: <80dcc2ba3aa0ef72abe18f8525d571ea39ac6382.1741911652.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 9 quoted lines

> +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?

Show 13 quoted lines

> diff --git a/git-compat-util.h b/git-compat-util.h
> index c3415ad7e0a..0aefd763751 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1584,4 +1584,10 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)
>  	((uintptr_t)&(ptr)->member - (uintptr_t)(ptr))
>  #endif /* !__GNUC__ */
>  
> +#ifdef CHECK_ASSERTION_SIDE_EFFECTS
> +#undef assert
> +extern int not_supposed_to_survive;
> +#define assert(expr) ((void)(not_supposed_to_survive || (expr)))
> +#endif /* CHECK_ASSERTION_SIDE_EFFECTS */

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.