On 24.05.2024 22:03, Andrew Cooper wrote:
> * Rename __attribute_pure__ to just __pure before it gains users.
> * Introduce __constructor which is going to be used in lib/, and is
> unconditionally cf_check.
> * Identify the areas of xen/bitops.h which are a mess.
> * Introduce xen/boot-check.h as helpers for compile and boot time testing.
> This provides a statement of the ABI, and a confirmation that arch-specific
> implementations behave as expected.
>
> Sadly Clang 7 and older isn't happy with the compile time checks. Skip them,
> and just rely on the runtime checks.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
Further remarks, though:
> ---
> xen/include/xen/bitops.h | 13 ++++++--
> xen/include/xen/boot-check.h | 60 ++++++++++++++++++++++++++++++++++++
> xen/include/xen/compiler.h | 3 +-
> 3 files changed, 72 insertions(+), 4 deletions(-)
> create mode 100644 xen/include/xen/boot-check.h
The bulk of the changes isn't about bitops; it's just that you're intending
to first use it for testing there. The subject prefix therefore is somewhat
misleading.
> --- /dev/null
> +++ b/xen/include/xen/boot-check.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/*
> + * Helpers for boot-time checks of basic logic, including confirming that
> + * examples which should be calculated by the compiler are.
> + */
> +#ifndef XEN_BOOT_CHECK_H
> +#define XEN_BOOT_CHECK_H
> +
> +#include <xen/lib.h>
> +
> +/* Hide a value from the optimiser. */
> +#define HIDE(x) \
> + ({ typeof(x) _x = (x); asm volatile ( "" : "+r" (_x) ); _x; })
In principle this is a macro that could be of use elsewhere. That's also
reflected in its entirely generic name. It therefore feels mis-placed in
this header. Otoh though the use of "+r" is more restricting than truly
necessary: While I'm not sure if "+g" would work, i.e. if that wouldn't
cause issues with literals, pretty surely "+rm" ought to work, removing
the strict requirement for the compiler to put a certain value in a
register.
Assuming you may have reservations against "+g" / "+rm" (and hence the
construct wants keeping here), maybe rename to e.g. BOOT_CHECK_HIDE()?
Alternatively, if generalized, moving to xen/macros.h would seem
appropriate to me.
Finally, plainly as a remark with no request for any change (but
possibly a minor argument against moving to xen/macros.h), this construct
won't, afaict, work if x is of array(-of-const) type. A more specialized
variant may need introducing, should any such use ever appear.
Jan
