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 <[email protected]>
---
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.
[...]
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'?
Cheers,
--
Julien Grall
