Re: Re: [PATCH] plug memory leaks in warn_parm_array_mismatch (PR 99055)

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Feb 11, 2021 at 7:35 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 2/11/21 12:59 AM, Richard Biener wrote:
> > On Wed, Feb 10, 2021 at 6:16 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> The attached patch replaces calls to print_generic_expr_to_str() with
> >> a helper function that returns a std::string and releases the caller
> >> from the responsibility to explicitly free memory.
> >
> > I don't like this.  What's the reason to use generic_expr_as_string
> > anyway?  We have %E pretty-printers after all.
>
> [Reposting; my first reply was too big.]
>
> The VLA bound is either an expression or the asterisk (*) when newbnd
> is null.  The reason for using the function is to avoid duplicating
> the warning call and making one with %E and another with "*".
>
> > Couldn't you have
> > fixed the leak by doing
> >
> > if (newbnd)
> >    free (newbndstr);
> >
> > etc.?
>
> Yes, I could have.  I considered it and chose not to because this
> is much simpler, safer (if newbnd were to change after newbndstr
> is set), and more in the "C++ spirit" (RAII).  This code already
> uses std::string (attr_access::array_as_string) and so my change
> is in line with it.
>
> I understand that you prefer a more C-ish coding style so if you
> consider it a prerequisite for accepting this fix I can make
> the change conform to it.  See the attached revision (although
> I hope you'll see why I chose not to go this route).

I can understand but I still disagree ;)

> For what it's worth, print_generic_expr_to_str() would be improved
> by returning std::string.  It would mean including <string> in
> most sources in the compiler, which I suspect may not be a popular
> suggestion with everyone here, and which is why I didn't make it
> to begin with.  But if you're open to it I'm happy to make that
> change either for GCC 12 or even now.

Well, looking at print_generic_expr_to_str I see

/* Print a single expression T to string, and return it.  */

char *
print_generic_expr_to_str (tree t)
{
  pretty_printer pp;
  dump_generic_node (&pp, t, 0, TDF_VOPS|TDF_MEMSYMS, false);
  return xstrdup (pp_formatted_text (&pp));
}

which makes me think that this instead should be sth like

class generic_expr_as_str : public pretty_printer
{
   generic_expr_as_str (tree t) { dump_generic_node (&pp, t, 0,
TDF_VOPS|TDF_MEMSYMS, false); }
   operator const char *() { return pp_formatted_text (&pp); }
   pretty_printer pp;
};

As we do seem to have a RAII pretty-printer class already.  That said,
I won't mind using

   pretty_printer pp;
   dump_generic_node (&pp, t, 0, TDF_VOPS|TDF_MEMSYMS, false);

and pp_formatted_text () in the users either.

That is, print_generic_expr_to_str looks just like a bad designed hack.
Using std::string there doesn't make it any better.

Don't you agree to that?

So your 2nd variant patch is OK but you might consider just using
a pretty-printer manually and even do away with the xstrdup in that
way entirely!

> >
> >> With the patch installed, Valgrind shows more leaks in this code that
> >> I'm not sure what to do about:
> >>
> >> 1) A tree built by build_type_attribute_qual_variant() called from
> >>      attr_access::array_as_string() to build a temporary type only
> >>      for the purposes of formatting it.
> >>
> >> 2) A tree (an attribute list) built by tree_cons() called from
> >>      build_attr_access_from_parms() that's used only for the duration
> >>      of the caller.
> >>
> >> Do these temporary trees need to be released somehow or are the leaks
> >> expected?
> >
> > You should configure GCC with --enable-valgrind-annotations to make
> > it aware of our GC.
>
> I did configure with that option:
>
> $ /src/gcc/master/configure --enable-checking=yes
> --enable-languages=all,jit,lto --enable-host-shared
> --enable-valgrind-annotations
>
> Attached to pr99055 is my valgrind output for gcc.dg/Wvla-parameter.c
> with the top of trunk and the patch applied:
>
> $ /build/gcc-master/gcc/xgcc -B /build/gcc-master/gcc -S -Wall
> /src/gcc/master/gcc/testsuite/gcc.dg/Wvla-parameter.c -wrapper
> valgrind,--leak-check=full,--show-leak-kinds=all,--track-origins=yes,--log-file=valgrind-out.txt
>
> Do you not see the same leaks?

Err, well.  --show-leak-kinds=all is probably the cause.  We
definitely do not force-release
all reachable GC allocated memory at program end.  Not sure if
valgrind annotations can
make that obvious to valgrind.  I'm just using --leak-check=full and
thus look for
unreleased and no longer reachable memory.

Richard.

>
> Martin
>