| Message ID | c9ff72160539cda49e726ac6ee1486be0d645180.1688032865.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix violations of MISRA C:2012 Rule 3.1 | expand |
> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: > > In the files modified by this patch there are a few occurrences of nested '//' > character sequences inside C-style comment blocks, which violate Rule 3.1. > The patch aims to resolve those by removing the nested comments. > > In the file 'xen/common/xmalloc_tlsf.c' the comment has been replaces by Typo: s/replaces/replaced/ > an ASSERT, to ensure that the invariant in the comment is enforced. > > In the file 'xen/include/xen/atomic.h' the nested comment has been removed, > since the code sample is already explained by the preceding comment. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Same as patch 2, missing “---" > Changes: > - Resending the patch with the right maintainers in CC. > Changes in V2: > - Split the patch into a series and reworked the fix; > - Apply the fix to the arm32 `flushtlb.h' file, for consistency. > Changes in V3: > - Replaced commmented-out 'if' with an ASSERT( *fl >= 0 ). > --- > xen/common/xmalloc_tlsf.c | 4 +--- > xen/include/xen/atomic.h | 2 +- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c > index 75bdf18c4e..95affcc571 100644 > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) > *fl = flsl(*r) - 1; > *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; > *fl -= FLI_OFFSET; > - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > - *fl = *sl = 0; > - */ > + ASSERT( *fl >= 0 ); I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it with spaces before and after the condition (like our if conditions) so I think they can be dropped. > *r &= ~t; > } > } > diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h > index 529213ebbb..fa750a18ae 100644 > --- a/xen/include/xen/atomic.h > +++ b/xen/include/xen/atomic.h > @@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i); > * int old = atomic_read(&v); > * int new = old + 1; > * if ( likely(old == atomic_cmpxchg(&v, old, new)) ) > - * break; // success! > + * break; > * } > */ > static inline int atomic_cmpxchg(atomic_t *v, int old, int new); > -- > 2.34.1 > > With the comments addressed: Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
On Thu, 29 Jun 2023, Luca Fancellu wrote: > > On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: > > > > In the files modified by this patch there are a few occurrences of nested '//' > > character sequences inside C-style comment blocks, which violate Rule 3.1. > > The patch aims to resolve those by removing the nested comments. > > > > In the file 'xen/common/xmalloc_tlsf.c' the comment has been replaces by > > Typo: s/replaces/replaced/ > > > an ASSERT, to ensure that the invariant in the comment is enforced. > > > > In the file 'xen/include/xen/atomic.h' the nested comment has been removed, > > since the code sample is already explained by the preceding comment. > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > Same as patch 2, missing “---" > > > Changes: > > - Resending the patch with the right maintainers in CC. > > Changes in V2: > > - Split the patch into a series and reworked the fix; > > - Apply the fix to the arm32 `flushtlb.h' file, for consistency. > > Changes in V3: > > - Replaced commmented-out 'if' with an ASSERT( *fl >= 0 ). > > --- > > xen/common/xmalloc_tlsf.c | 4 +--- > > xen/include/xen/atomic.h | 2 +- > > 2 files changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c > > index 75bdf18c4e..95affcc571 100644 > > --- a/xen/common/xmalloc_tlsf.c > > +++ b/xen/common/xmalloc_tlsf.c > > @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) > > *fl = flsl(*r) - 1; > > *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; > > *fl -= FLI_OFFSET; > > - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > > - *fl = *sl = 0; > > - */ > > + ASSERT( *fl >= 0 ); > > I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it with spaces > before and after the condition (like our if conditions) so I think they can be dropped. Yes, that's right. I am OK with this patch but I think we should wait for Jan's ack to be sure. An alternative that I feel more comfortable in Acking myself because it doesn't change the semantics of this code would be to change the 3 lines of code above with this: /* * ; FL will be always >0! * if ((*fl -= FLI_OFFSET) < 0) * fl = *sl = 0; */
> On 29 Jun 2023, at 20:20, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 29 Jun 2023, Luca Fancellu wrote: >>> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>> >>> In the files modified by this patch there are a few occurrences of nested '//' >>> character sequences inside C-style comment blocks, which violate Rule 3.1. >>> The patch aims to resolve those by removing the nested comments. >>> >>> In the file 'xen/common/xmalloc_tlsf.c' the comment has been replaces by >> >> Typo: s/replaces/replaced/ >> >>> an ASSERT, to ensure that the invariant in the comment is enforced. >>> >>> In the file 'xen/include/xen/atomic.h' the nested comment has been removed, >>> since the code sample is already explained by the preceding comment. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> >> Same as patch 2, missing “---" >> >>> Changes: >>> - Resending the patch with the right maintainers in CC. >>> Changes in V2: >>> - Split the patch into a series and reworked the fix; >>> - Apply the fix to the arm32 `flushtlb.h' file, for consistency. >>> Changes in V3: >>> - Replaced commmented-out 'if' with an ASSERT( *fl >= 0 ). >>> --- >>> xen/common/xmalloc_tlsf.c | 4 +--- >>> xen/include/xen/atomic.h | 2 +- >>> 2 files changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c >>> index 75bdf18c4e..95affcc571 100644 >>> --- a/xen/common/xmalloc_tlsf.c >>> +++ b/xen/common/xmalloc_tlsf.c >>> @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >>> *fl = flsl(*r) - 1; >>> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >>> *fl -= FLI_OFFSET; >>> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>> - *fl = *sl = 0; >>> - */ >>> + ASSERT( *fl >= 0 ); >> >> I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it with spaces >> before and after the condition (like our if conditions) so I think they can be dropped. > > Yes, that's right. I am OK with this patch but I think we should wait > for Jan's ack to be sure. > > An alternative that I feel more comfortable in Acking myself because it > doesn't change the semantics of this code would be to change the 3 lines > of code above with this: > > /* > * ; FL will be always >0! > * if ((*fl -= FLI_OFFSET) < 0) > * fl = *sl = 0; > */ Hi Stefano, I think we would have a violation for the Directive 4.4 (Sections of code should not be commented out) in this case, however it’s not listed in rules.rst and I don’t remember where to look to know if it was taken into consideration for the adoption in Xen in the “tailoring” phase.
On 29.06.2023 21:20, Stefano Stabellini wrote: > On Thu, 29 Jun 2023, Luca Fancellu wrote: >>> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>> --- a/xen/common/xmalloc_tlsf.c >>> +++ b/xen/common/xmalloc_tlsf.c >>> @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >>> *fl = flsl(*r) - 1; >>> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >>> *fl -= FLI_OFFSET; >>> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>> - *fl = *sl = 0; >>> - */ >>> + ASSERT( *fl >= 0 ); >> >> I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it with spaces >> before and after the condition (like our if conditions) so I think they can be dropped. > > Yes, that's right. I am OK with this patch but I think we should wait > for Jan's ack to be sure. > > An alternative that I feel more comfortable in Acking myself because it > doesn't change the semantics of this code would be to change the 3 lines > of code above with this: > > /* > * ; FL will be always >0! > * if ((*fl -= FLI_OFFSET) < 0) > * fl = *sl = 0; > */ While I'd be okay with this form, as Luca says it'll get us a different violation, which we ought to avoid. While I was the one to suggest the conversion to ASSERT(), having thought about it yet once more I'm now of the opinion that _any_ transformation of this commented out piece of code needs first understanding what was originally meant. Or alternatively, while converting to #if form, to add a comment making crystal clear that it's simply uncertain what was meant. Jan
On 04/07/23 17:57, Jan Beulich wrote: > On 29.06.2023 21:20, Stefano Stabellini wrote: >> On Thu, 29 Jun 2023, Luca Fancellu wrote: >>>> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>>> --- a/xen/common/xmalloc_tlsf.c >>>> +++ b/xen/common/xmalloc_tlsf.c >>>> @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >>>> *fl = flsl(*r) - 1; >>>> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >>>> *fl -= FLI_OFFSET; >>>> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>>> - *fl = *sl = 0; >>>> - */ >>>> + ASSERT( *fl >= 0 ); >>> >>> I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it with spaces >>> before and after the condition (like our if conditions) so I think they can be dropped. >> >> Yes, that's right. I am OK with this patch but I think we should wait >> for Jan's ack to be sure. >> >> An alternative that I feel more comfortable in Acking myself because it >> doesn't change the semantics of this code would be to change the 3 lines >> of code above with this: >> >> /* >> * ; FL will be always >0! >> * if ((*fl -= FLI_OFFSET) < 0) >> * fl = *sl = 0; >> */ > > While I'd be okay with this form, as Luca says it'll get us a different > violation, which we ought to avoid. While I was the one to suggest the > conversion to ASSERT(), having thought about it yet once more I'm now > of the opinion that _any_ transformation of this commented out piece of > code needs first understanding what was originally meant. Or > alternatively, while converting to #if form, to add a comment making > crystal clear that it's simply uncertain what was meant. > About the violation of D4.4: the Directive was never considered for compliance because it's an advisory directive, and hence considerably less urgent. Having looked a bit at the surrounding code, since *fl and *sl are used as array indices later in 'FIND_SUITABLE_BLOCK', I suggest using something along the lines of "If *fl ever becomes < 0, reset it to a safe value." (either using the form suggested by Stefano or an #if 0). In any case this should become a standalone patch, right?
On 06.07.2023 10:23, Nicola Vetrini wrote: > On 04/07/23 17:57, Jan Beulich wrote: >> On 29.06.2023 21:20, Stefano Stabellini wrote: >>> On Thu, 29 Jun 2023, Luca Fancellu wrote: >>>>> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>>>> --- a/xen/common/xmalloc_tlsf.c >>>>> +++ b/xen/common/xmalloc_tlsf.c >>>>> @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >>>>> *fl = flsl(*r) - 1; >>>>> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >>>>> *fl -= FLI_OFFSET; >>>>> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>>>> - *fl = *sl = 0; >>>>> - */ >>>>> + ASSERT( *fl >= 0 ); >>>> >>>> I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it with spaces >>>> before and after the condition (like our if conditions) so I think they can be dropped. >>> >>> Yes, that's right. I am OK with this patch but I think we should wait >>> for Jan's ack to be sure. >>> >>> An alternative that I feel more comfortable in Acking myself because it >>> doesn't change the semantics of this code would be to change the 3 lines >>> of code above with this: >>> >>> /* >>> * ; FL will be always >0! >>> * if ((*fl -= FLI_OFFSET) < 0) >>> * fl = *sl = 0; >>> */ >> >> While I'd be okay with this form, as Luca says it'll get us a different >> violation, which we ought to avoid. While I was the one to suggest the >> conversion to ASSERT(), having thought about it yet once more I'm now >> of the opinion that _any_ transformation of this commented out piece of >> code needs first understanding what was originally meant. Or >> alternatively, while converting to #if form, to add a comment making >> crystal clear that it's simply uncertain what was meant. >> > > About the violation of D4.4: the Directive was never considered for > compliance because it's an advisory directive, and hence considerably > less urgent. > > Having looked a bit at the surrounding code, since *fl and *sl are used > as array indices later in 'FIND_SUITABLE_BLOCK', I suggest using > something along the lines of "If *fl ever becomes < 0, reset it to a > safe value." (either using the form suggested by Stefano or an #if 0). The main issue I see with any such transformation is how the immediately preceding "*fl -= FLI_OFFSET;" is intended to interact with the commented out code. My gut feeling (but nothing else) says that what was meant would have been #if 1 *fl -= FLI_OFFSET; #else if ((*fl -= FLI_OFFSET) < 0) /* FL will be always >0! */ *fl = *sl = 0; #endif But of course without properly understanding the logic it might also have been *fl -= FLI_OFFSET; #if 0 if ((*fl -= FLI_OFFSET) < 0) /* FL will be always >0! */ *fl = *sl = 0; #endif > In any case this should become a standalone patch, right? Preferably, yes. Jan
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index 75bdf18c4e..95affcc571 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) *fl = flsl(*r) - 1; *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; *fl -= FLI_OFFSET; - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! - *fl = *sl = 0; - */ + ASSERT( *fl >= 0 ); *r &= ~t; } } diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 529213ebbb..fa750a18ae 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i); * int old = atomic_read(&v); * int new = old + 1; * if ( likely(old == atomic_cmpxchg(&v, old, new)) ) - * break; // success! + * break; * } */ static inline int atomic_cmpxchg(atomic_t *v, int old, int new);
In the files modified by this patch there are a few occurrences of nested '//' character sequences inside C-style comment blocks, which violate Rule 3.1. The patch aims to resolve those by removing the nested comments. In the file 'xen/common/xmalloc_tlsf.c' the comment has been replaces by an ASSERT, to ensure that the invariant in the comment is enforced. In the file 'xen/include/xen/atomic.h' the nested comment has been removed, since the code sample is already explained by the preceding comment. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Changes: - Resending the patch with the right maintainers in CC. Changes in V2: - Split the patch into a series and reworked the fix; - Apply the fix to the arm32 `flushtlb.h' file, for consistency. Changes in V3: - Replaced commmented-out 'if' with an ASSERT( *fl >= 0 ). --- xen/common/xmalloc_tlsf.c | 4 +--- xen/include/xen/atomic.h | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-)