[patch] Fix cleanup in finish_command

[patch] Fix cleanup in finish_command

[Jan Kratochvil]

Hi,

I was getting core files (despite GDB PASSes all tests) with the patchset
	[patch v3 00/23] record-btrace: reverse
	http://sourceware.org/ml/gdb-patches/2013-06/msg00214.html
	Message-Id: <1370851496-32313-1-git-send-email-markus.t.metzger@intel.com>

It probably comes from:
	finish^M
	Run till exit from #0  0x00000000004005b5 in bar () at gdb/testsuite/gdb.btrace/x86-tailcall.c:24^M
	0x00000000004005d5 in main () at gdb/testsuite/gdb.btrace/x86-tailcall.c:37^M
	37        answer = foo ();^M
	This record target does not trace registers.^M
	(gdb) PASS: gdb.btrace/tailcall.exp: step, 1.1

It dumps core even without -lmcheck.

(Regression test is still running here.)


Jan


#0  in delete_breakpoint (bpt=0x3ab0780) at breakpoint.c:13494
#1  in finish_command_continuation (arg=0x3a202c0, err=1) at infcmd.c:1593
#2  in do_my_continuations_1 (pmy_chain=0x7fff5c1c8178, err=1) at continuations.c:59
#3  in do_my_continuations (list=0x3916278, err=1) at continuations.c:83
#4  in do_all_continuations_ptid (ptid=..., continuations_p=0x3916278, err=1) at continuations.c:191
#5  in do_all_continuations_thread_callback (thread=0x3916190, data=0x7fff5c1c8254) at continuations.c:202
#6  in do_all_continuations_thread (thread=0x3916190, err=1) at continuations.c:211
#7  in clear_thread_inferior_resources (tp=0x3916190) at thread.c:123
#8  in delete_thread_1 (ptid=..., silent=0) at thread.c:296
#9  in delete_thread (ptid=...) at thread.c:313
#10 in delete_thread_of_inferior (tp=0x3916190, data=0x7fff5c1c83b0) at inferior.c:182
#11 in iterate_over_threads (callback=0x8805ba <delete_thread_of_inferior>, data=0x7fff5c1c83b0) at thread.c:370
#12 in exit_inferior_1 (inftoex=0x37e3620, silent=0) at inferior.c:260
#13 in exit_inferior (pid=8817) at inferior.c:289
#14 in generic_mourn_inferior () at target.c:3679
#15 in inf_ptrace_mourn_inferior (ops=0x368a8f0) at inf-ptrace.c:179
#16 in linux_nat_mourn_inferior (ops=0x368a8f0) at linux-nat.c:4101
#17 in target_mourn_inferior () at target.c:2836
#18 in linux_nat_kill (ops=0x368a8f0) at linux-nat.c:4089
#19 in target_kill () at target.c:482
#20 in record_kill (t=0x1f1fca0 <record_btrace_ops>) at record.c:185
#21 in target_kill () at target.c:482
#22 in kill_or_detach (inf=0x37e3620, args=0x7fff5c1c86d0) at top.c:1311
#23 in iterate_over_inferiors (callback=0x856ed9 <kill_or_detach>, data=0x7fff5c1c86d0) at inferior.c:396
#24 in quit_force (args=0x0, from_tty=0) at top.c:1419
#25 in quit_command (args=0x0, from_tty=0) at ./cli/cli-cmds.c:334
#26 in quit_cover () at top.c:298
#27 in async_disconnect (arg=0x0) at event-top.c:860
#28 in invoke_async_signal_handlers () at event-loop.c:993
#29 in process_event () at event-loop.c:320
#30 in gdb_do_one_event () at event-loop.c:406
#31 in start_event_loop () at event-loop.c:431
#32 in cli_command_loop () at event-top.c:177
#33 in current_interp_command_loop () at interps.c:331
#34 in captured_command_loop (data=0x0) at main.c:260
#35 in catch_errors (func=0x751bbb <captured_command_loop>, func_args=0x0, errstring=0xfdf1f4 "", mask=6) at exceptions.c:546
#36 in captured_main (data=0x7fff5c1c8a80) at main.c:1055
#37 in catch_errors (func=0x751e6e <captured_main>, func_args=0x7fff5c1c8a80, errstring=0xfdf1f4 "", mask=6) at exceptions.c:546
#38 in gdb_main (args=0x7fff5c1c8a80) at main.c:1064
#39 in main (argc=5, argv=0x7fff5c1c8b88) at gdb.c:34


