| Message ID | 4c0d38f2b707afa9aed1853a99d286fa2424fb9d.1702283415.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 2.1 | expand |
Hi, On 11/12/2023 10:30, Nicola Vetrini wrote: > The "return 1;" statements at the end of some cases in the switch > of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the > unreachability of the "return 1;" statement after the switch, thus > violating MISRA C:2012 Rule 2.1: > "A project shall not contain unreachable code". > > The same is true for the switch in 'arch_memory_op' from > 'xen/arch/arm/mm.c'. For both cases, I actually much prefer the "return" version in the cases. In particular for the vGIC emulation the switch is quite large and it would not be trivial to know what happens after the break. IOW, I would much prefer if we remove the "return ..." outside of the switch. Cheers,
On 11/12/2023 13:29, Julien Grall wrote: > > > Hi, > > On 11/12/2023 10:30, Nicola Vetrini wrote: >> The "return 1;" statements at the end of some cases in the switch >> of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the >> unreachability of the "return 1;" statement after the switch, thus >> violating MISRA C:2012 Rule 2.1: >> "A project shall not contain unreachable code". >> >> The same is true for the switch in 'arch_memory_op' from >> 'xen/arch/arm/mm.c'. > > For both cases, I actually much prefer the "return" version in the > cases. In particular for the vGIC emulation the switch is quite large > and it would not be trivial to know what happens after the break. Because of this... > > IOW, I would much prefer if we remove the "return ..." outside of the > switch. wouldn't it be better to add ASSERT_UNREACHABLE() before this return instead of removing it? This is what we have in e.g. vpl011 and it prevents mistakes. ~Michal
On 11/12/2023 13:06, Michal Orzel wrote: > > > On 11/12/2023 13:29, Julien Grall wrote: >> >> >> Hi, >> >> On 11/12/2023 10:30, Nicola Vetrini wrote: >>> The "return 1;" statements at the end of some cases in the switch >>> of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the >>> unreachability of the "return 1;" statement after the switch, thus >>> violating MISRA C:2012 Rule 2.1: >>> "A project shall not contain unreachable code". >>> >>> The same is true for the switch in 'arch_memory_op' from >>> 'xen/arch/arm/mm.c'. >> >> For both cases, I actually much prefer the "return" version in the >> cases. In particular for the vGIC emulation the switch is quite large >> and it would not be trivial to know what happens after the break. > Because of this... > >> >> IOW, I would much prefer if we remove the "return ..." outside of the >> switch. > wouldn't it be better to add ASSERT_UNREACHABLE() before this return instead of removing it? > This is what we have in e.g. vpl011 and it prevents mistakes. I am ok with that. But I am not sure if this would make ECLAIR unhappy. Cheers,
On 2023-12-11 15:14, Julien Grall wrote: > On 11/12/2023 13:06, Michal Orzel wrote: >> >> >> On 11/12/2023 13:29, Julien Grall wrote: >>> >>> >>> Hi, >>> >>> On 11/12/2023 10:30, Nicola Vetrini wrote: >>>> The "return 1;" statements at the end of some cases in the switch >>>> of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the >>>> unreachability of the "return 1;" statement after the switch, thus >>>> violating MISRA C:2012 Rule 2.1: >>>> "A project shall not contain unreachable code". >>>> >>>> The same is true for the switch in 'arch_memory_op' from >>>> 'xen/arch/arm/mm.c'. >>> >>> For both cases, I actually much prefer the "return" version in the >>> cases. In particular for the vGIC emulation the switch is quite large >>> and it would not be trivial to know what happens after the break. >> Because of this... >> >>> >>> IOW, I would much prefer if we remove the "return ..." outside of the >>> switch. >> wouldn't it be better to add ASSERT_UNREACHABLE() before this return >> instead of removing it? >> This is what we have in e.g. vpl011 and it prevents mistakes. > > I am ok with that. But I am not sure if this would make ECLAIR unhappy. > > Cheers, Should be okay. I'll test and report back
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index eeb65ca6bb79..9be8e711f61e 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -277,7 +277,7 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg) /* XXX: memsharing not working yet */ case XENMEM_get_sharing_shared_pages: case XENMEM_get_sharing_freed_pages: - return 0; + break; default: return -ENOSYS; diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c index 05429030b539..b9195bbd0538 100644 --- a/xen/arch/arm/vgic-v3-its.c +++ b/xen/arch/arm/vgic-v3-its.c @@ -1255,7 +1255,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info, reg32 & GITS_CTLR_ENABLE); spin_unlock(&its->its_lock); spin_unlock(&its->vcmd_lock); - return 1; + break; } case VREG32(GITS_IIDR): @@ -1292,7 +1292,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info, its->creadr = 0; spin_unlock(&its->its_lock); - return 1; + break; case VREG64(GITS_CWRITER): if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width; @@ -1308,7 +1308,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info, spin_unlock(&its->vcmd_lock); - return 1; + break; case VREG64(GITS_CREADR): goto write_ignore_64; @@ -1353,7 +1353,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info, its->baser_dev = reg; spin_unlock(&its->its_lock); - return 1; + break; case VREG64(GITS_BASER1): /* collection table */ if ( !vgic_reg64_check_access(info->dabt) ) goto bad_width; @@ -1384,7 +1384,7 @@ static int vgic_v3_its_mmio_write(struct vcpu *v, mmio_info_t *info, its->max_collections = 0; its->baser_coll = reg; spin_unlock(&its->its_lock); - return 1; + break; case VRANGE64(GITS_BASER2, GITS_BASER7): goto write_ignore_64;
The "return 1;" statements at the end of some cases in the switch of function 'vgic_v3_its_mmio_write' in 'vcig-v3-its.c' cause the unreachability of the "return 1;" statement after the switch, thus violating MISRA C:2012 Rule 2.1: "A project shall not contain unreachable code". The same is true for the switch in 'arch_memory_op' from 'xen/arch/arm/mm.c'. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/arch/arm/mm.c | 2 +- xen/arch/arm/vgic-v3-its.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-)