From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21141 invoked by alias); 26 Nov 2002 22:10:07 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 20994 invoked from network); 26 Nov 2002 22:10:03 -0000 Received: from unknown (HELO localhost.redhat.com) (216.138.202.10) by sources.redhat.com with SMTP; 26 Nov 2002 22:10:03 -0000 Received: from redhat.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id D27A33F30; Tue, 26 Nov 2002 17:09:57 -0500 (EST) Message-ID: <3DE3F135.6030605@redhat.com> Date: Tue, 26 Nov 2002 14:10:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:1.0.0) Gecko/20020824 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Michael Snyder , Mark Kettenis Cc: gdb-patches@sources.redhat.com, Daniel Jacobowitz Subject: [Fwd: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]] Content-Type: multipart/mixed; boundary="------------000302090405000700030207" X-SW-Source: 2002-11/txt/msg00664.txt.bz2 This is a multi-part message in MIME format. --------------000302090405000700030207 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-length: 368 For the thread maintainers (this was unfortunatly hidden in a gdb@ discussion). ####### ##### ##### # # # # # # # # ###### ##### ### # ### # # # # ### # # ##### ### ##### # DanielJ's off line for a week so someone will also need to commit. Andrew --------------000302090405000700030207 Content-Type: message/rfc822; name="Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]" Content-length: 10300 X-Mozilla-Status2: 00000000 Return-Path: Delivered-To: ac131313@localhost.redhat.com Received: from localhost (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id 80B943E4B for ; Thu, 21 Nov 2002 19:15:47 -0500 (EST) X-Sieve: cmu-sieve 2.0 Received: from potter.sfbay.redhat.com by localhost with IMAP (fetchmail-5.9.13) for ac131313@localhost (single-drop); Thu, 21 Nov 2002 19:15:47 -0500 (EST) Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by potter.sfbay.redhat.com (8.11.6/8.11.6) with ESMTP id gAM0EoM15521 for ; Thu, 21 Nov 2002 16:14:50 -0800 Received: from mx1.redhat.com (mx1.redhat.com [172.16.48.31]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with SMTP id gAM0EnD00620 for ; Thu, 21 Nov 2002 19:14:49 -0500 Received: from crack.them.org (mail@crack.them.org [65.125.64.184]) by mx1.redhat.com (8.11.6/8.11.6) with SMTP id gALNonP14552 for ; Thu, 21 Nov 2002 18:50:49 -0500 Received: from nevyn.them.org ([66.93.61.169] ident=mail) by crack.them.org with asmtp (Exim 3.12 #1 (Debian)) id 18F3LX-0008Qe-00; Thu, 21 Nov 2002 20:15:07 -0600 Received: from drow by nevyn.them.org with local (Exim 3.36 #1 (Debian)) id 18F1T5-0002uy-00; Thu, 21 Nov 2002 19:14:47 -0500 Date: Thu, 21 Nov 2002 19:14:47 -0500 From: Daniel Jacobowitz To: Andrew Cagney Cc: gdb@sources.redhat.com Subject: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)] Message-ID: <20021122001447.GA7884@nevyn.them.org> Mail-Followup-To: Andrew Cagney , gdb@sources.redhat.com References: <3DDD6150.10407@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3DDD6150.10407@redhat.com> User-Agent: Mutt/1.5.1i Content-length: 8385 On Thu, Nov 21, 2002 at 05:42:24PM -0500, Andrew Cagney wrote: > FYI, > > Too many memory reads/writes was one reason for a ptrace'd threaded > shlib program running slow, I suspect this is the other. Maybe, maybe not... definitely needs to go though! Thanks for such a thorough investigation, it gave me a good idea. > Briefly, the GNU/Linux thread code is giving regcache.c conflicting > stories over which inferior ptid should be in the register cache. As a > consequence, every single register fetch leads to a regcache flush and > re-fetch. Outch! > > > Briefly, core GDB tries to fetch a register. This eventually leads to > the call: > > regcache_raw_read(REGNUM) > > registers_tpid != inferior_tpid > (gdb) print registers_ptid > $6 = {pid = 31263, lwp = 0, tid = 0} > (gdb) print inferior_ptid > $7 = {pid = 31263, lwp = 31263, tid = 0} > -> flush regcache > -> registers_tpid = inferior_tpid > -- at this point regnum is invalid > target_fetch_registers (regnum) > > Since the inferior doesn't match the target, the cache is flushed, > inferior_ptid is updated, and the register is fetched. The fetch flows > on down into the depths of the target and the call: > > Seen the problem yet? Yup. Saw something else very interesting, too. > The long term fix is to have per-thread register caches, that is > progressing. > > I don't know about a short term fix though. I was working on a short-term fix and discovered it was almost entirely in place already. Look at a couple of random fetch_inferior_registers implementations; every one that a GNU/Linux platform uses already will fetch the LWP's registers if the LWP is non-zero. So why not give that to 'em? Leave the inferior_ptid as it is, and make fetch_inferior_registers honor the LWP id. I fixed sparc-nat.c (the fix is a little gross but holds to the style of the file); I fixed up lin-lwp; I timed a couple of tests. Look at what I got. Currently, on my desktop, to run linux-dp.exp and schedlock.exp: runtest linux-dp.exp schedlock.exp 101.06s user 50.80s system 130% cpu 1:56.27 total With change: runtest linux-dp.exp schedlock.exp 92.13s user 48.46s system 131% cpu 1:47.30 total Wait, I'm being foolish. Schedlock has some CPU-intensive loops in it so when you're trying to get CPU information for GDB it's not a good choice. Trying again with linux-dp.exp and print-threads.exp, currently: runtest linux-dp.exp print-threads.exp 17.21s user 48.22s system 82% cpu 1:19.56 total With change: runtest linux-dp.exp print-threads.exp 16.67s user 45.35s system 82% cpu 1:15.27 total Linux-dp also has some spinning and sending of SIGINTs, so again the work done isn't entirely deterministic - but that's still an interesting difference to me. And we're definitely finding the correct registers for each thread, at least on i386-linux; I'm pretty confident about other targets, too. And: it appears to fix the PR. We do so terrifyingly much reading from the inferior that it's kind of hard to tell; but normally it would have crashed by now. And I can "info threads" and print out $eip without it looping again. So, thoughts on the attached patch? -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer 2002-11-21 Daniel Jacobowitz Fix PR gdb/725 * lin-lwp.c (lin_lwp_fetch_registers): Remove. (lin_lwp_store_registers): Remove. (init_lin_lwp_ops): Use fetch_inferior_registers and store_inferior_registers directly. * sparc-nat.c (fetch_inferior_registers): Honor LWP ID. (store_inferior_registers): Likewise. Index: lin-lwp.c =================================================================== RCS file: /cvs/src/src/gdb/lin-lwp.c,v retrieving revision 1.36 diff -u -p -r1.36 lin-lwp.c --- lin-lwp.c 31 Oct 2002 21:00:08 -0000 1.36 +++ lin-lwp.c 22 Nov 2002 00:12:14 -0000 @@ -1343,32 +1343,6 @@ lin_lwp_mourn_inferior (void) child_ops.to_mourn_inferior (); } -static void -lin_lwp_fetch_registers (int regno) -{ - struct cleanup *old_chain = save_inferior_ptid (); - - if (is_lwp (inferior_ptid)) - inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid)); - - fetch_inferior_registers (regno); - - do_cleanups (old_chain); -} - -static void -lin_lwp_store_registers (int regno) -{ - struct cleanup *old_chain = save_inferior_ptid (); - - if (is_lwp (inferior_ptid)) - inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid)); - - store_inferior_registers (regno); - - do_cleanups (old_chain); -} - static int lin_lwp_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write, struct mem_attrib *attrib, @@ -1426,8 +1400,10 @@ init_lin_lwp_ops (void) lin_lwp_ops.to_detach = lin_lwp_detach; lin_lwp_ops.to_resume = lin_lwp_resume; lin_lwp_ops.to_wait = lin_lwp_wait; - lin_lwp_ops.to_fetch_registers = lin_lwp_fetch_registers; - lin_lwp_ops.to_store_registers = lin_lwp_store_registers; + /* fetch_inferior_registers and store_inferior_registers will + honor the LWP id, so we can use them directly. */ + lin_lwp_ops.to_fetch_registers = fetch_inferior_registers; + lin_lwp_ops.to_store_registers = store_inferior_registers; lin_lwp_ops.to_xfer_memory = lin_lwp_xfer_memory; lin_lwp_ops.to_kill = lin_lwp_kill; lin_lwp_ops.to_create_inferior = lin_lwp_create_inferior; Index: sparc-nat.c =================================================================== RCS file: /cvs/src/src/gdb/sparc-nat.c,v retrieving revision 1.15 diff -u -p -r1.15 sparc-nat.c --- sparc-nat.c 14 Nov 2002 20:37:29 -0000 1.15 +++ sparc-nat.c 22 Nov 2002 00:12:14 -0000 @@ -1,5 +1,6 @@ /* Functions specific to running gdb native on a SPARC running SunOS4. - Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001 + Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001, + 2002 Free Software Foundation, Inc. This file is part of GDB. @@ -58,6 +59,15 @@ fetch_inferior_registers (int regno) struct regs inferior_registers; struct fp_status inferior_fp_registers; int i; + int fetch_pid; + +#ifdef __linux__ + fetch_pid = TIDGET (inferior_ptid); + if (fetch_pid == 0) + fetch_pid = PIDGET (inferior_ptid); +#else + fetch_pid = PIDGET (inferior_ptid); +#endif /* We should never be called with deferred stores, because a prerequisite for writing regs is to have fetched them all (PREPARE_TO_STORE), sigh. */ @@ -75,7 +85,7 @@ fetch_inferior_registers (int regno) || regno >= Y_REGNUM || (!deprecated_register_valid[SP_REGNUM] && regno < I7_REGNUM)) { - if (0 != ptrace (PTRACE_GETREGS, PIDGET (inferior_ptid), + if (0 != ptrace (PTRACE_GETREGS, fetch_pid, (PTRACE_ARG3_TYPE) & inferior_registers, 0)) perror ("ptrace_getregs"); @@ -108,7 +118,7 @@ fetch_inferior_registers (int regno) regno == FPS_REGNUM || (regno >= FP0_REGNUM && regno <= FP0_REGNUM + 31)) { - if (0 != ptrace (PTRACE_GETFPREGS, PIDGET (inferior_ptid), + if (0 != ptrace (PTRACE_GETFPREGS, fetch_pid, (PTRACE_ARG3_TYPE) & inferior_fp_registers, 0)) perror ("ptrace_getfpregs"); @@ -153,6 +163,15 @@ store_inferior_registers (int regno) struct regs inferior_registers; struct fp_status inferior_fp_registers; int wanna_store = INT_REGS + STACK_REGS + FP_REGS; + int store_pid; + +#ifdef __linux__ + store_pid = TIDGET (inferior_ptid); + if (store_pid == 0) + store_pid = PIDGET (inferior_ptid); +#else + store_pid = PIDGET (inferior_ptid); +#endif /* First decide which pieces of machine-state we need to modify. Default for regno == -1 case is all pieces. */ @@ -236,7 +255,7 @@ store_inferior_registers (int regno) inferior_registers.r_y = *(int *) &deprecated_registers[REGISTER_BYTE (Y_REGNUM)]; - if (0 != ptrace (PTRACE_SETREGS, PIDGET (inferior_ptid), + if (0 != ptrace (PTRACE_SETREGS, store_pid, (PTRACE_ARG3_TYPE) & inferior_registers, 0)) perror ("ptrace_setregs"); } @@ -252,7 +271,7 @@ store_inferior_registers (int regno) &deprecated_registers[REGISTER_BYTE (FPS_REGNUM)], sizeof (FPU_FSR_TYPE)); if (0 != - ptrace (PTRACE_SETFPREGS, PIDGET (inferior_ptid), + ptrace (PTRACE_SETFPREGS, store_pid, (PTRACE_ARG3_TYPE) & inferior_fp_registers, 0)) perror ("ptrace_setfpregs"); } --------------000302090405000700030207--