gdb/
2013-06-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* infcmd.c (finish_forward): Move discard_cleanups earlier.

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 30621e4..4a32382 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1691,9 +1691,9 @@ finish_forward (struct symbol *function, struct frame_info *frame)
   cargs->function = function;
   add_continuation (tp, finish_command_continuation, cargs,
                     finish_command_continuation_free_arg);
+  discard_cleanups (old_chain);
   proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT, 0);
 
-  discard_cleanups (old_chain);
   if (!target_can_async_p ())
     do_all_continuations (0);
 }

Re: [patch] Fix cleanup in finish_command

[Jan Kratochvil]

On Wed, 19 Jun 2013 23:14:44 +0200, Jan Kratochvil wrote:
> I was getting core files (despite GDB PASSes all tests) with the patchset
> 	[patch v3 00/23] record-btrace: reverse
> 	http://sourceware.org/ml/gdb-patches/2013-06/msg00214.html
> 	Message-Id: <1370851496-32313-1-git-send-email-markus.t.metzger@intel.com>

BTW I have found the crash happens even with this patch, I haven't found the
real cause yet.


Jan

Re: [patch] Fix cleanup in finish_command

[Joel Brobecker]

> BTW I have found the crash happens even with this patch, I haven't found the
> real cause yet.

Interesting, as I couldn't understand the relationship between
the backtrace and the patch... You might also be in a situation
similar to what I faced on Darwin: a correct cleanup fix triggering
a latent bug; In that situation I found it useful to first git-bisect
to narrow down the commit that caused the change of behavior, and
then finish the bug off with valgrind's help.

-- 
Joel

RE: [patch] Fix cleanup in finish_command

[Metzger, Markus T]

> -----Original Message-----
> From: Joel Brobecker [mailto:brobecker@adacore.com]
> Sent: Thursday, June 20, 2013 5:18 PM


> > BTW I have found the crash happens even with this patch, I haven't found the
> > real cause yet.
> 
> Interesting, as I couldn't understand the relationship between
> the backtrace and the patch... You might also be in a situation
> similar to what I faced on Darwin: a correct cleanup fix triggering
> a latent bug; In that situation I found it useful to first git-bisect
> to narrow down the commit that caused the change of behavior, and
> then finish the bug off with valgrind's help.

The bisect will point somewhere into my patch series but it won't be
of much help since most of the patches are just preparing for the
enabling patch at the end of the series.

I have not seen those core files.
Can you point me to a specific test where this happens?  
Is there some indication in gdb.log?
Are you using remote or native configuration?


Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [patch] Fix cleanup in finish_command

[Jan Kratochvil]

On Thu, 20 Jun 2013 17:26:56 +0200, Metzger, Markus T wrote:
> Can you point me to a specific test where this happens?  

Each time with: gdb.btrace/tailcall.exp

Build gdb with -lmcheck just to be sure.

> Is there some indication in gdb.log?

No.

I get one FAIL but I expect it is due to buggy Nehalem:
next^M
0x00000000004005d5 in main () at gdb/testsuite/gdb.btrace/x86-tailcall.c:37^M
37        answer = foo ();^M
(gdb) FAIL: gdb.btrace/tailcall.exp: step, 1.5


> Are you using remote or native configuration?

Native, just: ulimit -c unlimited; runtest gdb.btrace/tailcall.exp


Pedro already gave an advice how to fix it properly if you take over this bug.


Unrelated:
There is testcase FAILing randomly, reproducible with "read1.so" from:
	http://sourceware.org/bugzilla/show_bug.cgi?id=12649
