From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18642 invoked by alias); 22 Nov 2002 02:37:28 -0000 Mailing-List: contact gdb-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-owner@sources.redhat.com Received: (qmail 18590 invoked from network); 22 Nov 2002 02:37:27 -0000 Received: from unknown (HELO crack.them.org) (65.125.64.184) by sources.redhat.com with SMTP; 22 Nov 2002 02:37:27 -0000 Received: from nevyn.them.org ([66.93.61.169] ident=mail) by crack.them.org with asmtp (Exim 3.12 #1 (Debian)) id 18F5ZW-0000CP-00; Thu, 21 Nov 2002 22:37:42 -0600 Received: from drow by nevyn.them.org with local (Exim 3.36 #1 (Debian)) id 18F3h6-0004mN-00; Thu, 21 Nov 2002 21:37:24 -0500 Date: Thu, 21 Nov 2002 18:37:00 -0000 From: Daniel Jacobowitz To: Andrew Cagney , msnyder@redhat.com, kettenis@gnu.org Cc: gdb@sources.redhat.com Subject: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)] Message-ID: <20021122023724.GA13441@nevyn.them.org> Mail-Followup-To: Andrew Cagney , msnyder@redhat.com, kettenis@gnu.org, gdb@sources.redhat.com References: <3DDD6150.10407@redhat.com> <20021122001447.GA7884@nevyn.them.org> <3DDD7D8E.2010407@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3DDD7D8E.2010407@redhat.com> User-Agent: Mutt/1.5.1i X-SW-Source: 2002-11/txt/msg00324.txt.bz2 On Thu, Nov 21, 2002 at 07:42:54PM -0500, Andrew Cagney wrote: > >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. > > [snip] > > >>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 > > Given that the numbers are being overwhelmed by all those memory read > ptrace calls, a ~5% improvement is significant. > > Try something simpler like running gdb under strace (tweak > testsuite/lib/gdb.exp to run 'strace $GDB' instead of $GDB) and then > count how many ptrace calls of each type occure. This was an excercise in patience; I had to mess with the timeouts, too. An initial strace log (of JUST ptrace calls!) was 221 MB. Once I bumped the timeouts high enough to run all of print-threads.exp without DejaGNU getting bored and failing, it was even bigger: 918 MB. As you noticed in your next message, most of these are PEEKTEXT's. This patch does still make a tiny bit of difference, though. Without the patch, the test takes about 16 minutes, has a 918MB log, shows the one failure that I expected to see in print-threads.exp (still haven't figured that one out), and: 2113 PTRACE_??? 18 PTRACE_ATTACH 233 PTRACE_CONT 18033347 PTRACE_PEEKTEXT 121 PTRACE_PEEKUSER 694 PTRACE_POKEDATA 8 PTRACE_POKETEXT 56 PTRACE_SINGLESTEP With the patch the log was 480K smaller (i.e. the same size :), the run took roughly the same time (not surprising, most of it is writing out the log), same one failure, and: 2107 PTRACE_??? 18 PTRACE_ATTACH 241 PTRACE_CONT 18031743 PTRACE_PEEKTEXT 121 PTRACE_PEEKUSER 694 PTRACE_POKEDATA 8 PTRACE_POKETEXT 56 PTRACE_SINGLESTEP The patch saved 6 PTRACE_??? calls and 1604 PTRACE_PEEKTEXT calls. OK, I'm not overwhelmed here. The ??? are PTRACE_GETREGS/PTRACE_SETREGS, of course. > > >> 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. > > It feels right. I'm hopeing that, eventually, the code will supply the > registers direct to a (one of many) `struct thread_info' object. > > >So, thoughts on the attached patch? > > Thread maintainer question (not so sure about the #ifdef linux though :-). The #ifdef can just go actually. The only operating system which uses that file and threads is Linux, and when the BSDs get thread debugging support I'd like to see them behave the same way, if they have multiple LWPs. Don't know anything about BSD threads. So, thread maintainers, any opinion? Here's the updated 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 02:35:31 -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 02:35:31 -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,11 @@ fetch_inferior_registers (int regno) struct regs inferior_registers; struct fp_status inferior_fp_registers; int i; + int fetch_pid; + + fetch_pid = TIDGET (inferior_ptid); + if (fetch_pid == 0) + fetch_pid = PIDGET (inferior_ptid); /* 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 +81,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 +114,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 +159,11 @@ 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; + + store_pid = TIDGET (inferior_ptid); + if (store_pid == 0) + store_pid = PIDGET (inferior_ptid); /* First decide which pieces of machine-state we need to modify. Default for regno == -1 case is all pieces. */ @@ -236,7 +247,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 +263,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"); }