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

[Zhao Liu]

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?
  * Another way is the forced conversion to gpointer (also e.g., in
    net/net.c, qemu_get_nic_models()).

Which way do you like? ;-)

> 
> 
> >       |                                                            ~~~^~~~~~
> > In file included from /usr/include/glib-2.0/glib.h:31,
> >                  from /qemu/contrib/plugins/execlog.c:9:
> > /usr/include/glib-2.0/glib/garray.h:192:62: note: expected ‘gpointer’ {aka 
> > ‘void *’} but argument is of type ‘const char *’
> >   192 |                                            gpointer          data);
> >       |                                            ~~~~~~~~~~~~~~~~~~^~~~
> >
> > In addition, I checked my glic version:
> >
> > $ldd --version
> > ldd (Ubuntu GLIBC 2.35-0ubuntu3.5) 2.35
> >
> > I think it's v2.35. Are these three warning reports valid?
> 
> It's the glib (not glibc) version that matters here.
> g_pattern_match_string was deprecated in 2.70 when the suggested
> alternative was added. However our baseline for glib is still:
> 
>   # When bumping glib minimum version, please check also whether to increase
>   # the _WIN32_WINNT setting in osdep.h according to the value from glib
>   glib_req_ver = '>=2.56.0'
>   glib_pc = dependency('glib-2.0', version: glib_req_ver, required: true,
>                       method: 'pkg-config')
> 
> The usual solution for this is to throw in a compat wrapper in
> glib-compat.h:
> 
> --8<---------------cut here---------------start------------->8---
> modified   include/glib-compat.h
> @@ -105,6 +105,24 @@ static inline gpointer g_memdup2_qemu(gconstpointer mem, 
> gsize byte_size)
>  }
>  #define g_memdup2(m, s) g_memdup2_qemu(m, s)
>  
> +/*
> + * g_pattern_match_string has been deprecated in Glib since 2.70 and
> + * will complain about it if you try to use it. Fortunately the
> + * signature of both functions is the same making it easy to work
> + * around.
> + */
> +static inline
> +gboolean g_pattern_spec_match_string_qemu(GPatternSpec *pspec,
> +                                          const gchar *string)
> +{
> +#if GLIB_CHECK_VERSION(2, 70, 0)
> +    return g_pattern_spec_match_string(pspec, string);
> +#else
> +    return g_pattern_match_string(pspec, string);
> +#endif
> +};
> +#define g_pattern_spec_match_string(p, s) 
> g_pattern_spec_match_string_qemu(p, s)
> +
>  #if defined(G_OS_UNIX)
>  /*
>   * Note: The fallback implementation is not MT-safe, and it returns a copy of
> modified   contrib/plugins/execlog.c
> @@ -334,8 +334,8 @@ static void registers_init(int vcpu_index)
>              for (int p = 0; p < rmatches->len; p++) {
>                  g_autoptr(GPatternSpec) pat = 
> g_pattern_spec_new(rmatches->pdata[p]);
>                  g_autofree gchar *rd_lower = g_utf8_strdown(rd->name, -1);
> -                if (g_pattern_match_string(pat, rd->name) ||
> -                    g_pattern_match_string(pat, rd_lower)) {
> +                if (g_pattern_spec_match_string(pat, rd->name) ||
> +                    g_pattern_spec_match_string(pat, rd_lower)) {
>                      Register *reg = init_vcpu_register(vcpu_index, rd);
>                      g_ptr_array_add(registers, reg);
> --8<---------------cut here---------------end--------------->8---
> 
> 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.

Thanks,
Zhao