[patch] Fix a crash on NULL event_thread

[patch] Fix a crash on NULL event_thread

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 5056 bytes --]

Hi Pedro,

various testcases - such as gdb.threads/bp_in_thread.exp - crash HEAD.  Tested
only on Fedora kernel-2.6.27-0.317.rc5.git10.fc10.x86_64 but I expect it can
happen anywhere.

LINUX_HANDLE_EXTENDED_WAIT calls ADD_LWP but not ADD_THREAD.

Found while investigating a bugreport from Shawn Starr.


Regards,
Jan


Program received signal SIGSEGV, Segmentation fault.
0x0000000000505915 in handle_inferior_event (ecs=0x7fff9c7fb2a0) at infrun.c:2136
2136          ecs->event_thread->stop_signal = ecs->ws.value.sig;
(gdb) bt
#0  0x0000000000505915 in handle_inferior_event (ecs=0x7fff9c7fb2a0) at infrun.c:2136
#1  0x00000000005047da in wait_for_inferior (treat_exec_as_sigtrap=0) at infrun.c:1581
#2  0x0000000000504415 in proceed (addr=18446744073709551615, siggnal=TARGET_SIGNAL_0, step=0) at infrun.c:1343
#3  0x00000000004ff721 in run_command_1 (args=0x0, from_tty=1, tbreak_at_main=0) at infcmd.c:538
#4  0x00000000004ff74a in run_command (args=0x0, from_tty=1) at infcmd.c:545
#5  0x000000000048ec64 in do_cfunc (c=0x2046050, args=0x0, from_tty=1) at .././gdb/cli/cli-decode.c:60
#6  0x000000000049185d in cmd_func (cmd=0x2046050, args=0x0, from_tty=1) at .././gdb/cli/cli-decode.c:1672
#7  0x000000000044dc6b in execute_command (p=0x20111c1 "", from_tty=1) at top.c:457
#8  0x0000000000519603 in command_handler (command=0x20111c0 "") at event-top.c:514
#9  0x0000000000519cc3 in command_line_handler (rl=0x2108ff0 "\020\220\020\002") at event-top.c:739
#10 0x00000000005feea5 in rl_callback_read_char () at callback.c:205
#11 0x0000000000518bb5 in rl_callback_read_char_wrapper (client_data=0x0) at event-top.c:178
#12 0x00000000005194c9 in stdin_event_handler (error=0, client_data=0x0) at event-top.c:433
#13 0x0000000000517ea4 in handle_file_event (event_file_desc=0) at event-loop.c:732
#14 0x000000000051753f in process_event () at event-loop.c:341
#15 0x000000000051758e in gdb_do_one_event (data=0x0) at event-loop.c:378
#16 0x0000000000513e25 in catch_errors (func=0x517554 <gdb_do_one_event>, func_args=0x0, errstring=0x711fea "", mask=6)
    at exceptions.c:516
#17 0x00000000004a38fb in tui_command_loop (data=0x0) at .././gdb/tui/tui-interp.c:153
#18 0x000000000051444f in current_interp_command_loop () at interps.c:289
#19 0x0000000000445f69 in captured_command_loop (data=0x0) at .././gdb/main.c:99
#20 0x0000000000513e25 in catch_errors (func=0x445f58 <captured_command_loop>, func_args=0x0, errstring=0x6f9641 "", mask=6)
    at exceptions.c:516
#21 0x0000000000446ffc in captured_main (data=0x7fff9c7fbbd0) at .././gdb/main.c:831
#22 0x0000000000513e25 in catch_errors (func=0x445f9b <captured_main>, func_args=0x7fff9c7fbbd0, errstring=0x6f9641 "",
    mask=6) at exceptions.c:516
