Re: [PATCH 1/7] xen/bitops: Cleanup ahead of rearrangements

From: Shawn Anastasio <sanastasio@raptorengineering.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Cc: "Jan Beulich" <JBeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Michal Orzel" <michal.orzel@amd.com>,
	"Oleksii Kurochko" <oleksii.kurochko@gmail.com>,
	"consulting @ bugseng . com" <consulting@bugseng.com>,
	"Simone Ballarin" <simone.ballarin@bugseng.com>,
	"Federico Serafini" <federico.serafini@bugseng.com>,
	"Nicola Vetrini" <nicola.vetrini@bugseng.com>
Subject: Re: [PATCH 1/7] xen/bitops: Cleanup ahead of rearrangements
Date: Wed, 13 Mar 2024 13:39:57 -0500	[thread overview]
Message-ID: <642f7eb8-3a44-4462-a199-4ee4bd24887e@raptorengineering.com> (raw)
In-Reply-To: <20240313172716.2325427-2-andrew.cooper3@citrix.com>

Hi Andrew,

On 3/13/24 12:27 PM, Andrew Cooper wrote:
> diff --git a/xen/common/bitops.c b/xen/common/bitops.c
> new file mode 100644
> index 000000000000..4c07191b4030
> --- /dev/null
> +++ b/xen/common/bitops.c
> @@ -0,0 +1,41 @@
> +#include <xen/bitops.h>
> +#include <xen/bug.h>
> +#include <xen/init.h>
> +
> +/* Hide a value from the optimiser. */
> +#define HIDE(x) ({ typeof(x) _x = x; asm volatile ( "" : "+r" (_x) ); _x; })
> +
> +/*
> + * Check that fn(val) can be calcuated by the compiler, and that it gives the
> + * expected answer.
> + */
> +#define COMPILE_CHECK(fn, val, res)                                     \
> +    do {                                                                \
> +        if ( fn(val) != res )                                           \
> +            asm (".error \"Compile time check '" STR(fn(val) == res) "' failed\""); \
> +    } while ( 0 )
> +

For improved diagnostics, I think it might make sense to explicitly
check if the expression can be evaluated at compile time and emit a
different error if not. Perhaps something like the following:

#define COMPILE_CHECK(fn, val, res)                                     \
    do {                                                                \
        __typeof__(fn(val)) actual = fn(val);                           \
        if ( !__builtin_constant_p(actual) )                            \
            asm (".error \"Unable to evaluate '" STR(fn(val)) "' at
compile time\"\n"); \
        else if ( actual != res )                                       \
            asm (".error \"Compile time check '" STR(fn(val) == res) "'
failed\""); \
    } while ( 0 )