From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29537 invoked by alias); 18 Mar 2008 23:48:35 -0000 Received: (qmail 29524 invoked by uid 22791); 18 Mar 2008 23:48:33 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 18 Mar 2008 23:48:08 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 2F715983BF; Tue, 18 Mar 2008 23:48:07 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id CD3FA9802B; Tue, 18 Mar 2008 23:48:06 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1JblXJ-0006aH-TN; Tue, 18 Mar 2008 19:48:05 -0400 Date: Tue, 18 Mar 2008 23:48:00 -0000 From: Daniel Jacobowitz To: Pedro Alves Cc: GDB Patches , Vladimir Prus Subject: Re: [linux] fix stepping over fork in follow-child mode. Message-ID: <20080318234805.GA14989@caradoc.them.org> Mail-Followup-To: Pedro Alves , GDB Patches , Vladimir Prus References: <200803182046.46420.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200803182046.46420.pedro@codesourcery.com> User-Agent: Mutt/1.5.17 (2007-12-11) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-03/txt/msg00266.txt.bz2 On Tue, Mar 18, 2008 at 08:46:46PM +0000, Pedro Alves wrote: > Hi, > > I noticed that issuing a next over a fork call, with > "follow-fork-mode child", wasn't working. The problem is that when we > update the breakpoints in the child to attach them to the current thread > if their thread their currently attached to doesn't exist anymore, > inferior_ptid doesn't hold the tid yet. Here's an alternative fix. Vladimir, this is also the patch I was talking about earlier on IRC. Not tested or finished yet. The basic idea: we only use tids for two things. We display them to the user and we pass them to libthread_db. So the problematic Linux behavior in which the tid is not available immediately for the first thread (not until libpthread.so has initialized, to be precise) is not a problem if we remove the tid from the ptid_t. Instead, Linux now always puts zero there. This lets us delete a lot of code that was already more or less dead, save some operations, et cetera. NULL thread_info->private should never happen when libpthread.so's pthread_create is used to create new threads. That's where most of the FIXMEs are for this patch; they're in places where if linux-nat.c detects a newly cloned process, we don't have thread_info->private filled in. I think the right thing to do is to fill it in lazily by calling td_ta_map_lwp2thr et cetera, and to handle it being NULL for clones the thread manager doesn't know about. I can work on this some more tomorrow, but you wanted a preview :-) -- Daniel Jacobowitz CodeSourcery Index: gdbthread.h =================================================================== RCS file: /cvs/src/src/gdb/gdbthread.h,v retrieving revision 1.20 diff -u -p -r1.20 gdbthread.h --- gdbthread.h 15 Mar 2008 13:53:25 -0000 1.20 +++ gdbthread.h 18 Mar 2008 23:41:17 -0000 @@ -81,6 +81,10 @@ extern struct thread_info *add_thread (p about new thread. */ extern struct thread_info *add_thread_silent (ptid_t ptid); +/* Same as add_thread, and sets the private info. */ +extern struct thread_info *add_thread_with_info (ptid_t ptid, + struct private_thread_info *); + /* Delete an existing thread list entry. */ extern void delete_thread (ptid_t); Index: linux-thread-db.c =================================================================== RCS file: /cvs/src/src/gdb/linux-thread-db.c,v retrieving revision 1.37 diff -u -p -r1.37 linux-thread-db.c --- linux-thread-db.c 23 Jan 2008 11:26:28 -0000 1.37 +++ linux-thread-db.c 18 Mar 2008 23:41:18 -0000 @@ -126,10 +126,6 @@ static void detach_thread (ptid_t ptid, #define GET_PID(ptid) ptid_get_pid (ptid) #define GET_LWP(ptid) ptid_get_lwp (ptid) -#define GET_THREAD(ptid) ptid_get_tid (ptid) - -#define is_lwp(ptid) (GET_LWP (ptid) != 0) -#define is_thread(ptid) (GET_THREAD (ptid) != 0) #define BUILD_LWP(lwp, pid) ptid_build (pid, lwp, 0) @@ -143,11 +139,8 @@ struct private_thread_info unsigned int dying:1; /* Cached thread state. */ - unsigned int th_valid:1; - unsigned int ti_valid:1; - td_thrhandle_t th; - td_thrinfo_t ti; + thread_t tid; }; @@ -257,7 +250,7 @@ thread_get_info_callback (const td_thrha thread_db_err_str (err)); /* Fill the cache. */ - thread_ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, ti.ti_tid); + thread_ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, 0); thread_info = find_thread_pid (thread_ptid); /* In the case of a zombie thread, don't continue. We don't want to @@ -266,13 +259,6 @@ thread_get_info_callback (const td_thrha { if (infop != NULL) *(struct thread_info **) infop = thread_info; - if (thread_info != NULL) - { - memcpy (&thread_info->private->th, thp, sizeof (*thp)); - thread_info->private->th_valid = 1; - memcpy (&thread_info->private->ti, &ti, sizeof (ti)); - thread_info->private->ti_valid = 1; - } return TD_THR_ZOMBIE; } @@ -284,39 +270,11 @@ thread_get_info_callback (const td_thrha gdb_assert (thread_info != NULL); } - memcpy (&thread_info->private->th, thp, sizeof (*thp)); - thread_info->private->th_valid = 1; - memcpy (&thread_info->private->ti, &ti, sizeof (ti)); - thread_info->private->ti_valid = 1; - if (infop != NULL) *(struct thread_info **) infop = thread_info; return 0; } - -/* Accessor functions for the thread_db information, with caching. */ - -static void -thread_db_map_id2thr (struct thread_info *thread_info, int fatal) -{ - td_err_e err; - - if (thread_info->private->th_valid) - return; - - err = td_ta_map_id2thr_p (thread_agent, GET_THREAD (thread_info->ptid), - &thread_info->private->th); - if (err != TD_OK) - { - if (fatal) - error (_("Cannot find thread %ld: %s"), - (long) GET_THREAD (thread_info->ptid), - thread_db_err_str (err)); - } - else - thread_info->private->th_valid = 1; -} /* Convert between user-level thread ids and LWP ids. */ @@ -328,11 +286,11 @@ thread_from_lwp (ptid_t ptid) struct thread_info *thread_info; ptid_t thread_ptid; + /* FIXME: Should this ever even happen? I don't think it + should reach here. */ if (GET_LWP (ptid) == 0) ptid = BUILD_LWP (GET_PID (ptid), GET_PID (ptid)); - gdb_assert (is_lwp (ptid)); - err = td_ta_map_lwp2thr_p (thread_agent, GET_LWP (ptid), &th); if (err != TD_OK) error (_("Cannot find user-level thread for LWP %ld: %s"), @@ -352,16 +310,8 @@ thread_from_lwp (ptid_t ptid) && thread_info == NULL) return pid_to_ptid (-1); - gdb_assert (thread_info && thread_info->private->ti_valid); - - return ptid_build (GET_PID (ptid), GET_LWP (ptid), - thread_info->private->ti.ti_tid); -} - -static ptid_t -lwp_from_thread (ptid_t ptid) -{ - return BUILD_LWP (GET_LWP (ptid), GET_PID (ptid)); + gdb_assert (ptid_get_tid (ptid) == 0); + return ptid; } @@ -672,6 +622,7 @@ static void attach_thread (ptid_t ptid, const td_thrhandle_t *th_p, const td_thrinfo_t *ti_p) { + struct private_thread_info *private; struct thread_info *tp; td_err_e err; @@ -705,10 +656,21 @@ attach_thread (ptid_t ptid, const td_thr if (lin_lwp_attach_lwp (BUILD_LWP (ti_p->ti_lid, GET_PID (ptid))) < 0) return; + /* Construct the thread's private data. */ + private = xmalloc (sizeof (struct private_thread_info)); + memset (private, 0, sizeof (struct private_thread_info)); + + /* A thread ID of zero may mean the thread library has not initialized + yet. But we shouldn't even get here if that's the case. FIXME: + if we change GDB to always have at least one thread in the thread + list this will have to go somewhere else; maybe private == NULL + until the thread_db target claims it. */ + gdb_assert (ti_p->ti_tid != 0); + private->th = *th_p; + private->tid = ti_p->ti_tid; + /* Add the thread to GDB's thread list. */ - tp = add_thread (ptid); - tp->private = xmalloc (sizeof (struct private_thread_info)); - memset (tp->private, 0, sizeof (struct private_thread_info)); + tp = add_thread_with_info (ptid, private); /* Enable thread event reporting for this thread. */ err = td_thr_event_enable_p (th_p, 1); @@ -742,47 +704,12 @@ thread_db_detach (char *args, int from_t { disable_thread_event_reporting (); - /* There's no need to save & restore inferior_ptid here, since the - inferior is not supposed to survive this function call. */ - inferior_ptid = lwp_from_thread (inferior_ptid); - target_beneath->to_detach (args, from_tty); /* Should this be done by detach_command? */ target_mourn_inferior (); } -static int -clear_lwpid_callback (struct thread_info *thread, void *dummy) -{ - /* If we know that our thread implementation is 1-to-1, we could save - a certain amount of information; it's not clear how much, so we - are always conservative. */ - - thread->private->th_valid = 0; - thread->private->ti_valid = 0; - - return 0; -} - -static void -thread_db_resume (ptid_t ptid, int step, enum target_signal signo) -{ - struct cleanup *old_chain = save_inferior_ptid (); - - if (GET_PID (ptid) == -1) - inferior_ptid = lwp_from_thread (inferior_ptid); - else if (is_thread (ptid)) - ptid = lwp_from_thread (ptid); - - /* Clear cached data which may not be valid after the resume. */ - iterate_over_threads (clear_lwpid_callback, NULL); - - target_beneath->to_resume (ptid, step, signo); - - do_cleanups (old_chain); -} - /* Check if PID is currently stopped at the location of a thread event breakpoint location. If it is, read the event message and act upon the event. */ @@ -833,7 +760,7 @@ check_event (ptid_t ptid) if (err != TD_OK) error (_("Cannot get thread info: %s"), thread_db_err_str (err)); - ptid = ptid_build (GET_PID (ptid), ti.ti_lid, ti.ti_tid); + ptid = ptid_build (GET_PID (ptid), ti.ti_lid, 0); switch (msg.event) { @@ -865,9 +792,6 @@ thread_db_wait (ptid_t ptid, struct targ { extern ptid_t trap_ptid; - if (GET_PID (ptid) != -1 && is_thread (ptid)) - ptid = lwp_from_thread (ptid); - ptid = target_beneath->to_wait (ptid, ourstatus); if (ourstatus->kind == TARGET_WAITKIND_EXITED @@ -913,15 +837,6 @@ thread_db_wait (ptid_t ptid, struct targ } static void -thread_db_kill (void) -{ - /* There's no need to save & restore inferior_ptid here, since the - inferior isn't supposed to survive this function call. */ - inferior_ptid = lwp_from_thread (inferior_ptid); - target_beneath->to_kill (); -} - -static void thread_db_mourn_inferior (void) { /* Forget about the child's process ID. We shouldn't need it @@ -954,7 +869,7 @@ find_new_threads_callback (const td_thrh if (ti.ti_state == TD_THR_UNKNOWN || ti.ti_state == TD_THR_ZOMBIE) return 0; /* A zombie -- ignore. */ - ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, ti.ti_tid); + ptid = ptid_build (GET_PID (inferior_ptid), ti.ti_lid, 0); if (ti.ti_tid == 0) { @@ -994,18 +909,21 @@ thread_db_find_new_threads (void) static char * thread_db_pid_to_str (ptid_t ptid) { - if (is_thread (ptid)) + struct thread_info *thread_info = find_thread_pid (ptid); + + if (thread_info != NULL) { static char buf[64]; - struct thread_info *thread_info; + thread_t tid; + + /* FIXME: What will set ->private? */ + gdb_assert (thread_info->private != NULL); + /* FIXME: What will ensure tid_valid? */ + tid = thread_info->private->tid; thread_info = find_thread_pid (ptid); - if (thread_info == NULL) - snprintf (buf, sizeof (buf), "Thread 0x%lx (LWP %ld) (Missing)", - GET_THREAD (ptid), GET_LWP (ptid)); - else - snprintf (buf, sizeof (buf), "Thread 0x%lx (LWP %ld)", - GET_THREAD (ptid), GET_LWP (ptid)); + snprintf (buf, sizeof (buf), "Thread 0x%lx (LWP %ld)", + tid, GET_LWP (ptid)); return buf; } @@ -1028,16 +946,6 @@ thread_db_extra_thread_info (struct thre return NULL; } -/* Return 1 if this thread has the same LWP as the passed PTID. */ - -static int -same_ptid_callback (struct thread_info *thread, void *arg) -{ - ptid_t *ptid_p = arg; - - return GET_LWP (thread->ptid) == GET_LWP (*ptid_p); -} - /* Get the address of the thread local variable in load module LM which is stored at OFFSET within the thread local storage for thread PTID. */ @@ -1046,26 +954,19 @@ thread_db_get_thread_local_address (ptid CORE_ADDR lm, CORE_ADDR offset) { + struct thread_info *thread_info; + /* If we have not discovered any threads yet, check now. */ - if (!is_thread (ptid) && !have_threads ()) + if (!have_threads ()) thread_db_find_new_threads (); - /* Try to find a matching thread if we still have the LWP ID instead - of the thread ID. */ - if (!is_thread (ptid)) - { - struct thread_info *thread; - - thread = iterate_over_threads (same_ptid_callback, &ptid); - if (thread != NULL) - ptid = thread->ptid; - } + /* Find the matching thread. */ + thread_info = find_thread_pid (ptid); - if (is_thread (ptid)) + if (thread_info != NULL) { td_err_e err; void *address; - struct thread_info *thread_info; /* glibc doesn't provide the needed interface. */ if (!td_thr_tls_get_addr_p) @@ -1075,10 +976,8 @@ thread_db_get_thread_local_address (ptid /* Caller should have verified that lm != 0. */ gdb_assert (lm != 0); - /* Get info about the thread. */ - thread_info = find_thread_pid (ptid); - gdb_assert (thread_info); - thread_db_map_id2thr (thread_info, 1); + /* Something should already have arranged this... FIXME: right? */ + gdb_assert (thread_info->private); /* Finally, get the address of the variable. */ err = td_thr_tls_get_addr_p (&thread_info->private->th, @@ -1122,9 +1021,7 @@ init_thread_db_ops (void) thread_db_ops.to_longname = "multi-threaded child process."; thread_db_ops.to_doc = "Threads and pthreads support."; thread_db_ops.to_detach = thread_db_detach; - thread_db_ops.to_resume = thread_db_resume; thread_db_ops.to_wait = thread_db_wait; - thread_db_ops.to_kill = thread_db_kill; thread_db_ops.to_mourn_inferior = thread_db_mourn_inferior; thread_db_ops.to_find_new_threads = thread_db_find_new_threads; thread_db_ops.to_pid_to_str = thread_db_pid_to_str; Index: thread.c =================================================================== RCS file: /cvs/src/src/gdb/thread.c,v retrieving revision 1.62 diff -u -p -r1.62 thread.c --- thread.c 15 Mar 2008 13:53:25 -0000 1.62 +++ thread.c 18 Mar 2008 23:41:18 -0000 @@ -132,10 +132,12 @@ add_thread_silent (ptid_t ptid) } struct thread_info * -add_thread (ptid_t ptid) +add_thread_with_info (ptid_t ptid, struct private_thread_info *private) { struct thread_info *result = add_thread_silent (ptid); + result->private = private; + if (print_thread_events) printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid)); @@ -144,6 +146,12 @@ add_thread (ptid_t ptid) return result; } +struct thread_info * +add_thread (ptid_t ptid) +{ + return add_thread_with_info (ptid, NULL); +} + void delete_thread (ptid_t ptid) { Index: testsuite/gdb.threads/fork-child-threads.exp =================================================================== RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/fork-child-threads.exp,v retrieving revision 1.1 diff -u -p -r1.1 fork-child-threads.exp --- testsuite/gdb.threads/fork-child-threads.exp 2 Jan 2008 13:36:38 -0000 1.1 +++ testsuite/gdb.threads/fork-child-threads.exp 18 Mar 2008 23:41:18 -0000 @@ -38,6 +38,10 @@ if ![runto_main] then { gdb_test "set follow-fork-mode child" gdb_breakpoint "start" + +# Make sure we can step over fork without losing our breakpoint. +gdb_test "next" ".*pthread_create \\(&thread, NULL, start, NULL\\);.*" "next over fork" + gdb_test "continue" "Breakpoint 2, start.*" "get to the spawned thread" # Wrong: