[XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
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.
A deviation for ECLAIR is also introduced.
Signed-off-by: Nicola Vetrini
---
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))
+
+#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m))
+#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m))
#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
#define count_args(args...) \
--
2.34.1
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On Thu, 12 Oct 2023, 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.
>
> A deviation for ECLAIR is also introduced.
>
> Signed-off-by: Nicola Vetrini
> ---
> 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
Please also add the same deviation and explanation to
docs/misra/deviations.rst. Other than that, this looks fine.
> ### 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))
> +
> +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m))
> +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m))
>
> #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
> #define count_args(args...) \
> --
> 2.34.1
>
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
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
---
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,
--
Julien Grall
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 13/10/2023 01:31, Stefano Stabellini wrote:
On Thu, 12 Oct 2023, 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.
A deviation for ECLAIR is also introduced.
Signed-off-by: Nicola Vetrini
---
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
Please also add the same deviation and explanation to
docs/misra/deviations.rst. Other than that, this looks fine.
Ok.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
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
---
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)
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 12.10.2023 17:28, Nicola Vetrini wrote:
> --- 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
Why is this added here rather than by ...
> --- 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))
a SAF--safe comment here?
> +#define LOWEST_BIT(x) ((x) & -(x))
Personally I consider the name misleading: I'd expect a macro of this
name to return a bit number, not a mask with a single bit set. No
good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too
long for my taste.
Jan
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 16/10/2023 17:33, Jan Beulich wrote:
On 12.10.2023 17:28, Nicola Vetrini wrote:
--- 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
Why is this added here rather than by ...
--- 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))
a SAF--safe comment here?
One reason is that now that violations only belonging to tool
configurations
and similar are documented in docs/misra/deviations.rst (committed in
Stefano's
branch for-4.19 [1]). Also, there were disagreements on the SAF naming
scheme, and
patches like those would not be accepted at the moment.
+#define LOWEST_BIT(x) ((x) & -(x))
Personally I consider the name misleading: I'd expect a macro of this
name to return a bit number, not a mask with a single bit set. No
good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too
long for my taste.
Jan
[1]
https://gitlab.com/xen-project/people/sstabellini/xen/-/commits/for-4.19
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 16.10.2023 18:17, Nicola Vetrini wrote:
> On 16/10/2023 17:33, Jan Beulich wrote:
>> On 12.10.2023 17:28, Nicola Vetrini wrote:
>>> --- 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
>>
>> Why is this added here rather than by ...
>>
>>> --- 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))
>>
>> a SAF--safe comment here?
>>
>
> One reason is that now that violations only belonging to tool
> configurations
> and similar are documented in docs/misra/deviations.rst (committed in
> Stefano's
> branch for-4.19 [1]).
But tool configuration means every analysis tool needs configuring
separately. That's why the comment tagging scheme was decided to be
preferred, iirc.
> Also, there were disagreements on the SAF naming
> scheme, and
> patches like those would not be accepted at the moment.
Well, that needs resolving. The naming there shouldn't lead to patches
being accepted that later may need redoing.
Jan
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On Mon, 16 Oct 2023, Jan Beulich wrote:
> On 16.10.2023 18:17, Nicola Vetrini wrote:
> > On 16/10/2023 17:33, Jan Beulich wrote:
> >> On 12.10.2023 17:28, Nicola Vetrini wrote:
> >>> --- 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
> >>
> >> Why is this added here rather than by ...
> >>
> >>> --- 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))
> >>
> >> a SAF--safe comment here?
> >>
> >
> > One reason is that now that violations only belonging to tool
> > configurations
> > and similar are documented in docs/misra/deviations.rst (committed in
> > Stefano's
> > branch for-4.19 [1]).
>
> But tool configuration means every analysis tool needs configuring
> separately. That's why the comment tagging scheme was decided to be
> preferred, iirc.
>
> > Also, there were disagreements on the SAF naming
> > scheme, and
> > patches like those would not be accepted at the moment.
>
> Well, that needs resolving. The naming there shouldn't lead to patches
> being accepted that later may need redoing.
I agree
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 16/10/2023 18:30, Jan Beulich wrote:
On 16.10.2023 18:17, Nicola Vetrini wrote:
On 16/10/2023 17:33, Jan Beulich wrote:
On 12.10.2023 17:28, Nicola Vetrini wrote:
--- 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
Why is this added here rather than by ...
--- 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))
a SAF--safe comment here?
One reason is that now that violations only belonging to tool
configurations
and similar are documented in docs/misra/deviations.rst (committed in
Stefano's
branch for-4.19 [1]).
But tool configuration means every analysis tool needs configuring
separately. That's why the comment tagging scheme was decided to be
preferred, iirc.
Also, there were disagreements on the SAF naming
scheme, and
patches like those would not be accepted at the moment.
Well, that needs resolving. The naming there shouldn't lead to patches
being accepted that later may need redoing.
Jan
While this is true, in this case I'm not willing to deviate with a SAF,
given that
some ECLAIR-specific configuration would be needed anyways, given that
I'm deviating a macro definition, rather than the line where it's
actually used (and maybe other tools would need
that as well).
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
+Luca > > > > > --- 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)) > > > > > > > > a SAF--safe comment here? > > > > > > > > > > One reason is that now that violations only belonging to tool > > > configurations > > > and similar are documented in docs/misra/deviations.rst (committed in > > > Stefano's > > > branch for-4.19 [1]). > > > > But tool configuration means every analysis tool needs configuring > > separately. That's why the comment tagging scheme was decided to be > > preferred, iirc. > > > > > Also, there were disagreements on the SAF naming > > > scheme, and > > > patches like those would not be accepted at the moment. > > > > Well, that needs resolving. The naming there shouldn't lead to patches > > being accepted that later may need redoing. > > > > Jan > > While this is true, in this case I'm not willing to deviate with a SAF, given > that > some ECLAIR-specific configuration would be needed anyways, given that I'm > deviating a macro definition, rather than the line where it's actually used > (and maybe other tools would need > that as well). Did I get it right that the problem with using SAF in this case is that it wouldn't be sufficient to add a SAF comment on top of the MACRO definition, but we would need a SAF comment on top of every MACRO invocation? If so, then not just for this MACRO but in general basically we have to use deviations.rst. Luca, do you know what would be the behavior for cppcheck and/or Coverity? I imagine it will be the same and they would also need a deviation at every MACRO invocation, not just the definition?
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 19.10.2023 21:58, Stefano Stabellini wrote: >> --- 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)) > > a SAF--safe comment here? > One reason is that now that violations only belonging to tool configurations and similar are documented in docs/misra/deviations.rst (committed in Stefano's branch for-4.19 [1]). >>> >>> But tool configuration means every analysis tool needs configuring >>> separately. That's why the comment tagging scheme was decided to be >>> preferred, iirc. >>> Also, there were disagreements on the SAF naming scheme, and patches like those would not be accepted at the moment. >>> >>> Well, that needs resolving. The naming there shouldn't lead to patches >>> being accepted that later may need redoing. >>> >>> Jan >> >> While this is true, in this case I'm not willing to deviate with a SAF, given >> that >> some ECLAIR-specific configuration would be needed anyways, given that I'm >> deviating a macro definition, rather than the line where it's actually used >> (and maybe other tools would need >> that as well). > > Did I get it right that the problem with using SAF in this case is that > it wouldn't be sufficient to add a SAF comment on top of the MACRO > definition, but we would need a SAF comment on top of every MACRO > invocation? > > If so, then not just for this MACRO but in general basically we have to > use deviations.rst. That would be pretty sad. Jan > Luca, do you know what would be the behavior for cppcheck and/or > Coverity? I imagine it will be the same and they would also need a > deviation at every MACRO invocation, not just the definition?
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 20/10/2023 08:00, Jan Beulich wrote: On 19.10.2023 21:58, Stefano Stabellini wrote: --- 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)) a SAF--safe comment here? One reason is that now that violations only belonging to tool configurations and similar are documented in docs/misra/deviations.rst (committed in Stefano's branch for-4.19 [1]). But tool configuration means every analysis tool needs configuring separately. That's why the comment tagging scheme was decided to be preferred, iirc. Also, there were disagreements on the SAF naming scheme, and patches like those would not be accepted at the moment. Well, that needs resolving. The naming there shouldn't lead to patches being accepted that later may need redoing. Jan While this is true, in this case I'm not willing to deviate with a SAF, given that some ECLAIR-specific configuration would be needed anyways, given that I'm deviating a macro definition, rather than the line where it's actually used (and maybe other tools would need that as well). Did I get it right that the problem with using SAF in this case is that it wouldn't be sufficient to add a SAF comment on top of the MACRO definition, but we would need a SAF comment on top of every MACRO invocation? If so, then not just for this MACRO but in general basically we have to use deviations.rst. That would be pretty sad. Jan Local deviation comments are for local deviations; deviating patterns is a tool configuration. Luca, do you know what would be the behavior for cppcheck and/or Coverity? I imagine it will be the same and they would also need a deviation at every MACRO invocation, not just the definition? -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
> On 19 Oct 2023, at 20:58, Stefano Stabellini wrote: > > +Luca > >> --- 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)) > > a SAF--safe comment here? > One reason is that now that violations only belonging to tool configurations and similar are documented in docs/misra/deviations.rst (committed in Stefano's branch for-4.19 [1]). >>> >>> But tool configuration means every analysis tool needs configuring >>> separately. That's why the comment tagging scheme was decided to be >>> preferred, iirc. >>> Also, there were disagreements on the SAF naming scheme, and patches like those would not be accepted at the moment. >>> >>> Well, that needs resolving. The naming there shouldn't lead to patches >>> being accepted that later may need redoing. >>> >>> Jan >> >> While this is true, in this case I'm not willing to deviate with a SAF, given >> that >> some ECLAIR-specific configuration would be needed anyways, given that I'm >> deviating a macro definition, rather than the line where it's actually used >> (and maybe other tools would need >> that as well). > > Did I get it right that the problem with using SAF in this case is that > it wouldn't be sufficient to add a SAF comment on top of the MACRO > definition, but we would need a SAF comment on top of every MACRO > invocation? > > If so, then not just for this MACRO but in general basically we have to > use deviations.rst. > > Luca, do you know what would be the behavior for cppcheck and/or > Coverity? I imagine it will be the same and they would also need a > deviation at every MACRO invocation, not just the definition? Seems that cppcheck reports at the macro definition, instead Coverity reports at the macro invocation.
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On 20.10.2023 12:40, Nicola Vetrini wrote: > On 20/10/2023 08:00, Jan Beulich wrote: >> On 19.10.2023 21:58, Stefano Stabellini wrote: --- 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)) >>> >>> a SAF--safe comment here? >>> >> >> One reason is that now that violations only belonging to tool >> configurations >> and similar are documented in docs/misra/deviations.rst (committed >> in >> Stefano's >> branch for-4.19 [1]). > > But tool configuration means every analysis tool needs configuring > separately. That's why the comment tagging scheme was decided to be > preferred, iirc. > >> Also, there were disagreements on the SAF naming >> scheme, and >> patches like those would not be accepted at the moment. > > Well, that needs resolving. The naming there shouldn't lead to > patches > being accepted that later may need redoing. > > Jan While this is true, in this case I'm not willing to deviate with a SAF, given that some ECLAIR-specific configuration would be needed anyways, given that I'm deviating a macro definition, rather than the line where it's actually used (and maybe other tools would need that as well). >>> >>> Did I get it right that the problem with using SAF in this case is >>> that >>> it wouldn't be sufficient to add a SAF comment on top of the MACRO >>> definition, but we would need a SAF comment on top of every MACRO >>> invocation? >>> >>> If so, then not just for this MACRO but in general basically we have >>> to >>> use deviations.rst. >> >> That would be pretty sad. > > Local deviation comments are for local deviations; deviating patterns is > a tool configuration. That's orthogonal. A deviating comment on a macro definition, when it is about an aspect that's meaningful only after the macro is expanded (i.e. not violating some rule concerning macro definitions only), would be quite helpful to limit the number of such comments that need sprinkling across the code base. Jan
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
On Fri, 20 Oct 2023, Luca Fancellu wrote: > > On 19 Oct 2023, at 20:58, Stefano Stabellini wrote: > > +Luca > > > >> --- 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)) > > > > a SAF--safe comment here? > > > > One reason is that now that violations only belonging to tool > configurations > and similar are documented in docs/misra/deviations.rst (committed in > Stefano's > branch for-4.19 [1]). > >>> > >>> But tool configuration means every analysis tool needs configuring > >>> separately. That's why the comment tagging scheme was decided to be > >>> preferred, iirc. > >>> > Also, there were disagreements on the SAF naming > scheme, and > patches like those would not be accepted at the moment. > >>> > >>> Well, that needs resolving. The naming there shouldn't lead to patches > >>> being accepted that later may need redoing. > >>> > >>> Jan > >> > >> While this is true, in this case I'm not willing to deviate with a SAF, > >> given > >> that > >> some ECLAIR-specific configuration would be needed anyways, given that I'm > >> deviating a macro definition, rather than the line where it's actually used > >> (and maybe other tools would need > >> that as well). > > > > Did I get it right that the problem with using SAF in this case is that > > it wouldn't be sufficient to add a SAF comment on top of the MACRO > > definition, but we would need a SAF comment on top of every MACRO > > invocation? > > > > If so, then not just for this MACRO but in general basically we have to > > use deviations.rst. > > > > Luca, do you know what would be the behavior for cppcheck and/or > > Coverity? I imagine it will be the same and they would also need a > > deviation at every MACRO invocation, not just the definition? > > Seems that cppcheck reports at the macro definition, instead Coverity reports > at the macro invocation. Does that mean that a /* SAF-xx-safe */ comment would work at the MACRO definition for cppcheck but not for Coverity? If so, then I wonder if cppcheck's behavior is what we would like to have, but from a code compliance point of view it is the least reliable, so that's the reason why both Coverity and ECLAIR don't implement it. In terms of correctness of the implementation we know cppcheck is lagging a bit behind...
Re: [XEN PATCH][for-next][for-4.19 v2 1/8] xen/include: add macro LOWEST_BIT
> On 20 Oct 2023, at 19:07, Stefano Stabellini wrote: > > On Fri, 20 Oct 2023, Luca Fancellu wrote: >>> On 19 Oct 2023, at 20:58, Stefano Stabellini wrote: >>> +Luca >>> --- 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)) >>> >>> a SAF--safe comment here? >>> >> >> One reason is that now that violations only belonging to tool >> configurations >> and similar are documented in docs/misra/deviations.rst (committed in >> Stefano's >> branch for-4.19 [1]). > > But tool configuration means every analysis tool needs configuring > separately. That's why the comment tagging scheme was decided to be > preferred, iirc. > >> Also, there were disagreements on the SAF naming >> scheme, and >> patches like those would not be accepted at the moment. > > Well, that needs resolving. The naming there shouldn't lead to patches > being accepted that later may need redoing. > > Jan While this is true, in this case I'm not willing to deviate with a SAF, given that some ECLAIR-specific configuration would be needed anyways, given that I'm deviating a macro definition, rather than the line where it's actually used (and maybe other tools would need that as well). >>> >>> Did I get it right that the problem with using SAF in this case is that >>> it wouldn't be sufficient to add a SAF comment on top of the MACRO >>> definition, but we would need a SAF comment on top of every MACRO >>> invocation? >>> >>> If so, then not just for this MACRO but in general basically we have to >>> use deviations.rst. >>> >>> Luca, do you know what would be the behavior for cppcheck and/or >>> Coverity? I imagine it will be the same and they would also need a >>> deviation at every MACRO invocation, not just the definition? >> >> Seems that cppcheck reports at the macro definition, instead Coverity reports >> at the macro invocation. > > Does that mean that a /* SAF-xx-safe */ comment would work at the MACRO > definition for cppcheck but not for Coverity? > > If so, then I wonder if cppcheck's behavior is what we would like to > have, but from a code compliance point of view it is the least reliable, > so that's the reason why both Coverity and ECLAIR don't implement it. In > terms of correctness of the implementation we know cppcheck is lagging a > bit behind... Yes, I’m starting to think that if we want to deviate a large number of macro usage, We should come up with something like declaring the macro somewhere and adding the in-code comment automatically by the script before the analysis... But we need to see how feasible it is.
