Re: [XEN PATCH 1/4] xen/arm: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

On 19/10/23 12:11, Julien Grall wrote:
> Hi,
>
> On 19/10/2023 09:43, Simone Ballarin wrote:
> > On 19/10/23 10:19, Julien Grall wrote:
> > > Hi Simone,
> > >
> > > On 19/10/2023 08:34, Simone Ballarin wrote:
> > > > On 18/10/23 17:03, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > On 18/10/2023 15:18, Simone Ballarin wrote:
> > > > > > Rule 13.1: Initializer lists shall not contain persistent side 
> > > > > > effects
> > > > > > This patch moves expressions with side-effects into new variables 
> > > > > > before
> > > > > > the initializer lists.
> > > > > Looking at the code, I feel the commit message should be a bit more 
> > > > > verbose because they are no apparent side-effects.
> > > > > > Function calls do not necessarily have side-effects, in these 
> > > > > > cases the
> > > > > > GCC pure or const attributes are added when possible.
> > > > > You are only adding pure in this patch.
> > > > >
> > > > > > No functional changes.
> > > > > >
> > > > > > Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> > > > > >
> > > > > > ---
> > > > > > Function attributes pure and const do not need to be added as GCC
> > > > > > attributes, they can be added using ECLAIR configurations.
> > > > > > ---
> > > > > >   xen/arch/arm/device.c              |  6 +++---
> > > > > >   xen/arch/arm/guestcopy.c           | 12 ++++++++----
> > > > > >   xen/arch/arm/include/asm/current.h |  2 +-
> > > > > >   3 files changed, 12 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > > > > > index 1f631d3274..e9be078415 100644
> > > > > > --- a/xen/arch/arm/device.c
> > > > > > +++ b/xen/arch/arm/device.c
> > > > > > @@ -319,6 +319,8 @@ int handle_device(struct domain *d, struct 
> > > > > > dt_device_node *dev, p2m_type_t p2mt,
> > > > > >       int res;
> > > > > >       paddr_t addr, size;
> > > > > >       bool own_device = !dt_device_for_passthrough(dev);
> > > > > > +    bool dev_is_hostbridge = is_pci_passthrough_enabled() &&
> > > > > > +                             device_get_class(dev) == 
> > > > > > DEVICE_PCI_HOSTBRIDGE;
> > > > > The commit message suggests that the code is moved because there 
> > > > > are side-effects. But none of them should have any side-effects.
> > > > device_get_class contains an 'ASSERT(dev != NULL)' which is definitely
> > > > a side-effect.
> > > Just to confirm my understanding, the side-effect here would be the 
> > > fact that it traps and then panic(). IOW, if the trap was 
> > > hypothetically replaced by a while (1), then it would not be an 
> > > issue. is it correct? >
> > No, it isn't. A infinite loop is a side effect.
> I am not sure why. There are no change of state here.
>
> > > I can see two solutions:
> > >    1) Remove the ASSERT(). It is only here to make the NULL 
> > > dereference obvious in debug build. That said, the stack trace for a 
> > > NULL dereference would still be as clear.
> > Removing the ASSERT just to make MISRA happy does not sound good to me.
> I didn't suggest the ASSERT() just ot make MISRA happy. I suggested it 
> because it has no value here (we still have stack track if there are any 
> issue).
> > >    2) Replace the ASSERT with a proper check if ( !dev ) return 
> > > DEVICE_UNKONWN. AFAIU, we would not be able to add a 
> > > ASSERT_UNREACHABLE() because this would be again a perceived 
> > > side-effect.
> > Replacing it with a proper check can be a solution, but I still prefer 
> > to add a deviation or move the invocation outside the initializer list.
> In general, I am not in favor of adding deviation if we can avoid them 
> because the code can changed and therefore either moot the deviation or 
> hide any other issue.
Ok, I will proceed with option 1.

> [...]
>
> > Yes, sorry I was looking to the wrong definition.
> > In ARM the problem is the presence of a *volatile* ASM.
> > Please take a look here:
> >
> > https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/arm/for-4.19/ARM64-Set2/latest/PROJECT.ecd;/by_service/MC3R1.R13.1.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":true,"selector":{"enabled":true,"negated":false,"kind":0,"domain":"fingerprint","inputs":[{"enabled":true,"text":"0da7f0c9aea5491eba343618f965c81f5d7aed3c"}]}}}
> Ok. So the problem is the READ_SYSREG(...). Is there a way we can 
> encapsulate the call so we don't need to use your propose trick or 
> deviate at every use of 'current'?
The point is that we need to move "READ_SYSREG(TPIDR_EL2)" outside
the initializer (there are no advantages in wrapping it on a function
if the function cannot be declared pure).

The proposed solution seems to me the cleanest way do to it. I do not 
see any other acceptable solutions.

> Cheers,
--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)