Re: [Fwd: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]]

Andrew Cagney
2002-11-27T20:55:54Z

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

Michael (I suspect Mark is in bed by now),

Is there any chance that the attached could be reviewed over the next
few hours?

Daniel (who is off this week -> us long weekend) noted that the #ifdef vis:

+#ifdef __linux__
+ fetch_pid = TIDGET (inferior_ptid);
+ if (fetch_pid == 0)
+ fetch_pid = PIDGET (inferior_ptid);
+#else
+ fetch_pid = PIDGET (inferior_ptid);
+#endif

isn't needed. The code should be unconditional. I can fix/commit that
provided the patch is ok.

It is effectively the last remaining fix for 5.3.

Andrew

[-- Attachment #2: mailbox-message://ac131313@movemail/fsf/gdb/patches#11953735 --]
[-- Type: message/rfc822, Size: 14090 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 368 bytes --]

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

[-- Attachment #2.1.2: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)] --]
[-- Type: message/rfc822, Size: 10321 bytes --]

From: Daniel Jacobowitz <drow@mvista.com>
To: Andrew Cagney <ac131313@redhat.com>
Cc: gdb@sources.redhat.com
Subject: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]
Date: Thu, 21 Nov 2002 19:14:47 -0500
Message-ID: <20021122001447.GA7884@nevyn.them.org>

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 <drow@mvista.com>

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");
}