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.)
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
PR c/99055 - memory leak in warn_parm_array_mismatch
gcc/c-family/ChangeLog:
PR c/99055
* c-warn.c (warn_parm_array_mismatch): Free strings returned from
print_generic_expr_to_str.
gcc/ChangeLog:
* tree-pretty-print.c (print_generic_expr_to_str): Update comment.
diff --git a/gcc/c-family/c-warn.c b/gcc/c-family/c-warn.c
index e6e28d9b139..2347e0b2e5d 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -3319,6 +3319,19 @@ warn_parm_ptrarray_mismatch (location_t origloc, tree curparms, tree newparms)
}
}
+/* Format EXPR if nonnull and return the formatted string. If EXPR is
+ null return DFLT. */
+
+static inline const char*
+expr_to_str (pretty_printer &pp, tree expr, const char *dflt)
+{
+ if (!expr)
+ return dflt;
+
+ dump_generic_node (&pp, expr, 0, TDF_VOPS | TDF_MEMSYMS, false);
+ return pp_formatted_text (&pp);
+}
+
/* Detect and diagnose a mismatch between an attribute access specification
on the original declaration of FNDECL and that on the parameters NEWPARMS
from its refeclaration. ORIGLOC is the location of the first declaration
@@ -3585,10 +3598,9 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms)
the same. */
continue;
- const char* const newbndstr =
- newbnd ? print_generic_expr_to_str (newbnd) : "*";
- const char* const curbndstr =
- curbnd ? print_generic_expr_to_str (curbnd) : "*";
+ pretty_printer pp1, pp2;
+ const char* const newbndstr = expr_to_str (pp1, newbnd, "*");
+ const char* const curbndstr = expr_to_str (pp2, curbnd, "*");
if (!newpos != !curpos
|| (newpos && !tree_int_cst_equal (newpos, curpos)))
diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
index aabe6bb23b9..986f75d1d5f 100644
--- a/gcc/tree-pretty-print.c
+++ b/gcc/tree-pretty-print.c
@@ -169,7 +169,8 @@ print_generic_expr (FILE *file, tree t, dump_flags_t flags)
pp_flush (tree_pp);
}
-/* Print a single expression T to string, and return it. */
+/* Print a single expression T to string, and return it. The caller
+ must free the returned memory. */
char *
print_generic_expr_to_str (tree t)
