On Mon, Sep 22, 2025 at 10:11:23AM +0000, Julian Ganz wrote:
> Hi Alex,
>
> September 21, 2025 at 6:46 PM, "Alex Bennée" <[email protected]
> mailto:[email protected]?to=%22Alex%20Benn%C3%A9e%22%20%3Calex.bennee%40linaro.org%3E
> > wrote:
> > Julian Ganz <[email protected]> writes:
> > > +static void report_mismatch(const char *pc_name, unsigned int
> > > vcpu_index,
> > > + enum qemu_plugin_discon_type type, uint64_t last,
> > > + uint64_t expected, uint64_t encountered)
> > > +{
> > > + GString *report;
> > >
> > This could be:
> >
> > g_autoptr(GString) buf = g_string_new(NULL);
>
> I wanted to avoid doing an allocation regardless of whether we do have
> anything to report or not. But I guess a performance plugin isn't
> performance critical enough to worry? Anyway, I will have another look
> at g_autoptr.
IMHO the use of GString is pointless, there is only a single
g_string_append_printf call. GString is only useful when
incrementally constructing a string from many pieces. This
code could have just used 'g_strdup_printf' AFAICT.
>
> > > + const char *discon_type_name = "unknown";
> > > +
> > > + if (addr_eq(expected, encountered)) {
> > > + return;
> > > + }
> > > +
> > > + switch (type) {
> > > + case QEMU_PLUGIN_DISCON_INTERRUPT:
> > > + discon_type_name = "interrupt";
> > > + break;
> > > + case QEMU_PLUGIN_DISCON_EXCEPTION:
> > > + discon_type_name = "exception";
> > > + break;
> > > + case QEMU_PLUGIN_DISCON_HOSTCALL:
> > > + discon_type_name = "hostcall";
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + report = g_string_new(NULL);
> > > + g_string_append_printf(report,
> > > + "Discon %s PC mismatch on VCPU %d\nExpected: %"
> > > + PRIx64"\nEncountered: %"PRIx64"\nExecuted Last: %"
> > > + PRIx64"\nEvent type: %s\n",
> > > + pc_name, vcpu_index, expected, encountered, last,
> > > + discon_type_name);
> > > + qemu_plugin_outs(report->str);
> > >
> > I think we might want to flush here because
>
> Yes we do. I probably (incorrectly) assumed `qemu_plugin_outs` already
> did that.
>
> >
> > >
> > > + if (abort_on_mismatch) {
> > > + g_abort();
> > > + }
> > >
> > This is firing on:
> >
> > 🕙17:35:50 alex@draig:tests/tcg/i386-linux-user on review/tcg-discon-v6
> > [$!?]
> > ➜ make run-plugin-catch-syscalls-with-libdiscons.so V=1
> > timeout -s KILL --foreground 120 env
> > QEMU=/home/alex/lsrc/qemu.git/builds/sanitisers/qemu-i386
> > /home/alex/lsrc/qemu.git/builds/sanitisers/qemu-i386 -plugin
> > ../plugins/libdiscons.so -d plugin -D
> > catch-syscalls-with-libdiscons.so.pout catch-syscalls >
> > run-plugin-catch-syscalls-with-libdiscons.so.out
> > Aborted
> > make: *** [Makefile:226: run-plugin-catch-syscalls-with-libdiscons.so]
> > Error 134
> > 🕙17:35:52 alex@draig:tests/tcg/i386-linux-user on review/tcg-discon-v6
> > [$!?] [🔴 USAGE]
> > ✗
> >
> > although it never gets to the point of reporting what failed:
> >
> > Thread 1 "qemu-i386" hit Breakpoint 1, __GI_abort () at ./stdlib/abort.c:72
> > warning: 72 ./stdlib/abort.c: No such file or directory
> > (gdb) bt
> > #0 __GI_abort () at ./stdlib/abort.c:72
> > #1 0x00007ffff630874d in report_mismatch (pc_name=0x7ffff630a220 "target",
> > vcpu_index=0, type=QEMU_PLUGIN_DISCON_EXCEPTION, last=134574955,
> > expected=134574953,
> > encountered=134574955) at ../../tests/tcg/plugins/discons.c:89
> > #2 0x00007ffff6308c0d in insn_exec (vcpu_index=0, userdata=0x0) at
> > ../../tests/tcg/plugins/discons.c:132
> > #3 0x00007fffea431114 in code_gen_buffer ()
> > #4 0x000055555577b0a6 in cpu_tb_exec (cpu=0x529000005200,
> > itb=0x7fffea431000 <code_gen_buffer+200659>, tb_exit=0x7ffff49c9530) at
> > ../../accel/tcg/cpu-exec.c:438
> > #5 0x000055555577c92f in cpu_loop_exec_tb (cpu=0x529000005200,
> > tb=0x7fffea431000 <code_gen_buffer+200659>, pc=134574955,
> > last_tb=0x7ffff49c9540, tb_exit=0x7ffff49c9530)
> > at ../../accel/tcg/cpu-exec.c:871
> > #6 0x000055555577d151 in cpu_exec_loop (cpu=0x529000005200,
> > sc=0x7ffff483a740) at ../../accel/tcg/cpu-exec.c:981
> > #7 0x000055555577d2fe in cpu_exec_setjmp (cpu=0x529000005200,
> > sc=0x7ffff483a740) at ../../accel/tcg/cpu-exec.c:998
> > #8 0x000055555577d4c8 in cpu_exec (cpu=0x529000005200) at
> > ../../accel/tcg/cpu-exec.c:1024
> > #9 0x00005555557bfc83 in cpu_loop (env=0x529000007dd0) at
> > ../../linux-user/i386/cpu_loop.c:215
> > #10 0x00005555558ee3e1 in main (argc=4, argv=0x7fffffffe688,
> > envp=0x7fffffffe6b0) at ../../linux-user/main.c:1038
> > (gdb) f 1
> > #1 0x00007ffff630874d in report_mismatch (pc_name=0x7ffff630a220 "target",
> > vcpu_index=0, type=QEMU_PLUGIN_DISCON_EXCEPTION, last=134574955,
> > expected=134574953,
> > encountered=134574955) at ../../tests/tcg/plugins/discons.c:89
> > 89 g_abort();
> > (gdb) p report
> > $1 = (GString *) 0x50300002bf00
> > (gdb) p report->Str
> > There is no member named Str.
> > (gdb) p report->str
> > $2 = (gchar *) 0x51100001fbc0 "Discon target PC mismatch on VCPU
> > 0\nExpected: 8057369\nEncountered: 805736b\nExecuted Last: 805736b\nEvent
> > type: exception\n"
> > (gdb)
> >
> > I think this is where it is going wrong:
> >
> > IN: _dl_early_allocate
> > 0x0805736b: 89 c2 movl %eax, %edx
> > 0x0805736d: 8d 1c 28 leal (%eax, %ebp), %ebx
> > 0x08057370: 89 c8 movl %ecx, %eax
> > 0x08057372: cd 80 int $0x80
>
> Thanks! I'll have a closer look.
>
> > >
> > > + g_string_free(report, true);
> > >
> > so we could drop this... or..
>
> As aborting is optional, we should free.
>
> Regards,
> Julian
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
