diff mbox

[RFA] Fix leak in displaced step.

Message ID 20181104203949.24160-1-philippe.waroquiers@skynet.be
State New, archived
Headers

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

Simon Marchi Nov. 9, 2018, 10:19 p.m. UTC | #1 
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
Philippe Waroquiers Nov. 10, 2018, 11:52 a.m. UTC | #2 
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 :)
Simon Marchi Nov. 10, 2018, 5:42 p.m. UTC | #3 
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 mbox

Patch

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