On February 12, 2021 7:21:25 PM GMT+01:00, Martin Sebor <mse...@gmail.com>
wrote:
>On 2/12/21 12:35 AM, Richard Biener wrote:
>> 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;
>> };
>
>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.
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
>>>
