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

[Martin Sebor]

On 2/12/21 12:35 AM, Richard Biener wrote:
> 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;
> };

This wouldn't be a good general solution either (in fact, I'd say
it would make it worse) because the string's lifetime is tied to
the lifetime of the class object, turning memory leaks to uses-
after-free.  Consider:

   const char *str = generic_expr_as_str (node);
   ...
   warning ("node = %s", str);   // str is dangling/invalid

(Trying to "fix" it by replacing the conversion operator with a named
member function like str() doesn't solve the problem.)

> 
> 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.

Okay.

> 
> 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?

I agree that print_generic_expr_to_str() could be improved.  But
a simple API that returns a string representation of a tree node
needs to be easy to use safely and correctly and hard to misuse.
It shouldn't require its clients to explicitly manage the lifetimes
of multiple objects.

But this isn't a new problem, and the solution is as old as C++
itself: have the API return an object of an RAII class like
std::string, or more generally something like std::unique_ptr.
In this case it could even be GCC's auto_vec<char*>, or (less
user-friendly) a garbage collected STRING_CST.

> 
> 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!

Avoiding the xstrdup sounds good to me.  I've changed the code to
use the pretty_printer directly and committed the attached patch.

Martin

> 
>>>
>>>> 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
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-99055.diff
Type: text/x-patch
Size: 2164 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20210212/2e7129e7/attachment-0001.bin>