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).
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.
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?
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..27c7baa61bb 100644
--- a/gcc/c-family/c-warn.c
+++ b/gcc/c-family/c-warn.c
@@ -3636,6 +3636,10 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms)
inform (&richloc, "previously declared as %s with bound "
"%<%s%>", curparmstr.c_str (), curbndstr);
+ if (newbnd)
+ free (const_cast <char *>(newbndstr));
+ if (curbnd)
+ free (const_cast <char *>(curbndstr));
continue;
}
}
@@ -3646,7 +3650,13 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms)
Compare them lexicographically to detect gross mismatches
such as between T[foo()] and T[bar()]. */
if (operand_equal_p (newbnd, curbnd, OEP_LEXICOGRAPHIC))
- continue;
+ {
+ if (newbnd)
+ free (const_cast <char *>(newbndstr));
+ if (curbnd)
+ free (const_cast <char *>(curbndstr));
+ continue;
+ }
if (warning_at (newloc, OPT_Wvla_parameter,
"argument %u of type %s declared with "
@@ -3654,8 +3664,18 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms)
parmpos + 1, newparmstr.c_str (), newbndstr))
inform (origloc, "previously declared as %s with bound %qs",
curparmstr.c_str (), curbndstr);
+
+ if (newbnd)
+ free (const_cast <char *>(newbndstr));
+ if (curbnd)
+ free (const_cast <char *>(curbndstr));
continue;
}
+
+ if (newbnd)
+ free (const_cast <char *>(newbndstr));
+ if (curbnd)
+ free (const_cast <char *>(curbndstr));
}
if (newa->minsize == cura->minsize
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)
