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)
