From dffdd8b51f3fb086faf750592709baa8faa66fd1 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 25 Feb 2021 15:52:29 -0500 Subject: [PATCH] gdb: relax assertion in target_mourn_inferior As reported in PR 26861, when killing an inferior on macOS, we hit the assert: ../../gdb-10.1/gdb/target.c:2149: internal-error: void target_mourn_inferior(ptid_t): Assertion `ptid == inferior_ptid' failed. This is because darwin_nat_target::kill passes a pid-only ptid to target_mourn_inferior, with the pid of the current inferior: target_mourn_inferior (ptid_t (inf->pid)); ... which doesn't satisfy the assert in target_mourn_inferior: gdb_assert (ptid == inferior_ptid); The reason for this assertion is that target_mourn_inferior is a prototype shared between GDB and GDBserver, so that shared code in gdb/nat (used in both GDB and GDBserver) can call target_mourn_inferior. In GDB's implementation, it is likely that some targets still rely on inferior_ptid being set to "the current thread we are working on". So until targets are completely decoupled from inferior_ptid (at least their mourn_inferior implementations), we need to ensure the passed in ptid matches inferior_ptid, to ensure the calling code called target_mourn_inferior with the right global context. However, I think the assert is a bit too restrictive. The mourn_inferior operation works on an inferior, not a specific thread. And by the time we call mourn_inferior, the threads of the inferior don't exist anymore, the process is gone, so it doesn't really make sense to require inferior_ptid to point a specific thread. I looked at all the target_ops::mourn_inferior implementations, those that read inferior_ptid only care about the pid field, which supports the idea that only the inferior matters. Other implementations look at the current inferior (call `current_inferior ()`). I think it would make sense to change target_mourn_inferior to accept only a pid rather than a ptid. It would then assert that the pid is the same as the current inferior's pid. However, this would be a quite involved change, so I'll keep it for later. To fix the macOS issue immediately, I propose to relax the assert to only compare the pids, as is done in this patch. Another solution would obviously be to make darwin_nat_target::kill pass inferior_ptid to target_mourn_inferior. However, the solution I propose is more in line with where I think we want to go (passing a pid to target_mourn_inferior). gdb/ChangeLog: PR gdb/26861 * target.c (target_mourn_inferior): Only compare pids in target_mourn_inferior. Change-Id: If2439ccc5aa67272ea16148a43c5362ef23fb2b8 --- gdb/ChangeLog | 6 ++++++ gdb/target.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 74e0dee1b0f..842dc0a1374 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,9 @@ +2021-02-25 Simon Marchi + + PR gdb/26861 + * target.c (target_mourn_inferior): Only compare pids in + target_mourn_inferior. + 2021-02-25 Jan Matyas PR gdb/26819 diff --git a/gdb/target.c b/gdb/target.c index ba445e7fd34..0889da82ea5 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2129,7 +2129,7 @@ default_mourn_inferior (struct target_ops *self) void target_mourn_inferior (ptid_t ptid) { - gdb_assert (ptid == inferior_ptid); + gdb_assert (ptid.pid () == inferior_ptid.pid ()); current_top_target ()->mourn_inferior (); /* We no longer need to keep handles on any of the object files. -- 2.30.2