Running ./gdb.btrace/exception.exp ...
FAIL: gdb.btrace/exception.exp: exception - flat (timeout)


Thanks,
Jan

RE: [patch] Fix cleanup in finish_command

[Metzger, Markus T]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Thursday, June 20, 2013 6:20 PM

> Unrelated:
> There is testcase FAILing randomly, reproducible with "read1.so" from:
> 	http://sourceware.org/bugzilla/show_bug.cgi?id=12649
> Running ./gdb.btrace/exception.exp ...
> FAIL: gdb.btrace/exception.exp: exception - flat (timeout)

I do not think that this is an actual issue.  It runs into a timeout because
there is so much memory to read.

When I add to my board file:

	set_board_info gdb,timeout 8000

it takes a very long time, but the test eventually passes.


> > Can you point me to a specific test where this happens?
> 
> Each time with: gdb.btrace/tailcall.exp

I don't get a core file, but I was able to reproduce the bug.

You need to "record btrace", then navigate in the trace using the "finish"
command.  When the inferior then terminates (either by killing it or by
continuing to its end), you get the SEGV in delete_breakpoint.

When I remove the throw_error() in record-btrace.c's to_fetch_registers
function (and just return), the bug disappears.

Looks like someone isn't expecting an error from this function.  Likely the
"finish" command or some code used by that command.  Maybe "finish" is
caught by this exception before it can clean up its breakpoint correctly.

Am I allowed to throw_error() in target functions?
Or should I just warn and return normally?
Or just return and not even warn?

Regards,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [patch] Fix cleanup in finish_command

[Jan Kratochvil]

On Fri, 21 Jun 2013 10:03:21 +0200, Metzger, Markus T wrote:
> > FAIL: gdb.btrace/exception.exp: exception - flat (timeout)
[...]
> it takes a very long time, but the test eventually passes.

OK, I see now, sorry I did not check it more.


> Am I allowed to throw_error() in target functions?
> Or should I just warn and return normally?
> Or just return and not even warn?

There isn't a technical reason why target functions could not throw.
One should just debug why the breakpoint is freed twice, I haven't done so
yet.


Jan

RE: [patch] Fix cleanup in finish_command

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Jan Kratochvil
> Sent: Friday, June 21, 2013 10:14 AM


> > Am I allowed to throw_error() in target functions?
> > Or should I just warn and return normally?
> > Or just return and not even warn?
> 
> There isn't a technical reason why target functions could not throw.
> One should just debug why the breakpoint is freed twice, I haven't done so
> yet.

I can do this, but I won't be able to start right away.

Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

RE: [patch] Fix cleanup in finish_command

[Metzger, Markus T]

> -----Original Message-----
> From: Metzger, Markus T
> Sent: Friday, June 21, 2013 10:23 AM


> > > Am I allowed to throw_error() in target functions?
> > > Or should I just warn and return normally?
> > > Or just return and not even warn?
> >
> > There isn't a technical reason why target functions could not throw.
> > One should just debug why the breakpoint is freed twice, I haven't done so
> > yet.
> 
> I can do this, but I won't be able to start right away.

Looks like throwing an error in the to_fetch_registers function is both
unexpected and unnecessary.  There is already a register_status enum
value to indicate that a register is not available.

When I simply return, I get exactly what I want, i.e. "info reg" prints all
registers except for rip as *value not available*.

I'll replace the throws with a simple return.  Also in to_store_registers
and to_xfer_partial.

Regards,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052

Re: [patch] Fix cleanup in finish_command

[Jan Kratochvil]

On Fri, 21 Jun 2013 10:03:21 +0200, Metzger, Markus T wrote:
> > Unrelated:
> > There is testcase FAILing randomly, reproducible with "read1.so" from:
> > 	http://sourceware.org/bugzilla/show_bug.cgi?id=12649
> > Running ./gdb.btrace/exception.exp ...
> > FAIL: gdb.btrace/exception.exp: exception - flat (timeout)
> 
> I do not think that this is an actual issue.  It runs into a timeout because
> there is so much memory to read.
> 
> When I add to my board file:
> 
> 	set_board_info gdb,timeout 8000
> 
> it takes a very long time, but the test eventually passes.

