On 03/13/2017 02:55 PM, Eric Blake wrote: > An upcoming patch will let the compiler warn us when we are silently > losing precision in traces; update the trace definitions to pass > through the full value at the callsite. > > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > hw/audio/trace-events | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-)
> # hw/audio/milkymist-ac97.c > -milkymist_ac97_memory_read(uint32_t addr, uint32_t value) "addr %08x value > %08x" > -milkymist_ac97_memory_write(uint32_t addr, uint32_t value) "addr %08x value > %08x" > +milkymist_ac97_memory_read(hwaddr addr, uint32_t value) "addr %08" > HWADDR_PRIx " value %08x" > +milkymist_ac97_memory_write(hwaddr addr, uint64_t value) "addr %08" > HWADDR_PRIx " value %08" PRIx64 Stefan pointed out to me on IRC that hwaddr is not a valid type when using --enable-trace-backend=ust. If hwaddr is always a 64-bit type, then using uint64_t/PRIx64 is a reasonable substitute to hwaddr/HWADDR_PRIx, but I wasn't sure if that was the case. But it certainly invalidates a large chunk of the series as I proposed it, where I'd have to switch any of my additions of hwaddr over to uint64_t. Using -Wformat to catch type mismatches catches both widening and narrowing issues, but maybe we only care about narrowing issues. As long as there are no varargs involved, silently widening a 32-bit input to a 64-bit function parameter before printing is safe; but my hack of adding a dead printf() means that it then fails to go through varargs correctly and forces type-correctness on the caller. We _want_ to cleanup callers that have 64-bit types passed to a 32-bit log format string, as that is silent loss of information. But if the only way to do that costs a lot of maintenance in also annotating source code to explicitly widen 32-bit types passed into 64-bit log format strings just to satisfy -Wformat, then it's a tougher sale. And then there's still the idea that maybe the rest of this series is not worth pursuing until gcc gives us some way to ignore things like __u64 being the same width but a different type than uint64_t (at least on 64-bit Linux). We can certainly take the cleanups that are safe, now that I've done one round of auditing (I still haven't finished a 32-bit Linux audit to see if it pops up more mismatches, and mingw already proved a bear to audit because of the ntohl() bug). But if we can't accept the final patch that uses the -Wformat hack, then I'm worried that we will slip in regressions over time, negating some of the effort I even put in on the initial audit. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
