Message ID | 20181204224802.8846-1-philippe.waroquiers@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 116496 invoked by alias); 4 Dec 2018 22:49:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 115941 invoked by uid 89); 4 Dec 2018 22:48:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS, TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=startup, HContent-Transfer-Encoding:8bit X-HELO: mailsec111.isp.belgacom.be Received: from mailsec111.isp.belgacom.be (HELO mailsec111.isp.belgacom.be) (195.238.20.107) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Dec 2018 22:48:21 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1543963690; x=1575499690; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=6EC9GJDHC+lOFv7t4Hh2KjiGZCfJfKUtj1vTCn7Rc40=; b=DOlPMCbtY3VtOprXap4A6O+pQx9SgiBUdIqPfVszx0XyDzoI9pd9LAHn 51lsnenrB8ZfOo20ae+aS63wyb7FGg==; Received: from 184.205-67-87.adsl-dyn.isp.belgacom.be (HELO md.home) ([87.67.205.184]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 04 Dec 2018 23:48:07 +0100 From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> Subject: [RFA] Fix a (one shot small) leak in language.c Date: Tue, 4 Dec 2018 23:48:02 +0100 Message-Id: <20181204224802.8846-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
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
>>>>> "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
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 --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);