btrace: avoid tp != NULL assertion
Commit Message
On 03/03/2015 10:49 AM, Metzger, Markus T wrote:
> I have no idea how regcache->ptid ended up with lwp=0.
>
> Here's the full backtrace in case it helps.
Yes it does.
> #6 0x0827a106 in i386_linux_resume (ops=0x9737ca0 <linux_ops_saved>, ptid=..., step=1,
> signal=GDB_SIGNAL_0) at gdb/i386-linux-nat.c:670
> #7 0x08280c12 in linux_resume_one_lwp (lp=0x9a0a5b8, step=1, signo=GDB_SIGNAL_0)
> at gdb/linux-nat.c:1529
Frame #7 where we convert the ptid to something inf-ptrace.c understands.
The fix is just to stop losing information. This also improves
performance a tiny bit, as currently we fetch registers out of ptrace
twice, once for {pid,lwp} in linux-nat.c, and another for {pid,0} in
inf-ptrace/i386-linux-nat.c. We'll now use a single regcache object
everywhere.
Please try the patch below.
Note that this in your patch:
+ lwp = ptid_get_lwp (ptid);
+ if (lwp == 0)
+ {
+ int pid;
+
+ /* Not a threaded program. Use the PID as LWP ID. */
+ pid = ptid_get_pid (ptid);
+ ptid = ptid_build (pid, pid, 0);
+ }
+
+ return find_thread_ptid (ptid);
+}
+
is wrong because when debugging a multi-threaded program, you'll
have e.g. ({pid,lwp}) {1,1}, {1,2}, {1,3}, etc. Your patch makes
btrace look for {2,2}, {3,3}, etc. in the thread list, which just
aren't there, and thus return NULL. I assume that that's why you
needed to downgrade the asserts to errors.
----
From 6f4e842f2cfefa18d789d2b658e2719fa5609e60 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 3 Mar 2015 11:03:34 +0000
Subject: [PATCH] No longer hack ptids when passing the request down to the
inf-ptrace layer
---
gdb/i386-linux-nat.c | 5 ++---
gdb/inf-ptrace.c | 25 ++++++++++++++++++++++---
gdb/linux-nat.c | 6 +-----
3 files changed, 25 insertions(+), 11 deletions(-)
Comments
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Tuesday, March 3, 2015 12:55 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH] btrace: avoid tp != NULL assertion
> Frame #7 where we convert the ptid to something inf-ptrace.c understands.
>
> The fix is just to stop losing information. This also improves
> performance a tiny bit, as currently we fetch registers out of ptrace
> twice, once for {pid,lwp} in linux-nat.c, and another for {pid,0} in
> inf-ptrace/i386-linux-nat.c. We'll now use a single regcache object
> everywhere.
>
> Please try the patch below.
It works. Thanks. I'll drop my patch, then.
> Note that this in your patch:
>
> + lwp = ptid_get_lwp (ptid);
> + if (lwp == 0)
> + {
> + int pid;
> +
> + /* Not a threaded program. Use the PID as LWP ID. */
> + pid = ptid_get_pid (ptid);
> + ptid = ptid_build (pid, pid, 0);
> + }
> +
> + return find_thread_ptid (ptid);
> +}
> +
>
> is wrong because when debugging a multi-threaded program, you'll
> have e.g. ({pid,lwp}) {1,1}, {1,2}, {1,3}, etc. Your patch makes
> btrace look for {2,2}, {3,3}, etc. in the thread list, which just
> aren't there, and thus return NULL. I assume that that's why you
> needed to downgrade the asserts to errors.
No, that wasn't the reason for replacing the assert. There are no such
errors in the gdb.btrace suite (which is mostly single-threaded) with my
patch and I have not seen any such errors otherwise, either. I just found
it safer and more user-friendly in case we run into further problems in
this area.
I see what you mean, though...
> - /* Convert to something the lower layer understands. */
> - ptid = pid_to_ptid (ptid_get_lwp (lp->ptid));
> - linux_ops->to_resume (linux_ops, ptid, step, signo);
Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
@@ -648,8 +648,7 @@ static void
i386_linux_resume (struct target_ops *ops,
ptid_t ptid, int step, enum gdb_signal signal)
{
- int pid = ptid_get_pid (ptid);
-
+ int pid = ptid_get_lwp (ptid);
int request;
if (catch_syscall_enabled () > 0)
@@ -659,7 +658,7 @@ i386_linux_resume (struct target_ops *ops,
if (step)
{
- struct regcache *regcache = get_thread_regcache (pid_to_ptid (pid));
+ struct regcache *regcache = get_thread_regcache (ptid);
struct gdbarch *gdbarch = get_regcache_arch (regcache);
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
ULONGEST pc;
@@ -289,6 +289,22 @@ inf_ptrace_stop (struct target_ops *self, ptid_t ptid)
kill (-inferior_process_group (), SIGINT);
}
+/* Return which PID to pass to ptrace in order to observe/control the
+ tracee identified by PTID. */
+
+static pid_t
+get_ptrace_pid (ptid_t ptid)
+{
+ pid_t pid;
+
+ /* If we have an LWPID to work with, use it. Otherwise, we're
+ dealing with a non-threaded program/target. */
+ pid = ptid_get_lwp (ptid);
+ if (pid == 0)
+ pid = ptid_get_pid (ptid);
+ return pid;
+}
+
/* Resume execution of thread PTID, or all threads if PTID is -1. If
STEP is nonzero, single-step it. If SIGNAL is nonzero, give it
that signal. */
@@ -297,13 +313,16 @@ static void
inf_ptrace_resume (struct target_ops *ops,
ptid_t ptid, int step, enum gdb_signal signal)
{
- pid_t pid = ptid_get_pid (ptid);
+ pid_t pid;
+
int request;
- if (pid == -1)
+ if (ptid_equal (minus_one_ptid, ptid))
/* Resume all threads. Traditionally ptrace() only supports
single-threaded processes, so simply resume the inferior. */
- pid = ptid_get_pid (inferior_ptid);
+ pid = get_ptrace_pid (inferior_ptid);
+ else
+ pid = get_ptrace_pid (ptid);
if (catch_syscall_enabled () > 0)
request = PT_SYSCALL;
@@ -1506,8 +1506,6 @@ linux_nat_detach (struct target_ops *ops, const char *args, int from_tty)
static void
linux_resume_one_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
{
- ptid_t ptid;
-
lp->step = step;
/* stop_pc doubles as the PC the LWP had when it was last resumed.
@@ -1524,9 +1522,7 @@ linux_resume_one_lwp (struct lwp_info *lp, int step, enum gdb_signal signo)
if (linux_nat_prepare_to_resume != NULL)
linux_nat_prepare_to_resume (lp);
- /* Convert to something the lower layer understands. */
- ptid = pid_to_ptid (ptid_get_lwp (lp->ptid));
- linux_ops->to_resume (linux_ops, ptid, step, signo);
+ linux_ops->to_resume (linux_ops, lp->ptid, step, signo);
lp->stop_reason = LWP_STOPPED_BY_NO_REASON;
lp->stopped = 0;
registers_changed_ptid (lp->ptid);