Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 13/10/2023 10:25, Julien Grall wrote:
> Hi Nicola,
>
> On 12/10/2023 16:28, Nicola Vetrini wrote:
> > The purpose of this macro is to encapsulate the well-known expression
> > 'x & -x', that in 2's complement architectures on unsigned integers 
> > will
> > give 2^ffs(x), where ffs(x) is the position of the lowest set bit in 
> > x.
> In the commit message it is clear that the macro will return the
> lowest set bit. But...
>
> > A deviation for ECLAIR is also introduced.
> >
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> > Changes in v2:
> > - rename to LOWEST_BIT
> > ---
> >   automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++
> >   xen/include/xen/macros.h                         | 6 ++++--
> >   2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > index d8170106b449..b8e1155ee49d 100644
> > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > @@ -274,6 +274,12 @@ still non-negative."
> >   -config=MC3R1.R10.1,etypes+={safe, 
> > "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", 
> > "dst_type(ebool||boolean)"}
> >   -doc_end
> >
> > +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to 
> > obtain the value
> > +2^ffs(x) for unsigned integers on two's complement architectures
> > +(all the architectures supported by Xen satisfy this requirement)."
> > +-config=MC3R1.R10.1,reports+={safe, 
> > "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
> > +-doc_end
> > +
> >   ### Set 3 ###
> >
> >   #
> > diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h
> > index d0caae7db298..af47179d1056 100644
> > --- a/xen/include/xen/macros.h
> > +++ b/xen/include/xen/macros.h
> > @@ -8,8 +8,10 @@
> >   #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
> >   #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> >
> > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> > +#define LOWEST_BIT(x) ((x) & -(x))
> ... this is not reflected in the name of the macro. So it is not
> obvious if it will return the lowest bit set or clear.
>
> Can you at least add a comment on top explaining what it returns?
> Something like:
>
> /* Return the lowest bit set */
>
> Cheers,
No problem

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)