On 2/12/21 12:36 PM, Richard Biener wrote:
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.
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