BTW the testcase is still not right, sometimes it fails due to:
	ERROR: internal buffer is full.
	ERROR: internal buffer is full.

(This is why I tried the "read1" reproducer for it...)

One can either increase the expect buffer size or rather use appropriate
lib/gdb.exp function for it.

To be just merged into the patchset I think.


Regards,
Jan


diff --git a/gdb/testsuite/gdb.btrace/exception.exp b/gdb/testsuite/gdb.btrace/exception.exp
index a8da2b0..430c84b 100644
--- a/gdb/testsuite/gdb.btrace/exception.exp
+++ b/gdb/testsuite/gdb.btrace/exception.exp
@@ -44,24 +44,28 @@ gdb_test_no_output "record btrace"
 gdb_continue_to_breakpoint "cont to $bp_2" ".*$srcfile:$bp_2.*"
 
 # show the flat branch trace
-gdb_test "record function-call-history 1" "
+send_gdb "record function-call-history 1\n"
+gdb_expect_list "exception - flat" "\r\n$gdb_prompt $" { "\r
 1\tmain\\(\\)\r
 2\ttest\\(\\)\r
 3\tfoo\\(\\)\r
 4\tbar\\(\\)\r
 5\tbad\\(\\)\r
-.*\r
-\[0-9\]*\ttest\\(\\)" "exception - flat"
+"
+    "\r\n\[0-9\]*\ttest\\(\\)"
+}
 
 # show the branch trace with calls indented
 #
 # here we see a known bug that the indentation starts at level 1 with
 # two leading spaces instead of level 0 without leading spaces.
-gdb_test "record function-call-history /c 1" "
+send_gdb "record function-call-history /c 1\n"
+gdb_expect_list "exception - calls indented" "\r\n$gdb_prompt $" { "\r
 1\tmain\\(\\)\r
 2\t  test\\(\\)\r
 3\t    foo\\(\\)\r
 4\t      bar\\(\\)\r
 5\t        bad\\(\\)\r
-.*\r
-\[0-9\]*\t  test\\(\\)" "exception - calls indented"
+"
+    "\r\n\[0-9\]*\t  test\\(\\)"
+}

Re: [patch] Fix cleanup in finish_command

[Pedro Alves]

On 06/20/2013 04:18 PM, Joel Brobecker wrote:
>> BTW I have found the crash happens even with this patch, I haven't found the
>> real cause yet.
> 
> Interesting, as I couldn't understand the relationship between
> the backtrace and the patch... You might also be in a situation
> similar to what I faced on Darwin: a correct cleanup fix triggering
> a latent bug; In that situation I found it useful to first git-bisect
> to narrow down the commit that caused the change of behavior, and
> then finish the bug off with valgrind's help.

Seems to be that GDB got a SIGHUP while within proceed.  That
causes a "quit", which kills the inferior, and cancels it's
threads' continuations, which deletes the finish breakpoint.
From the patch, I take it the cleanup that is supposed to delete
that same finish breakpoint on error somehow ran before that,
so that the end result is that GDB ended up trashed for
trying to delete the same breakpoint twice?

The discard_cleanups is after proceed because proceed may
error too, with e.g., a QUIT, though other regular errors
may happen.  I think fixing this needs a bit more work.
On error, the continuations are left stale in the thread
(try hacking an error call just before proceed, after installing
the continuation), which is wrong as it'll cause problems
for the next execution command...  I think we need to move
the discard_cleanups above proceed, but, also make sure the
continuations are cancelled on error (IOW, cancel the
whole command on error).  Maybe install a new cleanup
while `proceed' is running, that runs:

	  do_all_intermediate_continuations (1);
	  do_all_continuations (1);

(See inferior_event_handler.)

-- 
Pedro Alves