#23 0x000000000044702f in gdb_main (args=0x7fff9c7fbbd0) at .././gdb/main.c:840
#24 0x0000000000445f54 in main (argc=5, argv=0x7fff9c7fbcc8) at gdb.c:33
(gdb) l
2131          return;
2132    
2133        case TARGET_WAITKIND_STOPPED:
2134          if (debug_infrun)
2135            fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_STOPPED\n");
2136          ecs->event_thread->stop_signal = ecs->ws.value.sig;
2137          break;
2138    
2139          /* We had an event in the inferior, but we are not interested
2140             in handling it at this level. The lower layers have already
(gdb) p ecs->event_thread
$1 = (struct thread_info *) 0x0


wait4(-1, [{WIFSTOPPED(s) && WSTOPSIG(s) == SIGSTOP}], WNOHANG|__WCLONE, NULL) = 30755
wait4(-1, 0x7fff85ec98d8, WNOHANG|__WCLONE, NULL) = 0
wait4(-1, [{WIFSTOPPED(s) && WSTOPSIG(s) == SIGTRAP} | 0x30000], WNOHANG, NULL) = 30752
ptrace(0x4202 /* PTRACE_??? */, 30752, 0, 0x1c30f30) = -1 EINVAL (Invalid argument)
ptrace(0x4201 /* PTRACE_??? */, 30752, 0, 0x7fff85ec9558) = 0
ptrace(PTRACE_POKEUSER, 30755, offsetof(struct user, u_debugreg), 0) = 0
ptrace(PTRACE_POKEUSER, 30755, offsetof(struct user, u_debugreg) + 8, 0) = 0
ptrace(PTRACE_POKEUSER, 30755, offsetof(struct user, u_debugreg) + 16, 0) = 0
ptrace(PTRACE_POKEUSER, 30755, offsetof(struct user, u_debugreg) + 24, 0) = 0
ptrace(PTRACE_POKEUSER, 30755, offsetof(struct user, u_debugreg) + 56, 0) = 0
ptrace(PTRACE_CONT, 30755, 0, SIG_0)    = 0
ptrace(PTRACE_CONT, 30752, 0, SIG_0)    = 0
wait4(-1, 0x7fff85ec98d8, WNOHANG, NULL) = 0
rt_sigsuspend([])                       = ? ERESTARTNOHAND (To be restarted)
--- SIGCHLD (Child exited) @ 0 (0) ---
rt_sigreturn(0x11)                      = -1 EINTR (Interrupted system call)
wait4(-1, [{WIFSTOPPED(s) && WSTOPSIG(s) == SIGTRAP}], WNOHANG|__WCLONE, NULL) = 30755
ptrace(0x4202 /* PTRACE_??? */, 30755, 0, 0x1c45ee0) = 0
tkill(30755, SIG_0)                     = 0
tkill(30752, SIGSTOP)                   = 0
wait4(30752, [{WIFSTOPPED(s) && WSTOPSIG(s) == SIGSTOP}], 0, NULL) = 30752
ptrace(PTRACE_GETREGS, 30755, 0, 0x7fff85ec9340) = 0
ptrace(PTRACE_GETREGS, 30755, 0, 0x7fff85ec9300) = 0
ptrace(PTRACE_SETREGS, 30755, 0, 0x7fff85ec9300) = 0
write(1, "[New LWP 30755]\n"..., 16)    = 16
--- SIGSEGV (Segmentation fault) @ 0 (0) ---
+++ killed by SIGSEGV (core dumped) +++

