On 11.09.2024 16:27, Nicola Vetrini wrote:
> On 2024-09-11 16:10, Jan Beulich wrote:
>> On 11.09.2024 15:16, Marek Marczykowski-Górecki wrote:
>>> On Wed, Sep 11, 2024 at 02:50:03PM +0200, Jan Beulich wrote:
>>>> On 10.09.2024 21:06, Federico Serafini wrote:
>>>>> Refactor the code to improve readability
>>>>
>>>> I question this aspect. I'm not the maintainer of this code anymore,
>>>> so
>>>> my view probably doesn't matter much here.
>>>>
>>>>> and address violations of
>>>>> MISRA C:2012 Rule 13.6 ("The operand of the `sizeof' operator shall
>>>>> not contain any expression which has potential side effect").
>>>>
>>>> Where's the potential side effect? Since you move ...
>>>>
>>>>> --- a/xen/common/efi/runtime.c
>>>>> +++ b/xen/common/efi/runtime.c
>>>>> @@ -250,14 +250,20 @@ int efi_get_info(uint32_t idx, union
>>>>> xenpf_efi_info *info)
>>>>> info->cfg.addr = __pa(efi_ct);
>>>>> info->cfg.nent = efi_num_ct;
>>>>> break;
>>>>> +
>>>>> case XEN_FW_EFI_VENDOR:
>>>>> + {
>>>>> + XEN_GUEST_HANDLE_PARAM(CHAR16) vendor_name =
>>>>> + guest_handle_cast(info->vendor.name, CHAR16);
>>>>
>>>> .. this out, it must be the one. I've looked at it, yet I can't spot
>>>> anything:
>>>>
>>>> #define guest_handle_cast(hnd, type) ({ \
>>>> type *_x = (hnd).p; \
>>>> (XEN_GUEST_HANDLE_PARAM(type)) { _x }; \
>>>> })
>>>>
>>>> As a rule of thumb, when things aren't obvious, please call out the
>>>> specific aspect / property in descriptions of such patches.
>>>
>>> I guess it's because guest_handle_cast() is a macro, yet it's
>>> lowercase
>>> so looks like a function?
>>
>> If Eclair didn't look at the macro-expanded code, it wouldn't even see
>> the sizeof(). Hence I don't expect the thing to be mistaken for a
>> function
>> call.
>>
>
> Looking at the fully preprocessed code [1], there is an assignment to
> CHAR *_x inside a sizeof(), therefore compat_handle_cast is triggering
> the violation when used in such a way to be inside the sizeof().
I can see a number of initializers, but no assignment.
Jan
> if ( !((!!((((get_cpu_info()->current_vcpu))->domain)->arch.paging.mode
> & ((1 << 4) << 10))) || (
> __builtin_expect(!!(((n)) < (~0U / (sizeof(**(({ CHAR16 *_x =
> (__typeof__(**(info->vendor.name)._) *)(full_ptr_t)(info->
> vendor.name).c; (__compat_handle_CHAR16) { (full_ptr_t)_x };
> }))._)))),1) && ((unsigned long)((unsigned long)((void *)(
> full_ptr_t)(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._)
> *)(full_ptr_t)(info->vendor.name).c; (
> __compat_handle_CHAR16) { (full_ptr_t)_x }; })).c) + ((0 + ((n)) *
> (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->
> vendor.name)._) *)(full_ptr_t)(info->vendor.name).c;
> (__compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) ? (0 + ((n))
> * (sizeof(**(({ CHAR16 *_x = (__typeof__(**(info->vendor.name)._)
> *)(full_ptr_t)(info->vendor.name).c; (
> __compat_handle_CHAR16) { (full_ptr_t)_x }; }))._))) - 1 : 0)) <
> ((void)(((get_cpu_info()->current_vcpu))->domain), 0)))
> ) )
