diff mbox

[RFA] Fix a (one shot small) leak in language.c

Message ID 20181204224802.8846-1-philippe.waroquiers@skynet.be
State New
Headers show

Commit Message

Philippe Waroquiers Dec. 4, 2018, 10:48 p.m. UTC
Valgrind detects the following leak:
==28395== VALGRIND_GDB_ERROR_BEGIN
==28395== 5 bytes in 1 blocks are definitely lost in loss record 20 of 2,770
==28395==    at 0x4C2BE2D: malloc (vg_replace_malloc.c:299)
==28395==    by 0x41D9E7: xmalloc (common-utils.c:44)
==28395==    by 0x78BF39: xstrdup (xstrdup.c:34)
==28395==    by 0x51F1AC: _initialize_language() (language.c:1175)
==28395==    by 0x6B3356: initialize_all_files() (init.c:308)
==28395==    by 0x66D194: gdb_init(char*) (top.c:2159)
==28395==    by 0x554C11: captured_main_1 (main.c:863)
==28395==    by 0x554C11: captured_main (main.c:1167)
==28395==    by 0x554C11: gdb_main(captured_main_args*) (main.c:1193)
==28395==    by 0x29D837: main (gdb.c:32)
==28395==
==28395== VALGRIND_GDB_ERROR_END

This is a very small leak (1 block/5 bytes), happening only once
per GDB startup as far as I can see. But this fix make the nr of leaking
GDB in the testsuite decreasing from 628 to 566.

It is unclear why a xstrdup-ed value is assigned to 'language'
at initialization time, while a static "auto" string is assigned
as part of the set_language_command.
So, that shows that it is ok to initialize 'language' directly
with "auto".
Also, I cannot find any place where 'language' is xfree-d.
No leak was detected for 'range' and 'case_sensitive', but
similarly, no indication why a static string cannot be assigned.

Regression-tested on debian/x86_64.
Also, full testsuite run under valgrind, less tests leaking,
and no dangling pointer problem detected.

gdb/ChangeLog
2018-12-04  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* language.c (_initialize_language): Fix leak by assigning
	a static string to language.  Same for range and case_sensitive,
	even if no leak is detected for these variables.
---
 gdb/language.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Tom Tromey Dec. 5, 2018, 6:06 p.m. UTC | #1
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> It is unclear why a xstrdup-ed value is assigned to 'language'
Philippe> at initialization time, while a static "auto" string is assigned
Philippe> as part of the set_language_command.

git annotate shows it is ancient, so it's hard to say -- maybe things
were different back then or maybe it's just an ancient bug.

Philippe> 	* language.c (_initialize_language): Fix leak by assigning
Philippe> 	a static string to language.  Same for range and case_sensitive,
Philippe> 	even if no leak is detected for these variables.

I think technically the value should come from the array passed to
add_setshow_enum_cmd, but in practice that only matters if the values
are compared using ==, which is not the case here; and in any case the
current way (comparing using strcmp) seems more clear and thus
preferable.

So, this is ok.  Thanks.

Tom
Philippe Waroquiers Dec. 7, 2018, 3:48 p.m. UTC | #2
On Wed, 2018-12-05 at 11:06 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> It is unclear why a xstrdup-ed value is assigned to 'language'
> Philippe> at initialization time, while a static "auto" string is assigned
> Philippe> as part of the set_language_command.
> 
> git annotate shows it is ancient, so it's hard to say -- maybe things
> were different back then or maybe it's just an ancient bug.
I think that this was needed a long time ago: there was no enum
to store the language/range/case_sensitive value. Instead, the value was
stored as a dynamically allocated string.
The xstrdup became useless when the enums were introduced.

> 
> Philippe> 	* language.c (_initialize_language): Fix leak by assigning
> Philippe> 	a static string to language.  Same for range and case_sensitive,
> Philippe> 	even if no leak is detected for these variables.
> 
> I think technically the value should come from the array passed to
> add_setshow_enum_cmd, but in practice that only matters if the values
> are compared using ==, which is not the case here; and in any case the
> current way (comparing using strcmp) seems more clear and thus
> preferable.
> 
> So, this is ok.  Thanks.
Thanks for the review, pushed.

Philippe
diff mbox

Patch

diff --git a/gdb/language.c b/gdb/language.c
index e2f400161b..0ec61c3f5a 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -1172,9 +1172,9 @@  For Fortran the default is off; for other languages the default is on."),
 
   add_set_language_command ();
 
-  language = xstrdup ("auto");
-  range = xstrdup ("auto");
-  case_sensitive = xstrdup ("auto");
+  language = "auto";
+  range = "auto";
+  case_sensitive = "auto";
 
   /* Have the above take effect.  */
   set_language (language_auto);