Re: [PULL 26/29] contrib/plugins: extend execlog to track register changes

[Zhao Liu]

On Fri, Mar 01, 2024 at 04:30:51PM +0000, Alex Bennée wrote:
> Date: Fri, 01 Mar 2024 16:30:51 +0000
> From: Alex Bennée <alex.bennee@linaro.org>
> Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register
>  changes
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > On Fri, Mar 01, 2024 at 10:22:08AM +0000, Alex Bennée wrote:
> >> Date: Fri, 01 Mar 2024 10:22:08 +0000
> >> From: Alex Bennée <alex.bennee@linaro.org>
> >> Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register
> >>  changes
> >> 
> >> Zhao Liu <zhao1.liu@intel.com> writes:
> >> 
> >> > Hi Alex,
> >> >
> >> > I hit the following warnings (with "./configure --enable-werror"):
> >> >
> >> > /qemu/contrib/plugins/execlog.c: In function ‘registers_init’:
> >> > /qemu/contrib/plugins/execlog.c:330:17: warning: 
> >> > ‘g_pattern_match_string’ is deprecated: Use 
> >> > 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations]
> >> >   330 |                 if (g_pattern_match_string(pat, rd->name) ||
> >> >       |                 ^~
> >> > In file included from /usr/include/glib-2.0/glib.h:65,
> >> >                  from /qemu/contrib/plugins/execlog.c:9:
> >> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here
> >> >    55 | gboolean      g_pattern_match_string   (GPatternSpec *pspec,
> >> >       |               ^~~~~~~~~~~~~~~~~~~~~~
> >> > /qemu/contrib/plugins/execlog.c:331:21: warning: 
> >> > ‘g_pattern_match_string’ is deprecated: Use 
> >> > 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations]
> >> >   331 |                     g_pattern_match_string(pat, rd_lower)) {
> >> >       |                     ^~~~~~~~~~~~~~~~~~~~~~
> >> > In file included from /usr/include/glib-2.0/glib.h:65,
> >> >                  from /qemu/contrib/plugins/execlog.c:9:
> >> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here
> >> >    55 | gboolean      g_pattern_match_string   (GPatternSpec *pspec,
> >> >       |               ^~~~~~~~~~~~~~~~~~~~~~
> >> > /qemu/contrib/plugins/execlog.c:339:63: warning: passing argument 2 of 
> >> > ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target type 
> >> > [-Wdiscarded-qualifiers]
> >> >   339 |                             g_ptr_array_add(all_reg_names,
> >> > reg->name);
> >> 
> >> Hmm I missed that. Not sure what the neatest solution is in this case -
> >> g_ptr_array_new() doesn't have a destroy func so we shouldn't ever
> >> attempt to free it. We can manually re-add the const qualifier at the
> >> other end for completeness and I guess comment and cast?
> >
> > I find other palces use 2 ways:
> >   * Use g_strdup() to create a copy (e.g., net/net.c,
> >     add_nic_model_help()). But I'm not sure if this is OK since you said
> >     we shouldn't attempt to free it. May I ask if the free issue you
> >     mentioned will affect the use of g_strdup() here?
> 
> If we g_strdup we have to remember to free later and ensure the
> g_ptr_array has a free func.

Got it. Thanks!
 
> >   * Another way is the forced conversion to gpointer (also e.g., in
> >     net/net.c, qemu_get_nic_models()).
> 
> I think this is ok, but with a comment on all_reg_names just to remind
> any potential users that the strings are const and allocated by QEMU so
> are not to be modified or freed.
> 

Yes, this makes sense.

> > Which way do you like? ;-)
> >
> <snip>
> >> 
> >> but I hesitated to add it for this case as plugins shouldn't assume they
> >> have access to QEMU's internals. Maybe the glib-compat.h header could be
> >> treated as a special case.
> >
> > Thanks! This works on my side!
> >
> > I support to fix the compatibility as the above, after all it's always
> > confusing when we allow users to use newer glib and see warnings at
> > compile time!
> >
> >> >
> >> > I also noticed in target/arm/helper.c, there's another
> >> > g_pattern_match_string() but I haven't met the warning.
> >> 
> >> Hmm that's weird. I suspect glib suppresses the warnings with:
> >> 
> >>   /* Ask for warnings for anything that was marked deprecated in
> >>    * the defined version, or before. It is a candidate for rewrite.
> >>    */
> >>   #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56
> >>
> >
> > I'm not too familiar with the QEMU build framework, but based on this, a
> > natural question is, can this rule be applied to plugins code as well?
> > If so, this would also avoid warning.
> 
> I think the simplest solution would be to add:
> 
>   #include "glib-compat.h"
> 
> into qemu-plugin.h so plugins have the same deprecation rules as the
> QEMU they come from. I checked with Michael on IRC and Debian currently
> doesn't package any header files but if anyone starts shipping a qemu-dev
> package we'll need to make sure we include glib-compat.h in the same
> directory and qemu-plugin.h.

I teseted this way, and met these error and warnings:

$ make -j16
changing dir to build for make ""...
make[1]: Entering directory '/qemu/build'
In file included from /qemu/contrib/plugins/execlog.c:16:
/qemu/contrib/plugins/../../include/qemu/qemu-plugin.h:19:10: fatal error: 
glib-compat.h: No such file or directory
   19 | #include "glib-compat.h"
      |          ^~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [Makefile:49: execlog.o] Error 1
make[1]: *** [Makefile:182: contrib/plugins/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[1/2014] Compiling C object tests/plugin/libempty.so.p/empty.c.o
FAILED: tests/plugin/libempty.so.p/empty.c.o
cc -m64 -mcx16 -Itests/plugin/libempty.so.p -Itests/plugin -I../tests/plugin 
-I../include/qemu -I/usr/include/glib-2.0 
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall 
-Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body 
-Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k 
-Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self 
-Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs 
-Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local 
-Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings 
-Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem 
/qemu/linux-headers -isystem linux-headers -iquote . -iquote /qemu -iquote 
/qemu/include -iquote /qemu/host/include/x86_64 -iquote 
/qemu/host/include/generic -iquote /qemu/tcg/i386 -pthread -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common 
-fwrapv -fzero-call-used-regs=used-gpr -fPIC -MD -MQ 
tests/plugin/libempty.so.p/empty.c.o -MF tests/plugin/libempty.so.p/empty.c.o.d 
-o tests/plugin/libempty.so.p/empty.c.o -c ../tests/plugin/empty.c
In file included from ../include/qemu/qemu-plugin.h:19,
                 from ../tests/plugin/empty.c:14:
/qemu/include/glib-compat.h:22: error: "GLIB_VERSION_MIN_REQUIRED" redefined 
[-Werror]
   22 | #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56
      |
In file included from /usr/include/glib-2.0/glib/gtypes.h:34,
                 from /usr/include/glib-2.0/glib/galloca.h:32,
                 from /usr/include/glib-2.0/glib.h:30,
                 from ../include/qemu/qemu-plugin.h:14,
                 from ../tests/plugin/empty.c:14:
/usr/include/glib-2.0/glib/gversionmacros.h:335: note: this is the location of 
the previous definition
  335 | # define GLIB_VERSION_MIN_REQUIRED      (GLIB_VERSION_CUR_STABLE)
      |
In file included from ../include/qemu/qemu-plugin.h:19,
                 from ../tests/plugin/empty.c:14:
/qemu/include/glib-compat.h:27: error: "GLIB_VERSION_MAX_ALLOWED" redefined 
[-Werror]
   27 | #define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_56
      |
In file included from /usr/include/glib-2.0/glib/gtypes.h:34,
                 from /usr/include/glib-2.0/glib/galloca.h:32,
                 from /usr/include/glib-2.0/glib.h:30,
                 from ../include/qemu/qemu-plugin.h:14,
                 from ../tests/plugin/empty.c:14:
/usr/include/glib-2.0/glib/gversionmacros.h:364: note: this is the location of 
the previous definition
  364 | # define GLIB_VERSION_MAX_ALLOWED      (GLIB_VERSION_CUR_STABLE)
      |
cc1: all warnings being treated as errors


For redefination warings, I think it's because qemu-plugin.h has
included <glib.h>...so it seems this way is still not clean.

As a comparison, it may be that we also have to go with
g_pattern_spec_match_string_qemu(), which also avoids the potential
problems you mentioned with the possible qemu-dev package.

Will you send the cleanups for those warnings? If you're pressed for
time, I'd be happy to help you send them. ;-)

Thanks,
Zhao