Re: [XEN PATCH 1/3] EFI: address violations of MISRA C Rule 13.6

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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().
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)))
) )

[1] 
https://saas.eclairit.com:3787/fs/var/local/eclair/XEN.ecdf/ECLAIR_normal/staging/X86_64-BUGSENG/latest/PROJECT.ecd;/by_service/MC3R1.R13.6.html#{"select":true,"selection":{"hiddenAreaKinds":[],"hiddenSubareaKinds":[],"show":false,"selector":{"enabled":true,"negated":true,"kind":0,"domain":"message","inputs":[{"enabled":true,"text":"^.*xen/common/efi/runtime\\.c:258\\.15-258\\.31: 
`sizeof' expression trait"}]}}}
> > Wasn't there some other MISRA rule about lowercase/uppercase for macro 
> > names?
There isn't one imposing this restriction (at least in MISRA C:2012, I 
haven't checked earlier editions).
> I can't recall having run into one, but I also haven't memorized them 
> all.
> Jan
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)