[-- Attachment #2: ecs2.patch --]
[-- Type: text/plain, Size: 1745 bytes --]

2008-09-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix a crash on uninitialized ECS->EVENT_THREAD for a newly found thread.
	* infrun.c (wait_for_inferior): Move this ECS->EVENT_THREAD
	initialization ...
	(fetch_inferior_event): ... and this ECS->EVENT_THREAD initialization
	...
	(handle_inferior_event): ... here after ADD_THREAD together with the
	local ADJUST_PC_AFTER_BREAK call.

--- ./gdb/infrun.c	11 Sep 2008 14:21:49 -0000	1.318
+++ ./gdb/infrun.c	12 Sep 2008 22:02:19 -0000
@@ -1568,8 +1568,6 @@ wait_for_inferior (int treat_exec_as_sig
       else
 	ecs->ptid = target_wait (waiton_ptid, &ecs->ws);
 
-      ecs->event_thread = find_thread_pid (ecs->ptid);
-
       if (treat_exec_as_sigtrap && ecs->ws.kind == TARGET_WAITKIND_EXECD)
         {
           xfree (ecs->ws.value.execd_pathname);
@@ -1645,8 +1643,6 @@ fetch_inferior_event (void *client_data)
        thread.  */
     context_switch (ecs->ptid);
 
-  ecs->event_thread = find_thread_pid (ecs->ptid);
-
   /* Now figure out what to do with the result of the result.  */
   handle_inferior_event (ecs);
 
@@ -1854,8 +1850,6 @@ handle_inferior_event (struct execution_
   /* Always clear state belonging to the previous time we stopped.  */
   stop_stack_dummy = 0;
 
-  adjust_pc_after_break (ecs);
-
   reinit_frame_cache ();
 
   /* If it's a new process, add it to the thread database */
@@ -1868,6 +1862,10 @@ handle_inferior_event (struct execution_
       && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
     add_thread (ecs->ptid);
 
+  ecs->event_thread = find_thread_pid (ecs->ptid);
+
+  adjust_pc_after_break (ecs);
+
   if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
     {
       /* Mark the non-executing threads accordingly.  */

Re: [patch] Fix a crash on NULL event_thread

[Pedro Alves]

Hi Jan,

Sorry for the breakage.

On Friday 12 September 2008 23:12:27, Jan Kratochvil wrote:
> Hi Pedro,
>
> various testcases - such as gdb.threads/bp_in_thread.exp - crash HEAD. 
> Tested only on Fedora kernel-2.6.27-0.317.rc5.git10.fc10.x86_64 but I
> expect it can happen anywhere.
>
> LINUX_HANDLE_EXTENDED_WAIT calls ADD_LWP but not ADD_THREAD.
>
> Found while investigating a bugreport from Shawn Starr.

Hmm, it may be due to something having changed in the scheduling, as I'm
on ubuntu's 2.6.24-19-generic x86_64 SMP (dual core), and I never saw
that happen.

I'd like to get rid of that ecs->new_thread_event, as you'll notice
in handle_inferior_event just a bit below what you're changing, GDB will
just resume that thread afterwards, meaning, the real event is
just ignored.  In this case, you'll get the breakpoint hit again,
but we might lose something else... Plus, this new_thread_event is
sometimes masking targets misbehaving --- we've tripped on that a few
months ago.  What I'd like to do, if possible, is to make sure that
we never reach handle_inferior_event with a thread related event, for
a thread that is not in the thread list.

Would it be possible to add the thread to the thread list, in
addition to the lwp?

-- 
Pedro Alves

Re: [patch] Fix a crash on NULL event_thread

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1366 bytes --]

On Sat, 13 Sep 2008 00:44:55 +0200, Pedro Alves wrote:
> On Friday 12 September 2008 23:12:27, Jan Kratochvil wrote:
> > various testcases - such as gdb.threads/bp_in_thread.exp - crash HEAD. 
> > Tested only on Fedora kernel-2.6.27-0.317.rc5.git10.fc10.x86_64 but I
> > expect it can happen anywhere.
> >
> > LINUX_HANDLE_EXTENDED_WAIT calls ADD_LWP but not ADD_THREAD.
> 
> Hmm, it may be due to something having changed in the scheduling, as I'm
> on ubuntu's 2.6.24-19-generic x86_64 SMP (dual core), and I never saw
> that happen.

Yes, the Fedora kernels have a different ptrace implementation (based on
utrace by Roland McGrath) which has more free but still permitted timing.


> Would it be possible to add the thread to the thread list, in
> addition to the lwp?

IMO the reason for two lists is that really these two resources are different.
You can perfectly have tracked LWPs with no corresponding thread structures.
Attached a testcase using clone(2) which if you CTRL-C it has a state:
(gdb) plist thread_list ptid
$1 = {pid = 25112, lwp = 25112, tid = 0}
(gdb) plist lwp_list ptid
$2 = {pid = 25112, lwp = 25115, tid = 0}
$3 = {pid = 25112, lwp = 25112, tid = 0}

New thread notification will come from libthread_db but some time in between
we have no corresponding thread structures such as they will never exist for
standalone LWPs.


Regards,
Jan

[-- Attachment #2: clone-thread.c --]
[-- Type: text/plain, Size: 1063 bytes --]

#include <unistd.h>
#include <assert.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <stdlib.h>
#include <sched.h>
#include <sys/mman.h>

#define FLAGS (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND \
	       | CLONE_THREAD | CLONE_SYSVSEM)

static int
child_func (void *arg)
{
  sleep (60);
  _exit (EXIT_SUCCESS);
  abort ();
}

int
main (void)
{
#ifndef PAGE_SIZE
#define PAGE_SIZE 0x1000
#endif
  const size_t stack_size = PAGE_SIZE;
  unsigned char *stack = mmap (NULL, stack_size, PROT_READ | PROT_WRITE,
			       MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
  int child_tid;

  assert (stack != NULL);
  assert (stack != MAP_FAILED);
  stack[0] = 0;
  stack[stack_size - 1] = 0;

#ifdef __ia64__
  extern int __clone2 (int (*fn) (void *arg), void *child_stack,
		       size_t stack_size, int flags, void *arg);
  child_tid = __clone2 (child_func, stack + stack_size, stack_size,
			FLAGS, NULL);
#else	/* !__ia64__ */
  child_tid = clone (child_func, stack + stack_size, FLAGS, NULL);
#endif	/* !__ia64__ */

  sleep (60);

  return EXIT_SUCCESS;
}

Re: [patch] Fix a crash on NULL event_thread

[Pedro Alves]

Hi Jan, sorry for the delay,

On Monday 15 September 2008 19:56:27, Jan Kratochvil wrote:
> > Would it be possible to add the thread to the thread list, in
> > addition to the lwp?
>
> IMO the reason for two lists is that really these two resources are
> different. You can perfectly have tracked LWPs with no corresponding thread
> structures. Attached a testcase using clone(2) which if you CTRL-C it has a
> state: (gdb) plist thread_list ptid
> $1 = {pid = 25112, lwp = 25112, tid = 0}
> (gdb) plist lwp_list ptid
> $2 = {pid = 25112, lwp = 25115, tid = 0}
> $3 = {pid = 25112, lwp = 25112, tid = 0}
>
> New thread notification will come from libthread_db but some time in
> between we have no corresponding thread structures such as they will never
> exist for standalone LWPs.

I understand the difference between LWPs and pthread threads.

A posix threads library hides the underlying threads implementation,
but that doesn't mean that we can only call "threads", things
that are visible through pthreads.  That is, in your non-pthreads
example, you're happy to call new lwps "threads", and
use CLONE_*THREAD*.

From GDB's perspective, a "thread" is an abstraction that
represents a locus of execution.  It doesn't really matter
if its comes from pthreads or using CLONE_THREAD directly.  You'll
still need to be able to see them in "info threads", and switch
between them with "thread", and GDB will have to manage thread
stepping state in them.  In fact, the new_thread_event thing is
calling "add_thread" on an LWP, that hasn't hit a thread
create event breakpoint yet.  So, we're talking about *when/where* to
call add_thread.  In your clone-thread.c example, just put a break
on child_func and let it hit to reliably trigger the
new_thread_event path.

I've been cooking some ideas about decoupling where we trigger the
[New Thread ...] notifications (currently in add_thread), from
managing the data structures, and about making GDB's core not
so agnostic of 1:1, M:N, x:y, target/thread/lwp/process child/parent
relations, but those will come later.

In the mean time, your patch preserves the old behaviour, and
looks good to me.  You'll need someone else to approve it,
though.

-- 
Pedro Alves

Re: [patch] Fix a crash on NULL event_thread

[Daniel Jacobowitz]

On Sat, Sep 13, 2008 at 12:12:27AM +0200, Jan Kratochvil wrote:
> 2008-09-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix a crash on uninitialized ECS->EVENT_THREAD for a newly found thread.
> 	* infrun.c (wait_for_inferior): Move this ECS->EVENT_THREAD
> 	initialization ...
> 	(fetch_inferior_event): ... and this ECS->EVENT_THREAD initialization
> 	...
> 	(handle_inferior_event): ... here after ADD_THREAD together with the
> 	local ADJUST_PC_AFTER_BREAK call.

Please move the reinit_frame_cache call so it is still right after
adjust_pc_after_break.  Also, no need to capitalize
adjust_pc_after_break; this is like the texinfo @var{} markup.  You
capitalize FOO when you mean "the value of a variable named foo",
but the name of the variable is still "foo".

Otherwise looks OK, thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch] Fix a crash on NULL event_thread

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On Wed, 17 Sep 2008 20:03:37 +0200, Daniel Jacobowitz wrote:
> Please move the reinit_frame_cache call so it is still right after
> adjust_pc_after_break.

I expect the only possible user is tui_selected_frame_level_changed_hook
through DEPRECATED_SELECTED_FRAME_LEVEL_CHANGED_HOOK, thanks for catching it.


> Also, no need to capitalize adjust_pc_after_break; this is like the texinfo
> @var{} markup.  You capitalize FOO when you mean "the value of a variable
> named foo", but the name of the variable is still "foo".

Thanks, I though it has different rules.


> Otherwise looks OK, thanks.

Committed the attached patch (added there two dependency comments).


Regards,
Jan

[-- Attachment #2: ecs4-cvs.patch --]
[-- Type: text/plain, Size: 2017 bytes --]

2008-09-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix a crash on uninitialized ECS->EVENT_THREAD for a newly found thread.
	* infrun.c (wait_for_inferior): Move this ECS->EVENT_THREAD
	initialization ...
	(fetch_inferior_event): ... and this ECS->EVENT_THREAD initialization
	...
	(handle_inferior_event): ... here after the add_thread call together
	with the local adjust_pc_after_break and reinit_frame_cache calls.

===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.318
retrieving revision 1.319
diff -u -r1.318 -r1.319
--- src/gdb/infrun.c	2008/09/11 14:21:49	1.318
+++ src/gdb/infrun.c	2008/09/17 21:48:49	1.319
@@ -1568,8 +1568,6 @@
       else
 	ecs->ptid = target_wait (waiton_ptid, &ecs->ws);
 
-      ecs->event_thread = find_thread_pid (ecs->ptid);
-
       if (treat_exec_as_sigtrap && ecs->ws.kind == TARGET_WAITKIND_EXECD)
         {
           xfree (ecs->ws.value.execd_pathname);
@@ -1645,8 +1643,6 @@
        thread.  */
     context_switch (ecs->ptid);
 
-  ecs->event_thread = find_thread_pid (ecs->ptid);
-
   /* Now figure out what to do with the result of the result.  */
   handle_inferior_event (ecs);
 
@@ -1854,10 +1850,6 @@
   /* Always clear state belonging to the previous time we stopped.  */
   stop_stack_dummy = 0;
 
-  adjust_pc_after_break (ecs);
-
-  reinit_frame_cache ();
-
   /* If it's a new process, add it to the thread database */
 
   ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
@@ -1868,6 +1860,14 @@
       && ecs->ws.kind != TARGET_WAITKIND_SIGNALLED && ecs->new_thread_event)
     add_thread (ecs->ptid);
 
+  ecs->event_thread = find_thread_pid (ecs->ptid);
+
+  /* Dependent on valid ECS->EVENT_THREAD.  */
+  adjust_pc_after_break (ecs);
+
+  /* Dependent on the current PC value modified by adjust_pc_after_break.  */
+  reinit_frame_cache ();
+
   if (ecs->ws.kind != TARGET_WAITKIND_IGNORE)
     {
       /* Mark the non-executing threads accordingly.  */