On 07/02/24 13:40, Jan Beulich wrote:
On 07.02.2024 13:21, Simone Ballarin wrote:
On 07/02/24 11:24, Jan Beulich wrote:
On 07.02.2024 11:03, Simone Ballarin wrote:
On 06/02/24 13:04, Jan Beulich wrote:
On 02.02.2024 16:16, Simone Ballarin wrote:
Rule 13.1: Initializer lists shall not contain persistent side effects
Effects caused by debug/logging macros and functions (like ASSERT,
__bad_atomic_size,
LOG, etc ...) that crash execution or produce logs are not dangerous in
initializer
lists. The evaluation order in abnormal conditions is not relevant. Evaluation
order
of logging effects is always safe.
I thought I said so before: When talking of just logging, evaluation order
may very well have a impact on correctness. Therefore we shouldn't mix
debugging and logging.
My general feeling was that changes like the following one are not supported by
the community:
- x = { .field1 = function_with_logs_effects() /*other eventual code*/ };
+ int field1 = function_with_logs_effects();
+ x = { .field1 = field1 /*other eventual code*/};
so I tried to deviate as much as possible.
If having log effects is a good reason to do changes like the above, I can
propose a patch in that sense.
Just to avoid misunderstandings: I'm not advocating for changes like the
one you outline above. I simply consider the rule too strict: There's
nothing at risk when there's just a single operation with side effects
in an initializer.
I agree for the safe cases such as single item list initializers (independently
by the number of effect contained in io_apic_read).
In fact, I was about to propose in another patch to deviate cases like:
union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) };
union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) };
Even when there are multiple such operations, whether
there's anything at risk depends on whether any of the side effects
actually collide. In a number of cases the compiler would actually warn
(and thus, due to -Werror, the build would fail).
I don't completely agree on that, this requires an in-depth comprehension
of the code especially when complex call chains are involved. Moreover
these deviations need to be maintained when one of the function involved
changes.
Right, and I didn't really mean multiple function calls here, but e.g.
multiple ++ or --.
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -110,26 +110,34 @@ static unsigned long copy_guest(void *buf, uint64_t addr,
unsigned int len,
unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int
len)
{
return copy_guest((void *)from, (vaddr_t)to, len,
- GVA_INFO(current), COPY_to_guest | COPY_linear);
+ /* SAF-4-safe No persistent side effects */
+ GVA_INFO(current),
I _still_ think this leaves ambiguity. The more that you need to look
up GVA_INFO() to recognize what this is about.
Just to recap: here the point is that current reads a register with a volatile
asm, so the
violation is in the expansion of GVA_INFO(current). Both GVA_INFO and current
taken alone
are completely fine, so this is the only place where a SAF comment can be
placed.
The exapansion is:
((copy_info_t) { .gva = { ((*({ unsigned long __ptr; __asm__ ("" : "=r"(__ptr) :
"0"(&
per_cpu__curr_vcpu)); (typeof(&per_cpu__curr_vcpu)) (__ptr + (({ uint64_t _r; asm volatile("mrs
%0, ""TPIDR_EL2" : "=r"
(_r)); _r; }))); }))) } }), (1U << 1) | (1U << 2));
My proposals are:
1) address the violation moving the current expansion outside (extra variable);
2) put a more detailed comment to avoid the ambiguity;
3) use an ECL deviation for GVA_INFO(current).
Do you have any preference or proposal?
Imo 3 is not an option at all. Probably 1 wouldn't be too bad here, but
I still wouldn't like it (as matching a general pattern I try to avoid:
introducing local variables that are used just once and don't meaningfully
improve e.g. readability). Therefore out of what you list, 2 would remain.
But I'm not happy with a comment here either - as per above, there's
nothing that can go wrong here as long as there's only a single construct
with side effect(s).
So, would be changing the SAF in:
/* SAF-<new_id>-safe single item initializer */
OK for you?
A comment, as said, is only the least bad of what you did enumerate. But
for this code in particular I'm not a maintainer anyway, so it's not me
you need to convince. I'm taking this only as an example for discussing
underlying aspects.
Jan
I was generally thinking about the comments of this series, and I've
just realised that many of them can be summarized by the following sentence:
"We do not want changes to address violations of R13.1 that are not also
violations
of R13.2"
MC3R1.R13.2 rule The value of an expression and its persistent side
effects shall be the same under all permitted evaluation orders
MC3R1.R13.1 rule Initializer lists shall not contain persistent side
effects
At this point, my proposal is to remove R13.1 from the coding standard and add
R13.2 (eventually limiting its scope to initializer lists).
Maybe it is better to re-discuss the rule adoption during the next meeting?
--
Simone Ballarin, M.Sc.
Field Application Engineer, BUGSENG (https://bugseng.com)
