Message ID | 20191108144042.30245-3-philmd@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw: Remove dynamic field width from trace events | expand |
On 11/8/19 8:40 AM, Philippe Mathieu-Daudé wrote: > Since not all trace backends support dynamic field width in > format (dtrace via stap does not), replace by a static field > width instead. > > Reported-by: Eric Blake <eblake@redhat.com> > Buglink: https://bugs.launchpad.net/qemu/+bug/1844817 > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > v2: Do not update qemu_log_mask() > --- > hw/mips/gt64xxx_pci.c | 16 ++++++++-------- > hw/mips/trace-events | 4 ++-- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c > index 5cab9c1ee1..6743e7c929 100644 > --- a/hw/mips/gt64xxx_pci.c > +++ b/hw/mips/gt64xxx_pci.c > @@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr addr, > /* not really implemented */ > s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffffffe)); > s->regs[saddr] |= !!(s->regs[saddr] & 0xfffffffe); > - trace_gt64120_write("INTRCAUSE", size << 1, val); > + trace_gt64120_write("INTRCAUSE", size << 3, val); Again, this isn't mentioned in the commit message. Why are you changing parameter values? > +++ b/hw/mips/trace-events > @@ -1,4 +1,4 @@ > # gt64xxx.c > -gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s value:0x%0*" PRIx64 > -gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write %s value:0x%0*" PRIx64 > +gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s width:%d value:0x%08" PRIx64 > +gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write %s width:%d value:0x%08" PRIx64 Huh, we were really broken - the old code (if passed to printf) would try to parse 4 parameters, even though it was only passed 3. But it looks like you still need a v3.
On 11/8/19 4:58 PM, Eric Blake wrote: > On 11/8/19 8:40 AM, Philippe Mathieu-Daudé wrote: >> Since not all trace backends support dynamic field width in >> format (dtrace via stap does not), replace by a static field >> width instead. >> >> Reported-by: Eric Blake <eblake@redhat.com> >> Buglink: https://bugs.launchpad.net/qemu/+bug/1844817 >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> v2: Do not update qemu_log_mask() >> --- >> hw/mips/gt64xxx_pci.c | 16 ++++++++-------- >> hw/mips/trace-events | 4 ++-- >> 2 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c >> index 5cab9c1ee1..6743e7c929 100644 >> --- a/hw/mips/gt64xxx_pci.c >> +++ b/hw/mips/gt64xxx_pci.c >> @@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr >> addr, >> /* not really implemented */ >> s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffffffe)); >> s->regs[saddr] |= !!(s->regs[saddr] & 0xfffffffe); >> - trace_gt64120_write("INTRCAUSE", size << 1, val); >> + trace_gt64120_write("INTRCAUSE", size << 3, val); > > Again, this isn't mentioned in the commit message. Why are you changing > parameter values? > > >> +++ b/hw/mips/trace-events >> @@ -1,4 +1,4 @@ >> # gt64xxx.c >> -gt64120_read(const char *regname, int width, uint64_t value) "gt64120 >> read %s value:0x%0*" PRIx64 >> -gt64120_write(const char *regname, int width, uint64_t value) >> "gt64120 write %s value:0x%0*" PRIx64 >> +gt64120_read(const char *regname, int width, uint64_t value) "gt64120 >> read %s width:%d value:0x%08" PRIx64 >> +gt64120_write(const char *regname, int width, uint64_t value) >> "gt64120 write %s width:%d value:0x%08" PRIx64 > > Huh, we were really broken - the old code (if passed to printf) would > try to parse 4 parameters, even though it was only passed 3. But it > looks like you still need a v3. Oops. I am surprise the compiler doesn't emit a warning here...
On 11/14/19 10:24 PM, Philippe Mathieu-Daudé wrote: > On 11/8/19 4:58 PM, Eric Blake wrote: >> On 11/8/19 8:40 AM, Philippe Mathieu-Daudé wrote: >>> Since not all trace backends support dynamic field width in >>> format (dtrace via stap does not), replace by a static field >>> width instead. >>> >>> Reported-by: Eric Blake <eblake@redhat.com> >>> Buglink: https://bugs.launchpad.net/qemu/+bug/1844817 >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> v2: Do not update qemu_log_mask() >>> --- >>> hw/mips/gt64xxx_pci.c | 16 ++++++++-------- >>> hw/mips/trace-events | 4 ++-- >>> 2 files changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c >>> index 5cab9c1ee1..6743e7c929 100644 >>> --- a/hw/mips/gt64xxx_pci.c >>> +++ b/hw/mips/gt64xxx_pci.c >>> @@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr >>> addr, >>> /* not really implemented */ >>> s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffffffe)); >>> s->regs[saddr] |= !!(s->regs[saddr] & 0xfffffffe); >>> - trace_gt64120_write("INTRCAUSE", size << 1, val); >>> + trace_gt64120_write("INTRCAUSE", size << 3, val); >> >> Again, this isn't mentioned in the commit message. Why are you >> changing parameter values? >> >> >>> +++ b/hw/mips/trace-events >>> @@ -1,4 +1,4 @@ >>> # gt64xxx.c >>> -gt64120_read(const char *regname, int width, uint64_t value) >>> "gt64120 read %s value:0x%0*" PRIx64 >>> -gt64120_write(const char *regname, int width, uint64_t value) >>> "gt64120 write %s value:0x%0*" PRIx64 >>> +gt64120_read(const char *regname, int width, uint64_t value) >>> "gt64120 read %s width:%d value:0x%08" PRIx64 >>> +gt64120_write(const char *regname, int width, uint64_t value) >>> "gt64120 write %s width:%d value:0x%08" PRIx64 >> >> Huh, we were really broken - the old code (if passed to printf) would >> try to parse 4 parameters, even though it was only passed 3. But it >> looks like you still need a v3. > > Oops. I am surprise the compiler doesn't emit a warning here... I'm sorry I can't see the 4th parameter. Before: "gt64120 read %s value:0x%0*" PRIx64 #1 's' for 'const char *regname' #2 '0*' for 'int width' #3 'x' for 'uint64_t value' After: "gt64120 read %s width:%d value:0x%08" PRIx64 #1 's' for 'const char *regname' #2 'd' for 'int width' #3 '08x' for 'uint64_t value' Am I missing something?
On 11/18/19 1:10 PM, Philippe Mathieu-Daudé wrote: >>>> - trace_gt64120_write("INTRCAUSE", size << 1, val); >>>> + trace_gt64120_write("INTRCAUSE", size << 3, val); >>> >>> Again, this isn't mentioned in the commit message. Why are you >>> changing parameter values? >>> >>> >>>> +++ b/hw/mips/trace-events >>>> @@ -1,4 +1,4 @@ >>>> # gt64xxx.c >>>> -gt64120_read(const char *regname, int width, uint64_t value) >>>> "gt64120 read %s value:0x%0*" PRIx64 >>>> -gt64120_write(const char *regname, int width, uint64_t value) >>>> "gt64120 write %s value:0x%0*" PRIx64 >>>> +gt64120_read(const char *regname, int width, uint64_t value) >>>> "gt64120 read %s width:%d value:0x%08" PRIx64 >>>> +gt64120_write(const char *regname, int width, uint64_t value) >>>> "gt64120 write %s width:%d value:0x%08" PRIx64 >>> >>> Huh, we were really broken - the old code (if passed to printf) would >>> try to parse 4 parameters, even though it was only passed 3. But it >>> looks like you still need a v3. >> >> Oops. I am surprise the compiler doesn't emit a warning here... > > I'm sorry I can't see the 4th parameter. My fault for chasing a red herring. I guess I was mixing the three % post-patch with the two % and one * pre-patch, and somehow mis-counting three % plus one * as four arguments. But re-reading your confusion, yes, there were only three parameters being consumed. > > Before: "gt64120 read %s value:0x%0*" PRIx64 > > #1 's' for 'const char *regname' > #2 '0*' for 'int width' > #3 'x' for 'uint64_t value' > > After: "gt64120 read %s width:%d value:0x%08" PRIx64 > > #1 's' for 'const char *regname' > #2 'd' for 'int width' > #3 '08x' for 'uint64_t value' > > Am I missing something? Rather, I was.
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c index 5cab9c1ee1..6743e7c929 100644 --- a/hw/mips/gt64xxx_pci.c +++ b/hw/mips/gt64xxx_pci.c @@ -642,19 +642,19 @@ static void gt64120_writel(void *opaque, hwaddr addr, /* not really implemented */ s->regs[saddr] = ~(~(s->regs[saddr]) | ~(val & 0xfffffffe)); s->regs[saddr] |= !!(s->regs[saddr] & 0xfffffffe); - trace_gt64120_write("INTRCAUSE", size << 1, val); + trace_gt64120_write("INTRCAUSE", size << 3, val); break; case GT_INTRMASK: s->regs[saddr] = val & 0x3c3ffffe; - trace_gt64120_write("INTRMASK", size << 1, val); + trace_gt64120_write("INTRMASK", size << 3, val); break; case GT_PCI0_ICMASK: s->regs[saddr] = val & 0x03fffffe; - trace_gt64120_write("ICMASK", size << 1, val); + trace_gt64120_write("ICMASK", size << 3, val); break; case GT_PCI0_SERR0MASK: s->regs[saddr] = val & 0x0000003f; - trace_gt64120_write("SERR0MASK", size << 1, val); + trace_gt64120_write("SERR0MASK", size << 3, val); break; /* Reserved when only PCI_0 is configured. */ @@ -930,19 +930,19 @@ static uint64_t gt64120_readl(void *opaque, /* Interrupts */ case GT_INTRCAUSE: val = s->regs[saddr]; - trace_gt64120_read("INTRCAUSE", size << 1, val); + trace_gt64120_read("INTRCAUSE", size << 3, val); break; case GT_INTRMASK: val = s->regs[saddr]; - trace_gt64120_read("INTRMASK", size << 1, val); + trace_gt64120_read("INTRMASK", size << 3, val); break; case GT_PCI0_ICMASK: val = s->regs[saddr]; - trace_gt64120_read("ICMASK", size << 1, val); + trace_gt64120_read("ICMASK", size << 3, val); break; case GT_PCI0_SERR0MASK: val = s->regs[saddr]; - trace_gt64120_read("SERR0MASK", size << 1, val); + trace_gt64120_read("SERR0MASK", size << 3, val); break; /* Reserved when only PCI_0 is configured. */ diff --git a/hw/mips/trace-events b/hw/mips/trace-events index 75d4c73f2e..86a0213c77 100644 --- a/hw/mips/trace-events +++ b/hw/mips/trace-events @@ -1,4 +1,4 @@ # gt64xxx.c -gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s value:0x%0*" PRIx64 -gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write %s value:0x%0*" PRIx64 +gt64120_read(const char *regname, int width, uint64_t value) "gt64120 read %s width:%d value:0x%08" PRIx64 +gt64120_write(const char *regname, int width, uint64_t value) "gt64120 write %s width:%d value:0x%08" PRIx64 gt64120_isd_remap(uint64_t from_length, uint64_t from_addr, uint64_t to_length, uint64_t to_addr) "ISD: 0x%08" PRIx64 "@0x%08" PRIx64 " -> 0x%08" PRIx64 "@0x%08" PRIx64
Since not all trace backends support dynamic field width in format (dtrace via stap does not), replace by a static field width instead. Reported-by: Eric Blake <eblake@redhat.com> Buglink: https://bugs.launchpad.net/qemu/+bug/1844817 Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- v2: Do not update qemu_log_mask() --- hw/mips/gt64xxx_pci.c | 16 ++++++++-------- hw/mips/trace-events | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-)