Re: [PATCH v2 06/13] xen/bitops: Implement ffs() in common logic

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/05/2024 9:34 am, Andrew Cooper wrote:
> On 31/05/2024 7:56 am, Nicola Vetrini wrote:
>> On 2024-05-31 03:14, Stefano Stabellini wrote:
>>> On Fri, 24 May 2024, Andrew Cooper wrote:
>>>> Perform constant-folding unconditionally, rather than having it
>>>> implemented
>>>> inconsistency between architectures.
>>>>
>>>> Confirm the expected behaviour with compile time and boot time tests.
>>>>
>>>> For non-constant inputs, use arch_ffs() if provided but fall back to
>>>> generic_ffsl() if not.  In particular, RISC-V doesn't have a builtin
>>>> that
>>>> works in all configurations.
>>>>
>>>> For x86, rename ffs() to arch_ffs() and adjust the prototype.
>>>>
>>>> For PPC, __builtin_ctz() is 1/3 of the size of size of the transform to
>>>> generic_fls().  Drop the definition entirely.  ARM too benefits in
>>>> the general
>>>> case by using __builtin_ctz(), but less dramatically because it using
>>>> optimised asm().
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> This patch made me realize that we should add __builtin_ctz,
>>> __builtin_constant_p and always_inline to
>>> docs/misra/C-language-toolchain.rst as they don't seem to be currently
>>> documented and they are not part of the C standard
>>>
>>> Patch welcome :-)
>>>
>> I can send a patch for the builtins.
> That's very kind of you.
>
> In total by the end of this series, we've got __builtin_constant_p() 
> (definitely used elsewhere already), and __builtin_{ffs,ctz,clz}{,l}() 
> (3x primitives, 2x input types).
>
> If we're going for a list of the primitive operations, lets add
> __builtin_popcnt{,l}() too right away, because if it weren't for 4.19
> code freeze, I'd have cleaned up the hweight() helpers too.

Oh, and it's worth noting that __builtin_{ctz,clz}{,l}() have explicit
UB if given an input of 0.  (Sadly, even on architectures where the
underlying instruction emitted is safe with a 0 input. [0])

This is why every patch in the series using them checks for nonzero input.

UBSAN (with an adequate compiler) will instrument this, and Xen has
__ubsan_handle_invalid_builtin() to diagnose these.

~Andrew

[0] It turns out that Clang has a 2-argument form of the builtin with
the second being the "value forwarded" in case the first is 0.  I've not
investigated whether GCC has the same.