Re: [PATCH v6 23/25] tests: add plugin asserting correctness of discon event's to_pc

["Daniel P. Berrangé" <berrange@redhat.com>]

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" <alex.bennee@linaro.org mailto:alex.bennee@linaro.org?to=%22Alex%20Benn%C3%A9e%22%20%3Calex.bennee%40linaro.org%3E > wrote:
> > Julian Ganz <neither@nut.email> 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 :|