[v3] misra: address violation of MISRA C Rule 10.1

[XEN PATCH v3] misra: address violation of MISRA C Rule 10.1

Posted by Dmytro Prokopchuk1 3 months, 2 weeks ago

Rule 10.1: Operands shall not be of an
inappropriate essential type

The following are non-compliant:
- boolean used as a numeric value.

The result of the '__isleap' macro is a boolean.
Use a ternary operator to replace it with a numeric value.

The result of 'NOW() > timeout' is a boolean,
which is compared to a numeric value. Fix this.
Regression was introdiced by commit:
be7f047e08 (xen/arm: smmuv3: Replace linux functions with xen functions.)

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Changes since v2:
- improve the wording
Link to v2: https://patchew.org/Xen/41538b6b19811eb74c183051d3e7a4fd216404e6.1752232902.git.dmytro._5Fprokopchuk1@epam.com/
Link to the deviation of an unary minus: https://patchew.org/Xen/7e6263a15c71aafc41fe72cecd1f15c3ce8846f2.1752492180.git.dmytro._5Fprokopchuk1@epam.com/

Jan, regarding that:
If an expression is needed here, I'd suggest to use !!, as we have in
(luckily decreasing) number of places elsewhere. Personally I don't
understand though why a boolean cannot be used as an array index.

The '!!' isn't a solution here, we'll have other violation:
`!' logical negation operator has essential type boolean and standard type `int'
(https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/people/dimaprkp4k/xen/ECLAIR_normal/deviate_10.1_rule/ARM64/10674114852/PROJECT.ecd;/by_service/MC3A2.R10.1.html#{%22select%22:true,%22selection%22:{%22hiddenAreaKinds%22:[],%22hiddenSubareaKinds%22:[],%22show%22:false,%22selector%22:{%22enabled%22:true,%22negated%22:true,%22kind%22:0,%22domain%22:%22kind%22,%22inputs%22:[{%22enabled%22:true,%22text%22:%22violation%22}]}}})

Well, in our case boolean can be used as an array index.
But index value is limited: 0 or 1.
I guess MISRA wants to predict such errors related to index limitations.
And I think fixing the code is easier here, instead of writing a deviation.

---
 xen/common/time.c                     | 2 +-
 xen/drivers/passthrough/arm/smmu-v3.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/time.c b/xen/common/time.c
index 92f7b72464..980dddee28 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -84,7 +84,7 @@ struct tm gmtime(unsigned long t)
     }
     tbuf.tm_year = y - 1900;
     tbuf.tm_yday = days;
