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
