Violations of MISRA C Rule 20.7 in xen/arch/x86/include/asm/hvm/trace.h

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

Reporting the discussion that took place on Matrix on the matter, so 
that it can carry on here with all interested parties:
> Hi everyone. I was looking at MISRA violations for Rule 20.7 
> ("Expressions resulting from the expansion of macro parameters shall be 
> enclosed in parentheses") generated by
>     #define TRC_PAR_LONG(par) ((uint32_t)(par)), ((par) >> 32)
>
> The thing here is this: the simplest fix is
>
>     -#define TRC_PAR_LONG(par) ((uint32_t)(par)), ((par) >> 32)
>     +#define TRC_PAR_LO(par) ((uint32_t)(par))
>     +#define TRC_PAR_HI(par) ((par) >> 32)
>
> and then replace all call sites accordingly. However, in certain places 
> (e.g., svm.c:1445), this causes a build error:
>     arch/x86/hvm/svm/svm.c: In function ‘svm_inject_event’:
>     arch/x86/hvm/svm/svm.c:1445:1: error: macro "HVMTRACE_3D" requires 
> 4 arguments, but only 3 given
>     1445 | TRC_PAR_LO(_event.cr2), TRC_PAR_HI(_event.cr2));
>     | ^
>     In file included from arch/x86/hvm/svm/svm.c:31:
>
> this is due to the somewhat strange definition of HVMTRACE_3D which 
> works with the previous form of the macro, but not the current one, as 
> the macro argument in HVMTRACE_LONG_2D is now split (_LO is d2 and _HI 
> is variadic), and therefore it's not passed to HVMTRACE_3D anymore as 
> two arguments.
> I have a proposal: introduce a d3 argument to HVMTRACE_LONG_2D to 
> supply the additional argument and/or make HVMTRACE_LONG_2D 
> non-variadic. This would of course apply to the similarly named macros 
> in xen/arch/x86/include/asm/hvm/trace.h
Andrew Cooper:
> sigh - I'm half way through deleting all of that
> guess I ought to finish
Andrew Cooper:
> @Nicola Vetrini: 
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/xen-trace 
> On second thoughts I'm not sure it fixes the problem
> just changes it.  The problem is the use of macros to split a 64bit 
> quantity across two 32bit fields
Nicola Vetrini:
> It seems so, yes. But afaict this can be fixed by splitting the macro 
> definition in two, as done above, which wouldn't incur in the 
> compilation error on the new API
Andrew Cooper:
> given the users of TRACE_PARAM64() by the end, I'd prefer to do that 
> with real structs rather than perpetuating the macro mess
George Dunlap:
> It's been a long time since I looked at all this, but FWIW I inherited 
> all the weird macro stuff, never liked it, and myself used structs for 
> all new traces. So without having looked at the code at all, I'm 
> predisposed to agree w/ Andy's assessment that we should just rip out 
> the offending macros and replace them with structs.
@gwd: I believe the file that I was concerned about does not fall under 
the XENTRACE entry in MAINTAINERS; you may want to rectify that.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)