Message ID | 20181104203949.24160-1-philippe.waroquiers@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 9338 invoked by alias); 4 Nov 2018 20:39:58 -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 9323 invoked by uid 89); 4 Nov 2018 20:39:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mailsec109.isp.belgacom.be Received: from mailsec109.isp.belgacom.be (HELO mailsec109.isp.belgacom.be) (195.238.20.105) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 04 Nov 2018 20:39:57 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1541363998; x=1572899998; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=cNp+ZrMoCveuWifZS8gUWuPpmyhGf3JVS/X6+4JtER8=; b=aW16n9MjzxVCDHg9mWwtZeLAdUBOaUR8FyEjuNQLEaZbsI9DSo/tMHmA rg1MXTazlHTEi2/bBb7U9ppRC3/6xQ==; Received: from 110.212-243-81.adsl-dyn.isp.belgacom.be (HELO md.home) ([81.243.212.110]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 04 Nov 2018 21:39:55 +0100 From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> Subject: [RFA] Fix leak in displaced step. Date: Sun, 4 Nov 2018 21:39:49 +0100 Message-Id: <20181104203949.24160-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
Nov. 4, 2018, 8:39 p.m. UTC
Valgrind reports a definite leak of displaced->step_saved_copy (full leak stack trace below). This patch fixes the leak by calling xfree (displaced->step_saved_copy) in displaced_step_clear. ==4736== VALGRIND_GDB_ERROR_BEGIN ==4736== 128 bytes in 8 blocks are definitely lost in loss record 980 of 3,108 ==4736== at 0x4C2BE2D: malloc (vg_replace_malloc.c:299) ==4736== by 0x41B627: xmalloc (common-utils.c:44) ==4736== by 0x50D4E3: displaced_step_prepare_throw (infrun.c:1837) ==4736== by 0x50D4E3: displaced_step_prepare (infrun.c:1898) ==4736== by 0x50D4E3: resume_1 (infrun.c:2545) ==4736== by 0x50D4E3: resume(gdb_signal) (infrun.c:2741) ==4736== by 0x50DCD5: keep_going_pass_signal(execution_control_state*) (infrun.c:7793) ==4736== by 0x50E903: process_event_stop_test(execution_control_state*) (infrun.c:6843) ==4736== by 0x510925: handle_signal_stop(execution_control_state*) (infrun.c:6176) ==4736== by 0x513F79: handle_inferior_event_1 (infrun.c:5354) ==4736== by 0x513F79: handle_inferior_event(execution_control_state*) (infrun.c:5389) ==4736== by 0x51541B: fetch_inferior_event(void*) (infrun.c:3916) ==4736== by 0x4B3EEC: gdb_wait_for_event(int) (event-loop.c:859) ==4736== by 0x4B3FF6: gdb_do_one_event() [clone .part.4] (event-loop.c:322) ==4736== by 0x4B41B4: gdb_do_one_event (common-exceptions.h:219) ==4736== by 0x4B41B4: start_event_loop() (event-loop.c:371) ==4736== by 0x551237: captured_command_loop() (main.c:330) ==4736== by 0x55222C: captured_main (main.c:1177) ==4736== by 0x55222C: gdb_main(captured_main_args*) (main.c:1193) ==4736== by 0x29B4F7: main (gdb.c:32) ==4736== ==4736== VALGRIND_GDB_ERROR_END gdb/ChangeLog 2018-11-04 Philippe Waroquiers <philippe.waroquiers@skynet.be> * infrun.c (displaced_step_clear): Fix leak by xfree-ing displaced->step_saved_copy. --- gdb/infrun.c | 6 ++++++ 1 file changed, 6 insertions(+)
Comments
On 2018-11-04 3:39 p.m., Philippe Waroquiers wrote: > Valgrind reports a definite leak of displaced->step_saved_copy > (full leak stack trace below). > > This patch fixes the leak by calling xfree (displaced->step_saved_copy) > in displaced_step_clear. > > ==4736== VALGRIND_GDB_ERROR_BEGIN > ==4736== 128 bytes in 8 blocks are definitely lost in loss record 980 of 3,108 > ==4736== at 0x4C2BE2D: malloc (vg_replace_malloc.c:299) > ==4736== by 0x41B627: xmalloc (common-utils.c:44) > ==4736== by 0x50D4E3: displaced_step_prepare_throw (infrun.c:1837) > ==4736== by 0x50D4E3: displaced_step_prepare (infrun.c:1898) > ==4736== by 0x50D4E3: resume_1 (infrun.c:2545) > ==4736== by 0x50D4E3: resume(gdb_signal) (infrun.c:2741) > ==4736== by 0x50DCD5: keep_going_pass_signal(execution_control_state*) (infrun.c:7793) > ==4736== by 0x50E903: process_event_stop_test(execution_control_state*) (infrun.c:6843) > ==4736== by 0x510925: handle_signal_stop(execution_control_state*) (infrun.c:6176) > ==4736== by 0x513F79: handle_inferior_event_1 (infrun.c:5354) > ==4736== by 0x513F79: handle_inferior_event(execution_control_state*) (infrun.c:5389) > ==4736== by 0x51541B: fetch_inferior_event(void*) (infrun.c:3916) > ==4736== by 0x4B3EEC: gdb_wait_for_event(int) (event-loop.c:859) > ==4736== by 0x4B3FF6: gdb_do_one_event() [clone .part.4] (event-loop.c:322) > ==4736== by 0x4B41B4: gdb_do_one_event (common-exceptions.h:219) > ==4736== by 0x4B41B4: start_event_loop() (event-loop.c:371) > ==4736== by 0x551237: captured_command_loop() (main.c:330) > ==4736== by 0x55222C: captured_main (main.c:1177) > ==4736== by 0x55222C: gdb_main(captured_main_args*) (main.c:1193) > ==4736== by 0x29B4F7: main (gdb.c:32) > ==4736== > ==4736== VALGRIND_GDB_ERROR_END > > gdb/ChangeLog > > 2018-11-04 Philippe Waroquiers <philippe.waroquiers@skynet.be> > > * infrun.c (displaced_step_clear): Fix leak by xfree-ing > displaced->step_saved_copy. I assume you have regtested this? If so, LGTM. Thanks, Simon
On Fri, 2018-11-09 at 22:19 +0000, Simon Marchi wrote: > > 2018-11-04 Philippe Waroquiers <philippe.waroquiers@skynet.be> > > > > * infrun.c (displaced_step_clear): Fix leak by xfree-ing > > displaced->step_saved_copy. > > I assume you have regtested this? If so, LGTM. Yes, before submitting a patch, I run the gdb tests (including gdb.ada) and (try to) detect regressions by comparing with an untouched master build. I also re-run all tests once more before pushing, with which I now see one more 'unresolved testcase' in step-over-syscall.exp, which is clearly a regression I did not detect initially :(. So, I will further investigate, and submit another patch once I understand how to properly fix this leak. Thanks for looking at the patch, sorry for this wrong fix ... Philippe FYI, I am intending to run the full gdb testsuite under valgrind, and analyse the reported problems. Currently, there are only 2 remaining definite leaks seen when running a few small tests: * this leak (which is triggered for each step/next I think) * a 'one shot initialization' leak of 2 small blocksĀ 'ui::ui(_IO_FILE*, _IO_FILE*, _IO_FILE*)' There are many 'possibly lost' leaks in Python, which I guess are normal. There is also a bunch of possibly lost blocks created by the arch init code (seems to be 'one shot initialization' leaks also, unclear to me if these are normal). In summary, GDB seems reasonably 'leak free'. Then there were a bunch of uninitialized memory usage (conditional jumps, or used in syscalls) in various tests, but the whole testsuite came to a halt due to the 'lingering processes' problem, see other patch. So, no risk of having nothing to do during the week-end :)
On 2018-11-10 06:52, Philippe Waroquiers wrote: > On Fri, 2018-11-09 at 22:19 +0000, Simon Marchi wrote: >> > 2018-11-04 Philippe Waroquiers <philippe.waroquiers@skynet.be> >> > >> > * infrun.c (displaced_step_clear): Fix leak by xfree-ing >> > displaced->step_saved_copy. >> >> I assume you have regtested this? If so, LGTM. > Yes, before submitting a patch, I run the gdb tests (including gdb.ada) > and (try to) detect regressions by comparing with an untouched master > build. > I also re-run all tests once more before pushing, > with which I now see one more 'unresolved testcase' in > step-over-syscall.exp, which is clearly a regression I did not > detect initially :(. > > So, I will further investigate, and submit another patch once I > understand how to properly fix this leak. > > Thanks for looking at the patch, sorry for this wrong fix ... > > Philippe > > FYI, I am intending to run the full gdb testsuite under valgrind, > and analyse the reported problems. > Currently, there are only 2 remaining definite leaks seen when > running a few small tests: > * this leak (which is triggered for each step/next I think) > * a 'one shot initialization' leak of 2 small blocksĀ > 'ui::ui(_IO_FILE*, _IO_FILE*, _IO_FILE*)' > > There are many 'possibly lost' leaks in Python, which I guess are > normal. > There is also a bunch of possibly lost blocks created by the arch init > code > (seems to be 'one shot initialization' leaks also, unclear to me > if these are normal). > In summary, GDB seems reasonably 'leak free'. > > Then there were a bunch of uninitialized memory usage (conditional > jumps, or used in syscalls) in various tests, but the whole testsuite > came to a halt due to the 'lingering processes' problem, see other > patch. > > So, no risk of having nothing to do during the week-end :) Thank you very much for doing this! Simon
diff --git a/gdb/infrun.c b/gdb/infrun.c index 9473d1f20f..526ad2acb1 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1709,6 +1709,12 @@ displaced_step_clear (struct displaced_step_inferior_state *displaced) delete displaced->step_closure; displaced->step_closure = NULL; + + if (displaced->step_saved_copy != NULL) + { + xfree (displaced->step_saved_copy); + displaced->step_saved_copy = NULL; + } } static void