| Message ID | 20170313195547.21466-12-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mo, 2017-03-13 at 14:55 -0500, 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 <eblake@redhat.com> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
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 <eblake@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.
diff --git a/hw/audio/trace-events b/hw/audio/trace-events index 3210386..ea5f0c2 100644 --- a/hw/audio/trace-events +++ b/hw/audio/trace-events @@ -3,12 +3,12 @@ # hw/audio/cs4231.c cs4231_mem_readl_dreg(uint32_t reg, uint32_t ret) "read dreg %d: 0x%02x" cs4231_mem_readl_reg(uint32_t reg, uint32_t ret) "read reg %d: 0x%08x" -cs4231_mem_writel_reg(uint32_t reg, uint32_t old, uint32_t val) "write reg %d: 0x%08x -> 0x%08x" -cs4231_mem_writel_dreg(uint32_t reg, uint32_t old, uint32_t val) "write dreg %d: 0x%02x -> 0x%02x" +cs4231_mem_writel_reg(uint32_t reg, uint32_t old, uint64_t val) "write reg %d: 0x%08x -> 0x%08" PRIx64 +cs4231_mem_writel_dreg(uint32_t reg, uint32_t old, uint64_t val) "write dreg %d: 0x%02x -> 0x%02" PRIx64 # 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 milkymist_ac97_pulse_irq_crrequest(void) "Pulse IRQ CR request" milkymist_ac97_pulse_irq_crreply(void) "Pulse IRQ CR reply" milkymist_ac97_pulse_irq_dmaw(void) "Pulse IRQ DMA write"
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 <eblake@redhat.com> --- hw/audio/trace-events | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)