Re: [XEN PATCH v4 1/4] xen: add SAF deviation for debugging and logging effects

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

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)