| Message ID | a295b44a9fbb02f962720e086588437876e02ce3.1701430079.git.maria.celeste.cesario@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen: address violations of MISRA C:2012 Rule 11.8. | expand |
Hi Simone, On 01/12/2023 11:37, Simone Ballarin wrote: > From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> > > Remove or amend casts to comply with Rule 11.8. > > The violations are resolved either by adding missing const > qualifiers in casts or by removing unnecessary cast. > > Change type of operands from char* to uintptr_t: uintptr_t is > the appropriate type for memory address operations. > > No functional change. > > --- > Changes in v2: > - arm/regs.h: add const qualifier to the first operand, > change types of both operands from char* to uintptr_t. > - x86/regs.h: add const qualifier to both operands. Change > types of both operands from char* to uintptr_t to > conform with the arm version. > - dom0less-build.c: rebase change in the new file. > > Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > --- > xen/arch/arm/dom0less-build.c | 2 +- > xen/arch/arm/include/asm/atomic.h | 2 +- > xen/arch/arm/include/asm/regs.h | 2 +- > xen/arch/x86/include/asm/regs.h | 3 ++- > 4 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > index d39cbd969a..fb63ec6fd1 100644 > --- a/xen/arch/arm/dom0less-build.c > +++ b/xen/arch/arm/dom0less-build.c > @@ -354,7 +354,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, > if ( node == NULL ) > { > printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n", > - (char *)xen_path->data); > + xen_path->data); > return -EINVAL; > } > > diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h > index 64314d59b3..517216d2a8 100644 > --- a/xen/arch/arm/include/asm/atomic.h > +++ b/xen/arch/arm/include/asm/atomic.h > @@ -154,7 +154,7 @@ static always_inline void write_atomic_size(volatile void *p, > */ > static inline int atomic_read(const atomic_t *v) > { > - return *(volatile int *)&v->counter; > + return *(const volatile int *)&v->counter; > } > > static inline int _atomic_read(atomic_t v) > diff --git a/xen/arch/arm/include/asm/regs.h b/xen/arch/arm/include/asm/regs.h > index 8a0db95415..b28eb5de7a 100644 > --- a/xen/arch/arm/include/asm/regs.h > +++ b/xen/arch/arm/include/asm/regs.h > @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct cpu_user_regs *regs) > > static inline bool guest_mode(const struct cpu_user_regs *r) > { > - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); > + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - (const uintptr_t)(r); NIT: The const should not be necessary here. Am I correct? > /* Frame pointer must point into current CPU stack. */ > ASSERT(diff < STACK_SIZE); > /* If not a guest frame, it must be a hypervisor frame. */ > diff --git a/xen/arch/x86/include/asm/regs.h b/xen/arch/x86/include/asm/regs.h > index 3fb94deedc..64f1e0d400 100644 > --- a/xen/arch/x86/include/asm/regs.h > +++ b/xen/arch/x86/include/asm/regs.h > @@ -6,7 +6,8 @@ > > #define guest_mode(r) \ > ({ \ > - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); \ > + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - \ > + (const uintptr_t(r)); \ Was this compiled on x86? Shouldn't the last one be (const uintptr_t)(r))? Cheers,
On 01.12.2023 12:48, Julien Grall wrote: > On 01/12/2023 11:37, Simone Ballarin wrote: >> --- a/xen/arch/arm/include/asm/regs.h >> +++ b/xen/arch/arm/include/asm/regs.h >> @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct cpu_user_regs *regs) >> >> static inline bool guest_mode(const struct cpu_user_regs *r) >> { >> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); >> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - (const uintptr_t)(r); > > NIT: The const should not be necessary here. Am I correct? > >> --- a/xen/arch/x86/include/asm/regs.h >> +++ b/xen/arch/x86/include/asm/regs.h >> @@ -6,7 +6,8 @@ >> >> #define guest_mode(r) \ >> ({ \ >> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); \ >> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - \ >> + (const uintptr_t(r)); \ > > Was this compiled on x86? Shouldn't the last one be (const uintptr_t)(r))? And again with the stray const-s dropped and with indentation adjusted. Jan
On 01/12/23 12:48, Julien Grall wrote: > Hi Simone, > > On 01/12/2023 11:37, Simone Ballarin wrote: >> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >> >> Remove or amend casts to comply with Rule 11.8. >> >> The violations are resolved either by adding missing const >> qualifiers in casts or by removing unnecessary cast. >> >> Change type of operands from char* to uintptr_t: uintptr_t is >> the appropriate type for memory address operations. >> >> No functional change. >> >> --- >> Changes in v2: >> - arm/regs.h: add const qualifier to the first operand, >> change types of both operands from char* to uintptr_t. >> - x86/regs.h: add const qualifier to both operands. Change >> types of both operands from char* to uintptr_t to >> conform with the arm version. >> - dom0less-build.c: rebase change in the new file. >> >> Signed-off-by: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> --- >> xen/arch/arm/dom0less-build.c | 2 +- >> xen/arch/arm/include/asm/atomic.h | 2 +- >> xen/arch/arm/include/asm/regs.h | 2 +- >> xen/arch/x86/include/asm/regs.h | 3 ++- >> 4 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/dom0less-build.c >> b/xen/arch/arm/dom0less-build.c >> index d39cbd969a..fb63ec6fd1 100644 >> --- a/xen/arch/arm/dom0less-build.c >> +++ b/xen/arch/arm/dom0less-build.c >> @@ -354,7 +354,7 @@ static int __init handle_passthrough_prop(struct >> kernel_info *kinfo, >> if ( node == NULL ) >> { >> printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n", >> - (char *)xen_path->data); >> + xen_path->data); >> return -EINVAL; >> } >> diff --git a/xen/arch/arm/include/asm/atomic.h >> b/xen/arch/arm/include/asm/atomic.h >> index 64314d59b3..517216d2a8 100644 >> --- a/xen/arch/arm/include/asm/atomic.h >> +++ b/xen/arch/arm/include/asm/atomic.h >> @@ -154,7 +154,7 @@ static always_inline void >> write_atomic_size(volatile void *p, >> */ >> static inline int atomic_read(const atomic_t *v) >> { >> - return *(volatile int *)&v->counter; >> + return *(const volatile int *)&v->counter; >> } >> static inline int _atomic_read(atomic_t v) >> diff --git a/xen/arch/arm/include/asm/regs.h >> b/xen/arch/arm/include/asm/regs.h >> index 8a0db95415..b28eb5de7a 100644 >> --- a/xen/arch/arm/include/asm/regs.h >> +++ b/xen/arch/arm/include/asm/regs.h >> @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct >> cpu_user_regs *regs) >> static inline bool guest_mode(const struct cpu_user_regs *r) >> { >> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); >> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - >> (const uintptr_t)(r); > > NIT: The const should not be necessary here. Am I correct? The const in the first parameter is not necessary, I will drop it. > >> /* Frame pointer must point into current CPU stack. */ >> ASSERT(diff < STACK_SIZE); >> /* If not a guest frame, it must be a hypervisor frame. */ >> diff --git a/xen/arch/x86/include/asm/regs.h >> b/xen/arch/x86/include/asm/regs.h >> index 3fb94deedc..64f1e0d400 100644 >> --- a/xen/arch/x86/include/asm/regs.h >> +++ b/xen/arch/x86/include/asm/regs.h >> @@ -6,7 +6,8 @@ >> #define >> guest_mode(r) \ >> >> ({ \ >> - unsigned long diff = (char *)guest_cpu_user_regs() - (char >> *)(r); \ >> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() >> - \ >> + (const >> uintptr_t(r)); \ > > Was this compiled on x86? Shouldn't the last one be (const uintptr_t)(r))? > Yes, you are right. I'll fix in it in v3. > Cheers, >
On 01/12/23 14:03, Jan Beulich wrote: > On 01.12.2023 12:48, Julien Grall wrote: >> On 01/12/2023 11:37, Simone Ballarin wrote: >>> --- a/xen/arch/arm/include/asm/regs.h >>> +++ b/xen/arch/arm/include/asm/regs.h >>> @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct cpu_user_regs *regs) >>> >>> static inline bool guest_mode(const struct cpu_user_regs *r) >>> { >>> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); >>> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - (const uintptr_t)(r); >> >> NIT: The const should not be necessary here. Am I correct? >> >>> --- a/xen/arch/x86/include/asm/regs.h >>> +++ b/xen/arch/x86/include/asm/regs.h >>> @@ -6,7 +6,8 @@ >>> >>> #define guest_mode(r) \ >>> ({ \ >>> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); \ >>> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - \ >>> + (const uintptr_t(r)); \ >> >> Was this compiled on x86? Shouldn't the last one be (const uintptr_t)(r))? > > And again with the stray const-s dropped and with indentation adjusted. > I will remove the const in the first parameter and fix the indentation in the following way: unsigned long diff = (uintptr_t)guest_cpu_user_regs() - \ (const uintptr_t)(r); \ > Jan >
On 01/12/2023 13:42, Simone Ballarin wrote: > On 01/12/23 12:48, Julien Grall wrote: >> Hi Simone, >> >> On 01/12/2023 11:37, Simone Ballarin wrote: >>> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >>> >>> Remove or amend casts to comply with Rule 11.8. >>> >>> The violations are resolved either by adding missing const >>> qualifiers in casts or by removing unnecessary cast. >>> >>> Change type of operands from char* to uintptr_t: uintptr_t is >>> the appropriate type for memory address operations. >>> >>> No functional change. >>> >>> --- >>> Changes in v2: >>> - arm/regs.h: add const qualifier to the first operand, >>> change types of both operands from char* to uintptr_t. >>> - x86/regs.h: add const qualifier to both operands. Change >>> types of both operands from char* to uintptr_t to >>> conform with the arm version. >>> - dom0less-build.c: rebase change in the new file. >>> >>> Signed-off-by: Maria Celeste Cesario >>> <maria.celeste.cesario@bugseng.com> >>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >>> --- >>> xen/arch/arm/dom0less-build.c | 2 +- >>> xen/arch/arm/include/asm/atomic.h | 2 +- >>> xen/arch/arm/include/asm/regs.h | 2 +- >>> xen/arch/x86/include/asm/regs.h | 3 ++- >>> 4 files changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/arch/arm/dom0less-build.c >>> b/xen/arch/arm/dom0less-build.c >>> index d39cbd969a..fb63ec6fd1 100644 >>> --- a/xen/arch/arm/dom0less-build.c >>> +++ b/xen/arch/arm/dom0less-build.c >>> @@ -354,7 +354,7 @@ static int __init handle_passthrough_prop(struct >>> kernel_info *kinfo, >>> if ( node == NULL ) >>> { >>> printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n", >>> - (char *)xen_path->data); >>> + xen_path->data); >>> return -EINVAL; >>> } >>> diff --git a/xen/arch/arm/include/asm/atomic.h >>> b/xen/arch/arm/include/asm/atomic.h >>> index 64314d59b3..517216d2a8 100644 >>> --- a/xen/arch/arm/include/asm/atomic.h >>> +++ b/xen/arch/arm/include/asm/atomic.h >>> @@ -154,7 +154,7 @@ static always_inline void >>> write_atomic_size(volatile void *p, >>> */ >>> static inline int atomic_read(const atomic_t *v) >>> { >>> - return *(volatile int *)&v->counter; >>> + return *(const volatile int *)&v->counter; >>> } >>> static inline int _atomic_read(atomic_t v) >>> diff --git a/xen/arch/arm/include/asm/regs.h >>> b/xen/arch/arm/include/asm/regs.h >>> index 8a0db95415..b28eb5de7a 100644 >>> --- a/xen/arch/arm/include/asm/regs.h >>> +++ b/xen/arch/arm/include/asm/regs.h >>> @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct >>> cpu_user_regs *regs) >>> static inline bool guest_mode(const struct cpu_user_regs *r) >>> { >>> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); >>> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - >>> (const uintptr_t)(r); >> >> NIT: The const should not be necessary here. Am I correct? > > The const in the first parameter is not necessary, I will drop it. I am confused. In the case of 'r' the const applied to the pointee not the pointer (e.g. the pointer can be modified but not the content). So the 'const' should not be necessary even for the second parameter. Cheers,
On 01/12/23 14:48, Julien Grall wrote: > > > On 01/12/2023 13:42, Simone Ballarin wrote: >> On 01/12/23 12:48, Julien Grall wrote: >>> Hi Simone, >>> >>> On 01/12/2023 11:37, Simone Ballarin wrote: >>>> From: Maria Celeste Cesario <maria.celeste.cesario@bugseng.com> >>>> >>>> Remove or amend casts to comply with Rule 11.8. >>>> >>>> The violations are resolved either by adding missing const >>>> qualifiers in casts or by removing unnecessary cast. >>>> >>>> Change type of operands from char* to uintptr_t: uintptr_t is >>>> the appropriate type for memory address operations. >>>> >>>> No functional change. >>>> >>>> --- >>>> Changes in v2: >>>> - arm/regs.h: add const qualifier to the first operand, >>>> change types of both operands from char* to uintptr_t. >>>> - x86/regs.h: add const qualifier to both operands. Change >>>> types of both operands from char* to uintptr_t to >>>> conform with the arm version. >>>> - dom0less-build.c: rebase change in the new file. >>>> >>>> Signed-off-by: Maria Celeste Cesario >>>> <maria.celeste.cesario@bugseng.com> >>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >>>> --- >>>> xen/arch/arm/dom0less-build.c | 2 +- >>>> xen/arch/arm/include/asm/atomic.h | 2 +- >>>> xen/arch/arm/include/asm/regs.h | 2 +- >>>> xen/arch/x86/include/asm/regs.h | 3 ++- >>>> 4 files changed, 5 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/dom0less-build.c >>>> b/xen/arch/arm/dom0less-build.c >>>> index d39cbd969a..fb63ec6fd1 100644 >>>> --- a/xen/arch/arm/dom0less-build.c >>>> +++ b/xen/arch/arm/dom0less-build.c >>>> @@ -354,7 +354,7 @@ static int __init handle_passthrough_prop(struct >>>> kernel_info *kinfo, >>>> if ( node == NULL ) >>>> { >>>> printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n", >>>> - (char *)xen_path->data); >>>> + xen_path->data); >>>> return -EINVAL; >>>> } >>>> diff --git a/xen/arch/arm/include/asm/atomic.h >>>> b/xen/arch/arm/include/asm/atomic.h >>>> index 64314d59b3..517216d2a8 100644 >>>> --- a/xen/arch/arm/include/asm/atomic.h >>>> +++ b/xen/arch/arm/include/asm/atomic.h >>>> @@ -154,7 +154,7 @@ static always_inline void >>>> write_atomic_size(volatile void *p, >>>> */ >>>> static inline int atomic_read(const atomic_t *v) >>>> { >>>> - return *(volatile int *)&v->counter; >>>> + return *(const volatile int *)&v->counter; >>>> } >>>> static inline int _atomic_read(atomic_t v) >>>> diff --git a/xen/arch/arm/include/asm/regs.h >>>> b/xen/arch/arm/include/asm/regs.h >>>> index 8a0db95415..b28eb5de7a 100644 >>>> --- a/xen/arch/arm/include/asm/regs.h >>>> +++ b/xen/arch/arm/include/asm/regs.h >>>> @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct >>>> cpu_user_regs *regs) >>>> static inline bool guest_mode(const struct cpu_user_regs *r) >>>> { >>>> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); >>>> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - >>>> (const uintptr_t)(r); >>> >>> NIT: The const should not be necessary here. Am I correct? >> >> The const in the first parameter is not necessary, I will drop it. > > I am confused. In the case of 'r' the const applied to the pointee not > the pointer (e.g. the pointer can be modified but not the content). So > the 'const' should not be necessary even for the second parameter. > Yes, sorry. Here there is no reason to use a const: if we cast to a non-pointer type (uintptr_t) rule 11.8 does not apply. > Cheers, >
On 01.12.2023 14:44, Simone Ballarin wrote: > On 01/12/23 14:03, Jan Beulich wrote: >> On 01.12.2023 12:48, Julien Grall wrote: >>> On 01/12/2023 11:37, Simone Ballarin wrote: >>>> --- a/xen/arch/arm/include/asm/regs.h >>>> +++ b/xen/arch/arm/include/asm/regs.h >>>> @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct cpu_user_regs *regs) >>>> >>>> static inline bool guest_mode(const struct cpu_user_regs *r) >>>> { >>>> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); >>>> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - (const uintptr_t)(r); >>> >>> NIT: The const should not be necessary here. Am I correct? >>> >>>> --- a/xen/arch/x86/include/asm/regs.h >>>> +++ b/xen/arch/x86/include/asm/regs.h >>>> @@ -6,7 +6,8 @@ >>>> >>>> #define guest_mode(r) \ >>>> ({ \ >>>> - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); \ >>>> + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - \ >>>> + (const uintptr_t(r)); \ >>> >>> Was this compiled on x86? Shouldn't the last one be (const uintptr_t)(r))? >> >> And again with the stray const-s dropped and with indentation adjusted. >> > > I will remove the const in the first parameter and fix the indentation > in the following way: > > unsigned long diff = (uintptr_t)guest_cpu_user_regs() - \ > (const uintptr_t)(r); \ That still looks to be one off, but (supported by the \ placement) possibly merely an artifact of how your or my mail client is configured. It looks right at https://lists.xen.org/archives/html/xen-devel/2023-12/msg00057.html. Jan
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index d39cbd969a..fb63ec6fd1 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -354,7 +354,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, if ( node == NULL ) { printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n", - (char *)xen_path->data); + xen_path->data); return -EINVAL; } diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h index 64314d59b3..517216d2a8 100644 --- a/xen/arch/arm/include/asm/atomic.h +++ b/xen/arch/arm/include/asm/atomic.h @@ -154,7 +154,7 @@ static always_inline void write_atomic_size(volatile void *p, */ static inline int atomic_read(const atomic_t *v) { - return *(volatile int *)&v->counter; + return *(const volatile int *)&v->counter; } static inline int _atomic_read(atomic_t v) diff --git a/xen/arch/arm/include/asm/regs.h b/xen/arch/arm/include/asm/regs.h index 8a0db95415..b28eb5de7a 100644 --- a/xen/arch/arm/include/asm/regs.h +++ b/xen/arch/arm/include/asm/regs.h @@ -48,7 +48,7 @@ static inline bool regs_mode_is_32bit(const struct cpu_user_regs *regs) static inline bool guest_mode(const struct cpu_user_regs *r) { - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - (const uintptr_t)(r); /* Frame pointer must point into current CPU stack. */ ASSERT(diff < STACK_SIZE); /* If not a guest frame, it must be a hypervisor frame. */ diff --git a/xen/arch/x86/include/asm/regs.h b/xen/arch/x86/include/asm/regs.h index 3fb94deedc..64f1e0d400 100644 --- a/xen/arch/x86/include/asm/regs.h +++ b/xen/arch/x86/include/asm/regs.h @@ -6,7 +6,8 @@ #define guest_mode(r) \ ({ \ - unsigned long diff = (char *)guest_cpu_user_regs() - (char *)(r); \ + unsigned long diff = (const uintptr_t)guest_cpu_user_regs() - \ + (const uintptr_t(r)); \ /* Frame pointer must point into current CPU stack. */ \ ASSERT(diff < STACK_SIZE); \ /* If not a guest frame, it must be a hypervisor frame. */ \