Re: [Qemu-devel] [PATCH v2 11/30] trace: Fix parameter types in hw/audio

[Eric Blake]

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 <address@hidden>
> ---
>  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