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

[Martin Sebor <msebor@gmail.com>]

On 2/12/21 12:36 PM, Richard Biener wrote:
> On February 12, 2021 7:21:25 PM GMT+01:00, Martin Sebor <msebor@gmail.com> wrote:
>> 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.)
> 
> But the issue with std::string is that people will have to use .c_str() because of the gazillion C style interfaces in GCC
> and storage of std::string will eventually lead to lots of copying or use the other very same use after free or leak issues.
> 
> std::string isn't the great solution you are presenting it as.

It's the standard solution, but it isn't the only one.  If we don't
want to use it we can create our own, improved class (I did mention
auto_vec).  I'm only advocating the use of RAII to avoid these hunts
for memory leaks that can be trivially avoided by adopting basic C++
idioms.

Martin

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