| Message ID | 20170313195547.21466-31-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/13/2017 02:55 PM, Eric Blake wrote: > --- > > RFC for two reasons: > 1. the Makefile issue documented above means that incremental > builds won't benefit from this patch without manual intervention > (fresh builds, including docker, manage to test it, though) Dan's posted the fix for this one. > 2. there are still failures under 'make docker-test-mingw@fedora' > due to more type mismatches that still need to be squashed. I'm > still working on fixing those, but wanted to at least post this > series for initial review, especially so the maintainer can weigh > in on how much (or little) belongs in 2.9 mingw is a nightmare. Things like ntohl() having non-standard behavior on 32-bit mingw can only be worked around by hairy redefinitions in osdep.h or by casts at the callsites. See the conversation in 6/30, but I'm hoping gcc will give us a new knob to tone down the level of -Wformat warnings for two types that have different rank but the same width; if gcc gives us that, then we'd add a configure test, and conditionally generate the type-mismatch checking code ONLY when gcc is smart enough to not care about rank mismatch. Until then, this RFC 30/30 proved useful for flushing out the earlier cleanups (many of which you can/should still accept), but should NOT be incorporated as-is, not even when 2.10 opens.
On Wed, Mar 15, 2017 at 02:59:55PM -0500, Eric Blake wrote: > On 03/13/2017 02:55 PM, Eric Blake wrote: > > 2. there are still failures under 'make docker-test-mingw@fedora' > > due to more type mismatches that still need to be squashed. I'm > > still working on fixing those, but wanted to at least post this > > series for initial review, especially so the maintainer can weigh > > in on how much (or little) belongs in 2.9 > > mingw is a nightmare. Things like ntohl() having non-standard behavior > on 32-bit mingw can only be worked around by hairy redefinitions in > osdep.h or by casts at the callsites. See the conversation in 6/30, but > I'm hoping gcc will give us a new knob to tone down the level of > -Wformat warnings for two types that have different rank but the same > width; if gcc gives us that, then we'd add a configure test, and > conditionally generate the type-mismatch checking code ONLY when gcc is > smart enough to not care about rank mismatch. Until then, this RFC > 30/30 proved useful for flushing out the earlier cleanups (many of which > you can/should still accept), but should NOT be incorporated as-is, not > even when 2.10 opens. QEMU 2.9 regression fixes are appropriate at this stage in the release process. Fixes for bugs already in 2.8 are less likely to be merged as we head towards -rc2/-rc3. Stefan
diff --git a/scripts/tracetool/format/h.py b/scripts/tracetool/format/h.py index 3682f4e..491fd6e 100644 --- a/scripts/tracetool/format/h.py +++ b/scripts/tracetool/format/h.py @@ -73,6 +73,19 @@ def generate(events, backend, group): out(' }', '}') + if "vcpu" not in e.properties: + out('', + '#define %(api)s(...) \\', + ' do { \\', + ' if (0) { \\', + ' printf(%(fmt)s "\\n", ## __VA_ARGS__); \\', + ' } \\', + ' %(api)s(__VA_ARGS__); \\', + ' } while (0)', + api=e.api(), + fmt=e.fmt.rstrip("\n") + ) + backend.generate_end(events, group) out('#endif /* TRACE_%s_GENERATED_TRACERS_H */' % group.upper())
Our traces are processed through a function call (even though the function is static inline). While we could turn up compiler warnings to flag cases of implicit narrowing through a function parameter (such as a caller doing foo(my_uint64_t) to a function prototyped as void foo(int)), it would make the build very noisy as we have a lot of code that relies on that C-mandated behavior. Using the attribute printf marking of qemu_log_mask (when the log trace backend is enabled) is too late to catch scalar type changes (it does catch that the format string matches the parameters of the trace_foo() function, but does not detect format mismatches at the callsite). Completely emitting the traces as a macro is a possibility since they are already marked static inline, but doing so in a way that does not evaluate side effects more than once, and uses proper \ line continuation for a (lengthy) macro, is a more invasive change to the generator than I'm willing to make. So this patch introduces a compromise solution: almost[1] every trace_foo() is generated both as an inline function (unchanged from before, with the bulk of the code) and a forwarding macro (new in this patch) that uses a dead-code call to printf to trigger -Wformat type mismatch warnings from the compiler. The end result does not change the size of emitted code, but does enable us to get better checking; particularly useful since the format string in trace-events is distant from the callsites to trace_foo in the normal .c files. Note that the generated macro has to insert a trailing "\n" to shut up gcc's warning about an empty format string, and we have to use the gcc/clang extension of ##__VA_ARGS__ for comma elision since there are a few 0-arg traces. [1]The macro trick is NOT done for vcpu traces, because those trace calls are special; the format string associated with those events is in a different order than the resulting parameters to the function call, due to the additional logic generated for determining whether the vcpu needs filtering. Earlier patches show scenarios that this patch was able to catch, most of which just needed a tweak to the parameter types declared in trace-events, but some which were actual bugs in the code being traced. Note that there is a missing dependency in the makefiles; although trace/Makefile.objs defines $(tracetool-y) to include all .py files, the change to h.py does NOT force a rebuild of any of the generated trace.h files; and it probably has something to do with how our trace files are themselves considered a prerequisite of Makefile being up-to-date. I was unable to figure out the Makefile magic necessary to fix the dependency, but was able to manually work around the problem by running make -B block/trace.h-timestamp which forced a rerun of the tracetool, and therefore regenerated all of the trace.h files. Signed-off-by: Eric Blake <eblake@redhat.com> --- RFC for two reasons: 1. the Makefile issue documented above means that incremental builds won't benefit from this patch without manual intervention (fresh builds, including docker, manage to test it, though) 2. there are still failures under 'make docker-test-mingw@fedora' due to more type mismatches that still need to be squashed. I'm still working on fixing those, but wanted to at least post this series for initial review, especially so the maintainer can weigh in on how much (or little) belongs in 2.9 scripts/tracetool/format/h.py | 13 +++++++++++++ 1 file changed, 13 insertions(+)