Re: [PATCH 30/30] tools/testing/vma: output compared expression on ASSERT_[EQ, NE]()

On Fri, Jul 10, 2026 at 04:48:58PM -0400, Gregory Price wrote:
> On Mon, Jun 29, 2026 at 01:23:41PM +0100, Lorenzo Stoakes wrote:
> > -#define ASSERT_TRUE(_expr)						\
> > -	do {								\
> > -		if (!(_expr)) {						\
> > -			fprintf(stderr,					\
> > -				"Assert FAILED at %s:%d:%s(): %s is FALSE.\n", \
> > -				__FILE__, __LINE__, __FUNCTION__, #_expr); \
> > -			return false;					\
> > -		}							\
> > +#define __ASSERT_TRUE(_expr, _fmt, ...)					   \
> > +	do {								   \
> > +		if (!(_expr)) {						   \
> > +			fprintf(stderr,					   \
> > +				"Assert FAILED at %s:%d:%s(): %s is FALSE" \
> > +				_fmt ".\n",				   \
> > +				__FILE__, __LINE__, __FUNCTION__, #_expr   \
> > +				__VA_OPT__(,) __VA_ARGS__);		   \
> > +			return false;					   \
> > +		}							   \
> >  	} while (0)
> >
> > +#define __TO_SCALAR(x)	((unsigned long long)(uintptr_t)(x))
> > +
> > +#define ASSERT_TRUE(_expr) __ASSERT_TRUE(_expr, "")
>
> Mmmmm... macro madness.... I don't think this is what you want.
>
> I think you end up double-running the expression in the failure branch.
>
>   ASSERT_EQ(cleanup_mm(&mm, &vmi), 2)
>
> run through the preprocessor expands to:
>
>   do {
>       if (!( (cleanup_mm(&mm, &vmi)) == (2) )) {
>               **** first run ****
>
>           fprintf(stderr,
>               "Assert FAILED at %s:%d:%s(): %s is FALSE" " (0x%llx != 0x%llx)" ".\n",
>               "merge.c", 645, __FUNCTION__,
>               "(cleanup_mm(&mm, &vmi)) == (2)",
>               ((unsigned long long)(uintptr_t)(cleanup_mm(&mm, &vmi))),
>                                                **** second run ****
>
>               ((unsigned long long)(uintptr_t)(2)));
>           return false;
>       }
>   } while (0);
>
>
> A bunch of existing ASSERT callers mutate state, so there's no guarantee
> the printed value matches teh actual test value.
>
> I think you want something like:
>
> #define ASSERT_EQ(_val1, _val2) do {	\
> 	__auto_type _v1 = (_val1);	\
> 	__auto_type _v2 = (_val2);	\
> 	__ASSERT_TRUE(_v1 == _v2, " (0x%llx != 0x%llx)",	\
> 		__TO_SCALAR(_v1), __TO_SCALAR(_v2));	\
> } while (0)
>
> which expands to:
>
>   do {
>       __auto_type _v1 = (cleanup_mm(&mm, &vmi));
>       __auto_type _v2 = (2);
>       do {
>           if (!(_v1 == _v2)) {
>               fprintf(stderr, "...FALSE (0x%llx != 0x%llx).\n",
>                       "merge.c", 645, __FUNCTION__, "_v1 == _v2",
>                       ((unsigned long long)(uintptr_t)(_v1)),
>                       ((unsigned long long)(uintptr_t)(_v2)));
>               return false;
>           }
>       } while (0);
>   } while (0);
>
> ~Gregory

It's funny you should mention that... fixed in v2.

But ugh sorry that you reviewed this while I was also fixing this up (Claude
reported it also), your review's very appreciated :>)

Cheers, Lorenzo