Re: [PATCH v2 40/43] contrib/plugins: extend execlog to track register changes

[Frédéric Pétrot]

Le 11/01/2024 à 13:24, Alex Bennée a écrit :
> Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> writes:
>
> > Hello Alex,
> >
> >    just reporting below what might be a riscv only oddity (also applies to
> >    patch 41 but easier to report here).
> >
> > Le 03/01/2024 à 18:33, Alex Bennée a écrit :
> > > With the new plugin register API we can now track changes to register
> > > values. Currently the implementation is fairly dumb which will slow
> > > down if a large number of register values are being tracked. This
> > > could be improved by only instrumenting instructions which mention
> > > registers we are interested in tracking.
> > > Example usage:
> > >     ./qemu-aarch64 -D plugin.log -d plugin \
> > >        -cpu max,sve256=on \
> > >        -plugin contrib/plugins/libexeclog.so,reg=sp,reg=z\* \
> > >        ./tests/tcg/aarch64-linux-user/sha512-sve
> > > will display in the execlog any changes to the stack pointer (sp)
> > > and
> > > the SVE Z registers.
> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > > Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > Based-On: <20231025093128.33116-19-akihiko.odaki@daynix.com>
> >
> > > +static registers_init(int vcpu_index)
> > > +{
> > > +    GPtrArray *registers = g_ptr_array_new();
> > > +    g_autoptr(GArray) reg_list = qemu_plugin_get_registers(vcpu_index);
> > > +
> > > +    if (reg_list && reg_list->len) {
> > > +        /*
> > > +         * Go through each register in the complete list and
> > > +         * see if we want to track it.
> > > +         */
> > > +        for (int r = 0; r < reg_list->len; r++) {
> > > +            qemu_plugin_reg_descriptor *rd = &g_array_index(
> > > +                reg_list, qemu_plugin_reg_descriptor, r);
> >
> > riscv csrs are not continously numbered and the dynamically generated gdb xml
> > seems to follow that scheme.
> > So the calls to Glib string functions output quite a few assertion
> > warnings because for the non existing csrs rd->name is NULL (and there
> > are a bit less than 4000 such cases for rv64g).
> > Checking for NULL and then continue is a simple way to solve the issue, but
> > I am not sure this is the proper way to proceed, as it might stand in the
> > generation of the riscv xml description for gdb.
>
> I think in this case it might be easier to not expose it to the plugin
> user at all. Is the lack of names an omission? How does gdb see them?

  Well, info all-registers in gdb dumps only the subset of cs registers
  that are defined for the current parameters of the gdbarch.
  Interestingly enough, riscv_print_registers_info, the function that dumps
  register values for riscv in gdb, contains the following comment :
  1385           /* Registers with no name are not valid on this ISA.  */
  and then a check on the "nullness" of the register name to avoid outputting
  something.
  I guess we could follow the same path in QEMU, as having access to the csrs
  in the plugins is really very useful.

  Thanks,
  Frédéric