-    ip = (const unsigned short int *)__mon_lengths[__isleap(y)];
+    ip = (const unsigned short int *)__mon_lengths[__isleap(y) ? 1 : 0];
     for ( y = 0; days >= ip[y]; ++y )
         days -= ip[y];
     tbuf.tm_mon = y;
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index df16235057..4058b18f2c 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -315,7 +315,7 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool sync, bool wfe)
 
 	while (queue_sync_cons_in(q),
 	      (sync ? !queue_empty(&q->llq) : queue_full(&q->llq))) {
-		if ((NOW() > timeout) > 0)
+		if (NOW() > timeout)
 			return -ETIMEDOUT;
 
 		if (wfe) {
-- 
2.43.0

Re: [XEN PATCH v3] misra: address violation of MISRA C Rule 10.1

Posted by Jan Beulich 3 months, 2 weeks ago

On 14.07.2025 15:26, Dmytro Prokopchuk1 wrote:
> --- a/xen/common/time.c
> +++ b/xen/common/time.c
> @@ -84,7 +84,7 @@ struct tm gmtime(unsigned long t)
>      }
>      tbuf.tm_year = y - 1900;
>      tbuf.tm_yday = days;
> -    ip = (const unsigned short int *)__mon_lengths[__isleap(y)];
> +    ip = (const unsigned short int *)__mon_lengths[__isleap(y) ? 1 : 0];

Oh, and: This cast is one of the more dangerous ones, and afaict it's entirely
pointless. When touching this line, I think the cast would want removing at the
same time.

Jan

Re: [XEN PATCH v3] misra: address violation of MISRA C Rule 10.1

Posted by Jan Beulich 3 months, 2 weeks ago

On 14.07.2025 15:26, Dmytro Prokopchuk1 wrote:
> Rule 10.1: Operands shall not be of an
> inappropriate essential type
> 
> The following are non-compliant:
> - boolean used as a numeric value.
> 
> The result of the '__isleap' macro is a boolean.
> Use a ternary operator to replace it with a numeric value.
> 
> The result of 'NOW() > timeout' is a boolean,
> which is compared to a numeric value. Fix this.
> Regression was introdiced by commit:
> be7f047e08 (xen/arm: smmuv3: Replace linux functions with xen functions.)
> 
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
> Changes since v2:
> - improve the wording
> Link to v2: https://patchew.org/Xen/41538b6b19811eb74c183051d3e7a4fd216404e6.1752232902.git.dmytro._5Fprokopchuk1@epam.com/
> Link to the deviation of an unary minus: https://patchew.org/Xen/7e6263a15c71aafc41fe72cecd1f15c3ce8846f2.1752492180.git.dmytro._5Fprokopchuk1@epam.com/
> 
> Jan, regarding that:
> If an expression is needed here, I'd suggest to use !!, as we have in
> (luckily decreasing) number of places elsewhere. Personally I don't
> understand though why a boolean cannot be used as an array index.
> 
> The '!!' isn't a solution here, we'll have other violation:
> `!' logical negation operator has essential type boolean and standard type `int'
> (https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/people/dimaprkp4k/xen/ECLAIR_normal/deviate_10.1_rule/ARM64/10674114852/PROJECT.ecd;/by_service/MC3A2.R10.1.html#{%22select%22:true,%22selection%22:{%22hiddenAreaKinds%22:[],%22hiddenSubareaKinds%22:[],%22show%22:false,%22selector%22:{%22enabled%22:true,%22negated%22:true,%22kind%22:0,%22domain%22:%22kind%22,%22inputs%22:[{%22enabled%22:true,%22text%22:%22violation%22}]}}})

And that doesn't fall under any other of the deviations we already have?
__isleap() is used in another boolean context after all, and apparently
there's no issue there.

> Well, in our case boolean can be used as an array index.
> But index value is limited: 0 or 1.
> I guess MISRA wants to predict such errors related to index limitations.
> And I think fixing the code is easier here, instead of writing a deviation.

It may be easier indeed, but ...

> --- a/xen/common/time.c
> +++ b/xen/common/time.c
> @@ -84,7 +84,7 @@ struct tm gmtime(unsigned long t)
>      }
>      tbuf.tm_year = y - 1900;
>      tbuf.tm_yday = days;
> -    ip = (const unsigned short int *)__mon_lengths[__isleap(y)];
> +    ip = (const unsigned short int *)__mon_lengths[__isleap(y) ? 1 : 0];

... especially as long as it's un-annotated, I'd be very likely to submit
a patch to undo this again, should I ever run across this after having
forgotten about the change here. At least to me, _this_ is the confusing
way to write things.

Once you add a comment though, you can as well leave the code unchanged
and use a SAF comment.

Jan

Re: [XEN PATCH v3] misra: address violation of MISRA C Rule 10.1

Posted by Nicola Vetrini 3 months, 2 weeks ago

On 2025-07-14 15:49, Jan Beulich wrote:
> On 14.07.2025 15:26, Dmytro Prokopchuk1 wrote:
>> Rule 10.1: Operands shall not be of an
>> inappropriate essential type
>> 
>> The following are non-compliant:
>> - boolean used as a numeric value.
>> 
>> The result of the '__isleap' macro is a boolean.
>> Use a ternary operator to replace it with a numeric value.
>> 
>> The result of 'NOW() > timeout' is a boolean,
>> which is compared to a numeric value. Fix this.
>> Regression was introdiced by commit:
>> be7f047e08 (xen/arm: smmuv3: Replace linux functions with xen 
>> functions.)
>> 
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>> ---
>> Changes since v2:
>> - improve the wording
>> Link to v2: 
>> https://patchew.org/Xen/41538b6b19811eb74c183051d3e7a4fd216404e6.1752232902.git.dmytro._5Fprokopchuk1@epam.com/
>> Link to the deviation of an unary minus: 
>> https://patchew.org/Xen/7e6263a15c71aafc41fe72cecd1f15c3ce8846f2.1752492180.git.dmytro._5Fprokopchuk1@epam.com/
>> 
>> Jan, regarding that:
>> If an expression is needed here, I'd suggest to use !!, as we have in
>> (luckily decreasing) number of places elsewhere. Personally I don't
>> understand though why a boolean cannot be used as an array index.
>> 
>> The '!!' isn't a solution here, we'll have other violation:
>> `!' logical negation operator has essential type boolean and standard 
>> type `int'
>> (https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/people/dimaprkp4k/xen/ECLAIR_normal/deviate_10.1_rule/ARM64/10674114852/PROJECT.ecd;/by_service/MC3A2.R10.1.html#{%22select%22:true,%22selection%22:{%22hiddenAreaKinds%22:[],%22hiddenSubareaKinds%22:[],%22show%22:false,%22selector%22:{%22enabled%22:true,%22negated%22:true,%22kind%22:0,%22domain%22:%22kind%22,%22inputs%22:[{%22enabled%22:true,%22text%22:%22violation%22}]}}})
> 
> And that doesn't fall under any other of the deviations we already 
> have?
> __isleap() is used in another boolean context after all, and apparently
> there's no issue there.
> 

Hi Jan,

I double-checked: there is no specific deviation for using a boolean as 
an array subscript.

This is the only problematic use of a macro that returns an essentially 
boolean expr used as an operand to an operator that expects an integer, 
which is the reason of the violation:
xen/common/time.c:#define __isleap(year) \
xen/common/time.c:    while ( days >= (rem = __isleap(y) ? 366 : 365) )
xen/common/time.c:        days += __isleap(y) ? 366 : 365;
xen/common/time.c:    ip = (const unsigned short int 
*)__mon_lengths[__isleap(y)];

Thanks,
  Nicola

>> Well, in our case boolean can be used as an array index.
>> But index value is limited: 0 or 1.
>> I guess MISRA wants to predict such errors related to index 
>> limitations.
>> And I think fixing the code is easier here, instead of writing a 
>> deviation.
> 
> It may be easier indeed, but ...
> 
>> --- a/xen/common/time.c
>> +++ b/xen/common/time.c
>> @@ -84,7 +84,7 @@ struct tm gmtime(unsigned long t)
>>      }
>>      tbuf.tm_year = y - 1900;
>>      tbuf.tm_yday = days;
>> -    ip = (const unsigned short int *)__mon_lengths[__isleap(y)];
>> +    ip = (const unsigned short int *)__mon_lengths[__isleap(y) ? 1 : 
>> 0];
> 
> ... especially as long as it's un-annotated, I'd be very likely to 
> submit
> a patch to undo this again, should I ever run across this after having
> forgotten about the change here. At least to me, _this_ is the 
> confusing
> way to write things.
> 
> Once you add a comment though, you can as well leave the code unchanged
> and use a SAF comment.
> 
> Jan

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: [XEN PATCH v3] misra: address violation of MISRA C Rule 10.1

Posted by Jan Beulich 3 months, 2 weeks ago

On 14.07.2025 16:16, Nicola Vetrini wrote:
> On 2025-07-14 15:49, Jan Beulich wrote:
>> On 14.07.2025 15:26, Dmytro Prokopchuk1 wrote:
>>> Rule 10.1: Operands shall not be of an
>>> inappropriate essential type
>>>
>>> The following are non-compliant:
>>> - boolean used as a numeric value.
>>>
>>> The result of the '__isleap' macro is a boolean.
>>> Use a ternary operator to replace it with a numeric value.
>>>
>>> The result of 'NOW() > timeout' is a boolean,
>>> which is compared to a numeric value. Fix this.
>>> Regression was introdiced by commit:
>>> be7f047e08 (xen/arm: smmuv3: Replace linux functions with xen 
>>> functions.)
>>>
>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
>>> ---
>>> Changes since v2:
>>> - improve the wording
>>> Link to v2: 
>>> https://patchew.org/Xen/41538b6b19811eb74c183051d3e7a4fd216404e6.1752232902.git.dmytro._5Fprokopchuk1@epam.com/
>>> Link to the deviation of an unary minus: 
>>> https://patchew.org/Xen/7e6263a15c71aafc41fe72cecd1f15c3ce8846f2.1752492180.git.dmytro._5Fprokopchuk1@epam.com/
>>>
>>> Jan, regarding that:
>>> If an expression is needed here, I'd suggest to use !!, as we have in
>>> (luckily decreasing) number of places elsewhere. Personally I don't
>>> understand though why a boolean cannot be used as an array index.
>>>
>>> The '!!' isn't a solution here, we'll have other violation:
>>> `!' logical negation operator has essential type boolean and standard 
>>> type `int'
>>> (https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/people/dimaprkp4k/xen/ECLAIR_normal/deviate_10.1_rule/ARM64/10674114852/PROJECT.ecd;/by_service/MC3A2.R10.1.html#{%22select%22:true,%22selection%22:{%22hiddenAreaKinds%22:[],%22hiddenSubareaKinds%22:[],%22show%22:false,%22selector%22:{%22enabled%22:true,%22negated%22:true,%22kind%22:0,%22domain%22:%22kind%22,%22inputs%22:[{%22enabled%22:true,%22text%22:%22violation%22}]}}})
>>
>> And that doesn't fall under any other of the deviations we already 
>> have?
>> __isleap() is used in another boolean context after all, and apparently
>> there's no issue there.
> 
> I double-checked: there is no specific deviation for using a boolean as 
> an array subscript.
> 
> This is the only problematic use of a macro that returns an essentially 
> boolean expr used as an operand to an operator that expects an integer, 
> which is the reason of the violation:
> xen/common/time.c:#define __isleap(year) \
> xen/common/time.c:    while ( days >= (rem = __isleap(y) ? 366 : 365) )
> xen/common/time.c:        days += __isleap(y) ? 366 : 365;
> xen/common/time.c:    ip = (const unsigned short int 
> *)__mon_lengths[__isleap(y)];

Oh, I see - the !! simply doesn't alter the situation.

Jan