On Thu, Feb 11, 2021 at 7:35 PM Martin Sebor <mse...@gmail.com> wrote:
>
> On 2/11/21 12:59 AM, Richard Biener wrote:
> > On Wed, Feb 10, 2021 at 6:16 PM Martin Sebor <mse...@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
>
