* [PATCH 0/3] Fix thread exited messages for remote targets
@ 2023-08-21 9:31 Andrew Burgess
2023-08-21 9:31 ` [PATCH 1/3] gdb: make inferior::clear_thread_list always silent Andrew Burgess
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Andrew Burgess @ 2023-08-21 9:31 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Andrew Burgess
The point of this series is to fix the thread ... exited messages for
remote targets. I made a previous attempt to fix this issue:
https://inbox.sourceware.org/gdb-patches/0215c520c680334efac7f0544d834ec1fb7aff8e.1669634536.git.aburgess@redhat.com/
https://inbox.sourceware.org/gdb-patches/d6d49338119d2a32e060f95219aacac31bdf4886.1674207665.git.aburgess@redhat.com/
But Pedro hinted that he had a better solution as part of his step
over clone/exit work, the latest version of which can be found here:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-31-pedro@palves.net/
Unfortunately, the larger series, of which that commit is a part,
seems to have stalled. However, that particular patch doesn't really
depend on anything else in the series, and so, in this series I've
pulled out this one patch (3/3), along with one other patch on which
the above sort-of depended (1/3 - this changes a similar area of GDB).
The remaining patch (2/3) is my own work, and is an additional
refactor which I think follows naturally from the first patch.
Patches #1 and #3 are mostly Pedro's work, though, some updates were
needed during the rebase. Any bugs introduced by the rebase are mine.
---
Andrew Burgess (1):
gdb: remove the silent parameter from exit_inferior_1 and cleanup
Pedro Alves (2):
gdb: make inferior::clear_thread_list always silent
gdb: centralize "[Thread ...exited]" notifications
gdb/annotate.c | 4 +-
gdb/breakpoint.c | 4 +-
gdb/bsd-kvm.c | 2 +-
gdb/corelow.c | 2 +-
gdb/fbsd-nat.c | 3 -
gdb/gdbthread.h | 22 ++++++--
gdb/inferior.c | 35 ++++--------
gdb/inferior.h | 12 ++--
gdb/infrun.c | 2 +-
gdb/interps.c | 6 +-
gdb/interps.h | 8 ++-
gdb/linux-nat.c | 8 +--
gdb/mi/mi-interp.c | 4 +-
gdb/mi/mi-interp.h | 3 +-
gdb/netbsd-nat.c | 3 -
gdb/observable.h | 11 ++--
gdb/procfs.c | 6 --
gdb/python/py-inferior.c | 4 +-
gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp | 14 +----
.../gdb.threads/thread-bp-deleted.exp | 12 +---
gdb/thread.c | 56 +++++++++++++------
gdb/tracectf.c | 2 +-
gdb/tracefile-tfile.c | 2 +-
gdb/windows-nat.c | 20 ++-----
24 files changed, 121 insertions(+), 124 deletions(-)
base-commit: b080fe54fb3414b488b8ef323c6c50def061f918
--
2.25.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] gdb: make inferior::clear_thread_list always silent
2023-08-21 9:31 [PATCH 0/3] Fix thread exited messages for remote targets Andrew Burgess
@ 2023-08-21 9:31 ` Andrew Burgess
2023-08-21 9:31 ` [PATCH 2/3] gdb: remove the silent parameter from exit_inferior_1 and cleanup Andrew Burgess
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2023-08-21 9:31 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Andrew Burgess
From: Pedro Alves <pedro@palves.net>
After this commit:
commit a78ef8757418105c35685c5d82b9fdf79459321b
Date: Wed Jun 22 18:10:00 2022 +0100
Always emit =thread-exited notifications, even if silent
The function mi_interp::on_thread_exited (or mi_thread_exit as the
function was called back then) no longer makes use of the "silent"
parameter.
As a result there is no difference between inferior::clear_thread_list
with silent true or false, because:
- None of the interpreter ::on_thread_exited functions rely on the
silent parameter, and
- None of GDB's thread_exit observers rely on the silent parameter
either.
This commit removes the silent parameter from
inferior::clear_thread_list, and makes the function always silent.
This commit was originally part of a larger series:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-1-pedro@palves.net/
But didn't really need to be part of that series. I had an interest
in seeing this patch merged:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-31-pedro@palves.net/
Which also didn't really need to be part of the larger series, but
does depend, at least a little, on this commit. In order to get the
fix I'm interested in merged quicker, I (Andrew Burgess) have rebased
this commit outside of the original series. Any bugs introduced while
splitting this patch out and rebasing, are entirely my own.
There should be no user visible changes after this commit.
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
---
gdb/inferior.c | 12 ++++++------
gdb/inferior.h | 5 ++---
gdb/thread.c | 2 +-
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/gdb/inferior.c b/gdb/inferior.c
index eee4785fbf7..7220b704f20 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -247,13 +247,13 @@ inferior::find_thread (ptid_t ptid)
/* See inferior.h. */
void
-inferior::clear_thread_list (bool silent)
+inferior::clear_thread_list ()
{
thread_list.clear_and_dispose ([=] (thread_info *thr)
{
- threads_debug_printf ("deleting thread %s, silent = %d",
- thr->ptid.to_string ().c_str (), silent);
- set_thread_exited (thr, silent);
+ threads_debug_printf ("deleting thread %s",
+ thr->ptid.to_string ().c_str ());
+ set_thread_exited (thr, true /* silent */);
if (thr->deletable ())
delete thr;
});
@@ -272,7 +272,7 @@ notify_inferior_removed (inferior *inf)
void
delete_inferior (struct inferior *inf)
{
- inf->clear_thread_list (true);
+ inf->clear_thread_list ();
auto it = inferior_list.iterator_to (*inf);
inferior_list.erase (it);
@@ -307,7 +307,7 @@ notify_inferior_disappeared (inferior *inf)
static void
exit_inferior_1 (struct inferior *inf, int silent)
{
- inf->clear_thread_list (silent);
+ inf->clear_thread_list ();
notify_inferior_disappeared (inf);
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 74578353482..c8429b86268 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -496,9 +496,8 @@ class inferior : public refcounted_object,
/* Find (non-exited) thread PTID of this inferior. */
thread_info *find_thread (ptid_t ptid);
- /* Delete all threads in the thread list. If SILENT, exit threads
- silently. */
- void clear_thread_list (bool silent);
+ /* Delete all threads in the thread list, silently. */
+ void clear_thread_list ();
/* Continuations-related methods. A continuation is an std::function
to be called to finish the execution of a command when running
diff --git a/gdb/thread.c b/gdb/thread.c
index 63ed87e9aa6..2cb9e5e36d0 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -246,7 +246,7 @@ init_thread_list (void)
highest_thread_num = 0;
for (inferior *inf : all_inferiors ())
- inf->clear_thread_list (true);
+ inf->clear_thread_list ();
}
/* Allocate a new thread of inferior INF with target id PTID and add
--
2.25.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] gdb: remove the silent parameter from exit_inferior_1 and cleanup
2023-08-21 9:31 [PATCH 0/3] Fix thread exited messages for remote targets Andrew Burgess
2023-08-21 9:31 ` [PATCH 1/3] gdb: make inferior::clear_thread_list always silent Andrew Burgess
@ 2023-08-21 9:31 ` Andrew Burgess
2023-08-21 9:31 ` [PATCH 3/3] gdb: centralize "[Thread ...exited]" notifications Andrew Burgess
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2023-08-21 9:31 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Andrew Burgess
After the previous commit, exit_inferior_1 no longer makes use of the
silent parameter. This commit removes this parameter and cleans up
the callers.
After doing this exit_inferior_1, exit_inferior, and
exit_inferior_silent are all equivalent, so rename exit_inferior_1 to
exit_inferior and delete exit_inferior_silent, update all the callers.
Also I spotted the declaration exit_inferior_num_silent in inferior.h,
but this function is not defined anywhere, so I deleted the
declaration.
There should be no user visible changes after this commit.
---
gdb/bsd-kvm.c | 2 +-
gdb/corelow.c | 2 +-
gdb/inferior.c | 23 +++++------------------
gdb/inferior.h | 7 +++----
gdb/infrun.c | 2 +-
gdb/tracectf.c | 2 +-
gdb/tracefile-tfile.c | 2 +-
7 files changed, 13 insertions(+), 27 deletions(-)
diff --git a/gdb/bsd-kvm.c b/gdb/bsd-kvm.c
index 49041ef99bf..7fb51dcc7f2 100644
--- a/gdb/bsd-kvm.c
+++ b/gdb/bsd-kvm.c
@@ -154,7 +154,7 @@ bsd_kvm_target::close ()
bsd_kvm_corefile.clear ();
switch_to_no_thread ();
- exit_inferior_silent (current_inferior ());
+ exit_inferior (current_inferior ());
}
static LONGEST
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 46bb1077b6d..4d692dc53e6 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -328,7 +328,7 @@ core_target::clear_core ()
{
switch_to_no_thread (); /* Avoid confusion from thread
stuff. */
- exit_inferior_silent (current_inferior ());
+ exit_inferior (current_inferior ());
/* Clear out solib state while the bfd is still open. See
comments in clear_solib in solib.c. */
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 7220b704f20..cf4caa923cf 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -301,11 +301,10 @@ notify_inferior_disappeared (inferior *inf)
gdb::observers::inferior_exit.notify (inf);
}
-/* If SILENT then be quiet -- don't announce a inferior exit, or the
- exit of its threads. */
+/* See inferior.h. */
-static void
-exit_inferior_1 (struct inferior *inf, int silent)
+void
+exit_inferior (struct inferior *inf)
{
inf->clear_thread_list ();
@@ -335,27 +334,15 @@ exit_inferior_1 (struct inferior *inf, int silent)
reinit_frame_cache ();
}
-void
-exit_inferior (inferior *inf)
-{
- exit_inferior_1 (inf, 0);
-}
-
-void
-exit_inferior_silent (inferior *inf)
-{
- exit_inferior_1 (inf, 1);
-}
-
/* See inferior.h. */
void
detach_inferior (inferior *inf)
{
- /* Save the pid, since exit_inferior_1 will reset it. */
+ /* Save the pid, since exit_inferior will reset it. */
int pid = inf->pid;
- exit_inferior_1 (inf, 0);
+ exit_inferior (inf);
if (print_inferior_events)
gdb_printf (_("[Inferior %d (%s) detached]\n"),
diff --git a/gdb/inferior.h b/gdb/inferior.h
index c8429b86268..04672582984 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -704,12 +704,11 @@ extern void delete_inferior (struct inferior *todel);
/* Delete an existing inferior list entry, due to inferior detaching. */
extern void detach_inferior (inferior *inf);
+/* Notify observers and interpreters that INF has gone away. Reset the INF
+ object back to an default, empty, state. Clear register and frame
+ caches. */
extern void exit_inferior (inferior *inf);
-extern void exit_inferior_silent (inferior *inf);
-
-extern void exit_inferior_num_silent (int num);
-
extern void inferior_appeared (struct inferior *inf, int pid);
/* Search function to lookup an inferior of TARG by target 'pid'. */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 72852e63906..4730d290442 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1328,7 +1328,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
following_inferior = add_inferior_with_spaces ();
swap_terminal_info (following_inferior, execing_inferior);
- exit_inferior_silent (execing_inferior);
+ exit_inferior (execing_inferior);
following_inferior->pid = pid;
}
diff --git a/gdb/tracectf.c b/gdb/tracectf.c
index 0ed2092c32b..1f3ec58227f 100644
--- a/gdb/tracectf.c
+++ b/gdb/tracectf.c
@@ -1187,7 +1187,7 @@ ctf_target::close ()
trace_dirname.reset ();
switch_to_no_thread (); /* Avoid confusion from thread stuff. */
- exit_inferior_silent (current_inferior ());
+ exit_inferior (current_inferior ());
trace_reset_local_state ();
}
diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 59055e7ea1a..3440f375021 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -616,7 +616,7 @@ tfile_target::close ()
gdb_assert (trace_fd != -1);
switch_to_no_thread (); /* Avoid confusion from thread stuff. */
- exit_inferior_silent (current_inferior ());
+ exit_inferior (current_inferior ());
::close (trace_fd);
trace_fd = -1;
--
2.25.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] gdb: centralize "[Thread ...exited]" notifications
2023-08-21 9:31 [PATCH 0/3] Fix thread exited messages for remote targets Andrew Burgess
2023-08-21 9:31 ` [PATCH 1/3] gdb: make inferior::clear_thread_list always silent Andrew Burgess
2023-08-21 9:31 ` [PATCH 2/3] gdb: remove the silent parameter from exit_inferior_1 and cleanup Andrew Burgess
@ 2023-08-21 9:31 ` Andrew Burgess
2023-08-21 16:10 ` [PATCH 0/3] Fix thread exited messages for remote targets John Baldwin
2023-08-22 13:02 ` Pedro Alves
4 siblings, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2023-08-21 9:31 UTC (permalink / raw)
To: gdb-patches; +Cc: Pedro Alves, Andrew Burgess
From: Pedro Alves <pedro@palves.net>
Currently, each target backend is responsible for printing "[Thread
...exited]" before deleting a thread. This leads to unnecessary
differences between targets, like e.g. with the remote target, we
never print such messages, even though we do print "[New Thread ...]".
E.g., debugging the gdb.threads/attach-many-short-lived-threads.exp
with gdbserver, letting it run for a bit, and then pressing Ctrl-C, we
currently see:
(gdb) c
Continuing.
^C[New Thread 3850398.3887449]
[New Thread 3850398.3887500]
[New Thread 3850398.3887551]
[New Thread 3850398.3887602]
[New Thread 3850398.3887653]
...
Thread 1 "attach-many-sho" received signal SIGINT, Interrupt.
0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80)
at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c
(gdb)
Above, we only see "New Thread" notifications, even though threads
were deleted.
After this patch, we'll see:
(gdb) c
Continuing.
^C[Thread 3558643.3577053 exited]
[Thread 3558643.3577104 exited]
[Thread 3558643.3577155 exited]
[Thread 3558643.3579603 exited]
...
[New Thread 3558643.3597415]
[New Thread 3558643.3600015]
[New Thread 3558643.3599965]
...
Thread 1 "attach-many-sho" received signal SIGINT, Interrupt.
0x00007ffff7e6a23f in __GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7fffffffda80, rem=rem@entry=0x7fffffffda80)
at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
78 in ../sysdeps/unix/sysv/linux/clock_nanosleep.c
(gdb) q
This commit fixes this by moving the thread exit printing to common
code instead, triggered from within delete_thread (or rather,
set_thread_exited).
There's one wrinkle, though. While most targest want to print:
[Thread ... exited]
the Windows target wants to print:
[Thread ... exited with code <exit_code>]
... and sometimes wants to suppress the notification for the main
thread. To address that, this commits adds a delete_thread_with_code
function, only used by that target (so far).
This fix was originally posted as part of a larger series:
https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-1-pedro@palves.net/
But didn't really need to be part of that series. In order to get
this fix merged sooner, I (Andrew Burgess) have rebased this commit
outside of the original series. Any bugs introduced while splitting
this patch out and rebasing, are entirely my own.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30129
Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
---
gdb/annotate.c | 4 +-
gdb/breakpoint.c | 4 +-
gdb/fbsd-nat.c | 3 --
gdb/gdbthread.h | 22 ++++++--
gdb/inferior.c | 2 +-
gdb/interps.c | 6 ++-
gdb/interps.h | 8 ++-
gdb/linux-nat.c | 8 +--
gdb/mi/mi-interp.c | 4 +-
gdb/mi/mi-interp.h | 3 +-
gdb/netbsd-nat.c | 3 --
gdb/observable.h | 11 ++--
gdb/procfs.c | 6 ---
gdb/python/py-inferior.c | 4 +-
gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp | 14 ++---
.../gdb.threads/thread-bp-deleted.exp | 12 +----
gdb/thread.c | 54 +++++++++++++------
gdb/windows-nat.c | 20 +++----
18 files changed, 100 insertions(+), 88 deletions(-)
diff --git a/gdb/annotate.c b/gdb/annotate.c
index d403a47ba2f..8385429042d 100644
--- a/gdb/annotate.c
+++ b/gdb/annotate.c
@@ -232,7 +232,9 @@ annotate_thread_changed (void)
/* Emit notification on thread exit. */
static void
-annotate_thread_exited (struct thread_info *t, int silent)
+annotate_thread_exited (thread_info *t,
+ gdb::optional<ULONGEST> exit_code,
+ bool /* silent */)
{
if (annotation_level > 1)
{
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f88ca1c9b65..4ff344b5f56 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -3362,7 +3362,9 @@ remove_breakpoints (void)
that thread. */
static void
-remove_threaded_breakpoints (struct thread_info *tp, int silent)
+remove_threaded_breakpoints (thread_info *tp,
+ gdb::optional<ULONGEST> /* exit_code */,
+ int /* silent */)
{
for (breakpoint &b : all_breakpoints_safe ())
{
diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
index 41ca1f61561..0ee3bccb5ef 100644
--- a/gdb/fbsd-nat.c
+++ b/gdb/fbsd-nat.c
@@ -1394,9 +1394,6 @@ fbsd_nat_target::wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
{
fbsd_lwp_debug_printf ("deleting thread for LWP %u",
pl.pl_lwpid);
- if (print_thread_events)
- gdb_printf (_("[%s exited]\n"),
- target_pid_to_str (wptid).c_str ());
low_delete_thread (thr);
delete_thread (thr);
fbsd_inf->num_lwps--;
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index baff68aee92..48f32bb3a0b 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -623,16 +623,30 @@ extern struct thread_info *add_thread_with_info (process_stratum_target *targ,
/* Delete thread THREAD and notify of thread exit. If the thread is
currently not deletable, don't actually delete it but still tag it
- as exited and do the notification. */
-extern void delete_thread (struct thread_info *thread);
+ as exited and do the notification. EXIT_CODE is the thread's exit
+ code. If SILENT, don't actually notify the CLI. THREAD must not
+ be NULL or an assertion will fail. */
+extern void delete_thread_with_exit_code (thread_info *thread,
+ ULONGEST exit_code,
+ bool silent = false);
+
+/* Delete thread THREAD and notify of thread exit. If the thread is
+ currently not deletable, don't actually delete it but still tag it
+ as exited and do the notification. THREAD must not be NULL or an
+ assertion will fail. */
+extern void delete_thread (thread_info *thread);
/* Like delete_thread, but be quiet about it. Used when the process
this thread belonged to has already exited, for example. */
extern void delete_thread_silent (struct thread_info *thread);
/* Mark the thread exited, but don't delete it or remove it from the
- inferior thread list. */
-extern void set_thread_exited (thread_info *tp, bool silent);
+ inferior thread list. EXIT_CODE is the thread's exit code, if
+ available. If SILENT, then don't inform the CLI about the
+ exit. */
+extern void set_thread_exited (thread_info *tp,
+ gdb::optional<ULONGEST> exit_code = {},
+ bool silent = false);
/* Delete a step_resume_breakpoint from the thread database. */
extern void delete_step_resume_breakpoint (struct thread_info *);
diff --git a/gdb/inferior.c b/gdb/inferior.c
index cf4caa923cf..ce4960a508a 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -253,7 +253,7 @@ inferior::clear_thread_list ()
{
threads_debug_printf ("deleting thread %s",
thr->ptid.to_string ().c_str ());
- set_thread_exited (thr, true /* silent */);
+ set_thread_exited (thr, {}, true /* silent */);
if (thr->deletable ())
delete thr;
});
diff --git a/gdb/interps.c b/gdb/interps.c
index f665e86b65c..7baa8491eb1 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -457,9 +457,11 @@ interps_notify_new_thread (thread_info *t)
/* See interps.h. */
void
-interps_notify_thread_exited (thread_info *t, int silent)
+interps_notify_thread_exited (thread_info *t,
+ gdb::optional<ULONGEST> exit_code,
+ int silent)
{
- interps_notify (&interp::on_thread_exited, t, silent);
+ interps_notify (&interp::on_thread_exited, t, exit_code, silent);
}
/* See interps.h. */
diff --git a/gdb/interps.h b/gdb/interps.h
index 3a0e8372e10..c041d0d95b6 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -122,7 +122,9 @@ class interp : public intrusive_list_node<interp>
virtual void on_new_thread (thread_info *t) {}
/* Notify the interpreter that thread T has exited. */
- virtual void on_thread_exited (thread_info *, int silent) {}
+ virtual void on_thread_exited (thread_info *,
+ gdb::optional<ULONGEST> exit_code,
+ int silent) {}
/* Notify the interpreter that inferior INF was added. */
virtual void on_inferior_added (inferior *inf) {}
@@ -297,7 +299,9 @@ extern void interps_notify_user_selected_context_changed
extern void interps_notify_new_thread (thread_info *t);
/* Notify all interpreters that thread T has exited. */
-extern void interps_notify_thread_exited (thread_info *t, int silent);
+extern void interps_notify_thread_exited (thread_info *t,
+ gdb::optional<ULONGEST> exit_code,
+ int silent);
/* Notify all interpreters that inferior INF was added. */
extern void interps_notify_inferior_added (inferior *inf);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 250a8f43282..e58d67183b5 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -906,13 +906,7 @@ exit_lwp (struct lwp_info *lp)
struct thread_info *th = linux_target->find_thread (lp->ptid);
if (th)
- {
- if (print_thread_events)
- gdb_printf (_("[%s exited]\n"),
- target_pid_to_str (lp->ptid).c_str ());
-
- delete_thread (th);
- }
+ delete_thread (th);
delete_lwp (lp->ptid);
}
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 0e51c884a65..6a26da123c6 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -277,7 +277,9 @@ mi_interp::on_new_thread (thread_info *t)
}
void
-mi_interp::on_thread_exited (thread_info *t, int silent)
+mi_interp::on_thread_exited (thread_info *t,
+ gdb::optional<ULONGEST> /* exit_code */,
+ int /* silent */)
{
target_terminal::scoped_restore_terminal_state term_state;
target_terminal::ours_for_output ();
diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h
index 6326ec7d524..bc975d75ad8 100644
--- a/gdb/mi/mi-interp.h
+++ b/gdb/mi/mi-interp.h
@@ -51,7 +51,8 @@ class mi_interp final : public interp
void on_command_error () override;
void on_user_selected_context_changed (user_selected_what selection) override;
void on_new_thread (thread_info *t) override;
- void on_thread_exited (thread_info *t, int silent) override;
+ void on_thread_exited (thread_info *t, gdb::optional<ULONGEST> exit_code,
+ int silent) override;
void on_inferior_added (inferior *inf) override;
void on_inferior_appeared (inferior *inf) override;
void on_inferior_disappeared (inferior *inf) override;
diff --git a/gdb/netbsd-nat.c b/gdb/netbsd-nat.c
index 86df0c8fe6a..d1614ec0783 100644
--- a/gdb/netbsd-nat.c
+++ b/gdb/netbsd-nat.c
@@ -626,9 +626,6 @@ nbsd_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
/* NetBSD does not store an LWP exit status. */
ourstatus->set_thread_exited (0);
- if (print_thread_events)
- gdb_printf (_("[%s exited]\n"),
- target_pid_to_str (wptid).c_str ());
delete_thread (thr);
}
diff --git a/gdb/observable.h b/gdb/observable.h
index f6047a239cd..c0bafc51f14 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -109,10 +109,13 @@ extern observable<struct objfile */* objfile */> free_objfile;
/* The thread specified by T has been created. */
extern observable<struct thread_info */* t */> new_thread;
-/* The thread specified by T has exited. The SILENT argument
- indicates that gdb is removing the thread from its tables without
- wanting to notify the user about it. */
-extern observable<struct thread_info */* t */, int /* silent */> thread_exit;
+/* The thread specified by T has exited. EXIT_CODE is the thread's
+ exit code, if available. The SILENT argument indicates that GDB is
+ removing the thread from its tables without wanting to notify the
+ CLI about it. */
+extern observable<thread_info */* t */,
+ gdb::optional<ULONGEST> /* exit_code */,
+ bool /* silent */> thread_exit;
/* An explicit stop request was issued to PTID. If PTID equals
minus_one_ptid, the request applied to all threads. If
diff --git a/gdb/procfs.c b/gdb/procfs.c
index f0c2c2cf56c..d55a16a9fb5 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -2117,9 +2117,6 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
case PR_SYSENTRY:
if (what == SYS_lwp_exit)
{
- if (print_thread_events)
- gdb_printf (_("[%s exited]\n"),
- target_pid_to_str (retval).c_str ());
delete_thread (this->find_thread (retval));
proc_resume (pi, ptid, 0, GDB_SIGNAL_0);
goto wait_again;
@@ -2223,9 +2220,6 @@ procfs_target::wait (ptid_t ptid, struct target_waitstatus *status,
}
else if (what == SYS_lwp_exit)
{
- if (print_thread_events)
- gdb_printf (_("[%s exited]\n"),
- target_pid_to_str (retval).c_str ());
delete_thread (this->find_thread (retval));
status->set_spurious ();
return retval;
diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 792f05b118e..906d402827c 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -361,7 +361,9 @@ add_thread_object (struct thread_info *tp)
}
static void
-delete_thread_object (struct thread_info *tp, int ignore)
+delete_thread_object (thread_info *tp,
+ gdb::optional<ULONGEST> /* exit_code */,
+ bool /* silent */)
{
if (!gdb_python_initialized)
return;
diff --git a/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
index 0ebca924801..bffbb290071 100644
--- a/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
+++ b/gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp
@@ -235,17 +235,9 @@ foreach_mi_ui_mode mode {
# The output has arrived! Check how we did. There are other bugs
# that come into play here which change what output we'll see.
- if { $saw_mi_thread_exited && $saw_mi_bp_deleted \
- && $saw_cli_thread_exited \
- && $saw_cli_bp_deleted } {
- kpass "gdb/30129" $gdb_test_name
- } elseif { $saw_mi_thread_exited && $saw_mi_bp_deleted \
- && !$saw_cli_thread_exited \
- && $saw_cli_bp_deleted } {
- kfail "gdb/30129" $gdb_test_name
- } else {
- fail "$gdb_test_name"
- }
+ gdb_assert { $saw_mi_thread_exited && $saw_mi_bp_deleted \
+ && $saw_cli_thread_exited \
+ && $saw_cli_bp_deleted } $gdb_test_name
}
}
diff --git a/gdb/testsuite/gdb.threads/thread-bp-deleted.exp b/gdb/testsuite/gdb.threads/thread-bp-deleted.exp
index 019bdddee81..92178ff838b 100644
--- a/gdb/testsuite/gdb.threads/thread-bp-deleted.exp
+++ b/gdb/testsuite/gdb.threads/thread-bp-deleted.exp
@@ -155,17 +155,7 @@ if {$is_remote} {
exp_continue
}
- # When PR gdb/30129 is fixed then this can all be collapsed down
- # into a single gdb_assert call. This is split out like this
- # because the SAW_BP_DELETED part is working, and we want to
- # spot if that stops working.
- if { $saw_thread_exited && $saw_bp_deleted } {
- kpass "gdb/30129" $gdb_test_name
- } elseif {!$saw_thread_exited && $saw_bp_deleted} {
- kfail "gdb/30129" $gdb_test_name
- } else {
- fail $gdb_test_name
- }
+ gdb_assert { $saw_thread_exited && $saw_bp_deleted } $gdb_test_name
}
}
} else {
diff --git a/gdb/thread.c b/gdb/thread.c
index 2cb9e5e36d0..c8145da59bc 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -194,16 +194,29 @@ clear_thread_inferior_resources (struct thread_info *tp)
/* Notify interpreters and observers that thread T has exited. */
static void
-notify_thread_exited (thread_info *t, int silent)
+notify_thread_exited (thread_info *t, gdb::optional<ULONGEST> exit_code,
+ int silent)
{
- interps_notify_thread_exited (t, silent);
- gdb::observers::thread_exit.notify (t, silent);
+ if (!silent && print_thread_events)
+ {
+ if (exit_code.has_value ())
+ gdb_printf (_("[%s exited with code %s]\n"),
+ target_pid_to_str (t->ptid).c_str (),
+ pulongest (*exit_code));
+ else
+ gdb_printf (_("[%s exited]\n"),
+ target_pid_to_str (t->ptid).c_str ());
+ }
+
+ interps_notify_thread_exited (t, exit_code, silent);
+ gdb::observers::thread_exit.notify (t, exit_code, silent);
}
/* See gdbthread.h. */
void
-set_thread_exited (thread_info *tp, bool silent)
+set_thread_exited (thread_info *tp, gdb::optional<ULONGEST> exit_code,
+ bool silent)
{
/* Dead threads don't need to step-over. Remove from chain. */
if (thread_is_in_step_over_chain (tp))
@@ -222,7 +235,7 @@ set_thread_exited (thread_info *tp, bool silent)
if (proc_target != nullptr)
proc_target->maybe_remove_resumed_with_pending_wait_status (tp);
- notify_thread_exited (tp, silent);
+ notify_thread_exited (tp, exit_code, silent);
/* Tag it as exited. */
tp->state = THREAD_EXITED;
@@ -470,20 +483,22 @@ global_thread_step_over_chain_remove (struct thread_info *tp)
global_thread_step_over_list.erase (it);
}
-/* Delete the thread referenced by THR. If SILENT, don't notify
- the observer of this exit.
-
- THR must not be NULL or a failed assertion will be raised. */
+/* Helper for the different delete_thread variants. */
static void
-delete_thread_1 (thread_info *thr, bool silent)
+delete_thread_1 (thread_info *thr, gdb::optional<ULONGEST> exit_code,
+ bool silent)
{
gdb_assert (thr != nullptr);
- threads_debug_printf ("deleting thread %s, silent = %d",
- thr->ptid.to_string ().c_str (), silent);
+ threads_debug_printf ("deleting thread %s, exit_code = %s, silent = %d",
+ thr->ptid.to_string ().c_str (),
+ (exit_code.has_value ()
+ ? pulongest (*exit_code)
+ : "<none>"),
+ silent);
- set_thread_exited (thr, silent);
+ set_thread_exited (thr, exit_code, silent);
if (!thr->deletable ())
{
@@ -499,16 +514,25 @@ delete_thread_1 (thread_info *thr, bool silent)
/* See gdbthread.h. */
+void
+delete_thread_with_exit_code (thread_info *thread, ULONGEST exit_code,
+ bool silent)
+{
+ delete_thread_1 (thread, exit_code, silent);
+}
+
+/* See gdbthread.h. */
+
void
delete_thread (thread_info *thread)
{
- delete_thread_1 (thread, false /* not silent */);
+ delete_thread_1 (thread, {}, false /* not silent */);
}
void
delete_thread_silent (thread_info *thread)
{
- delete_thread_1 (thread, true /* silent */);
+ delete_thread_1 (thread, {}, true /* not silent */);
}
struct thread_info *
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index c09e459a801..d8b33452f46 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -612,21 +612,13 @@ windows_nat_target::delete_thread (ptid_t ptid, DWORD exit_code,
id = ptid.lwp ();
- /* Emit a notification about the thread being deleted.
+ /* Note that no notification was printed when the main thread was
+ created, and thus, unless in verbose mode, we should be symmetrical,
+ and avoid an exit notification for the main thread here as well. */
- Note that no notification was printed when the main thread
- was created, and thus, unless in verbose mode, we should be
- symmetrical, and avoid that notification for the main thread
- here as well. */
-
- if (info_verbose)
- gdb_printf ("[Deleting %s]\n", target_pid_to_str (ptid).c_str ());
- else if (print_thread_events && !main_thread_p)
- gdb_printf (_("[%s exited with code %u]\n"),
- target_pid_to_str (ptid).c_str (),
- (unsigned) exit_code);
-
- ::delete_thread (this->find_thread (ptid));
+ bool silent = (main_thread_p && !info_verbose);
+ thread_info *to_del = this->find_thread (ptid);
+ delete_thread_with_exit_code (to_del, exit_code, silent);
auto iter = std::find_if (windows_process.thread_list.begin (),
windows_process.thread_list.end (),
--
2.25.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Fix thread exited messages for remote targets
2023-08-21 9:31 [PATCH 0/3] Fix thread exited messages for remote targets Andrew Burgess
` (2 preceding siblings ...)
2023-08-21 9:31 ` [PATCH 3/3] gdb: centralize "[Thread ...exited]" notifications Andrew Burgess
@ 2023-08-21 16:10 ` John Baldwin
2023-08-22 13:07 ` Pedro Alves
2023-08-22 13:02 ` Pedro Alves
4 siblings, 1 reply; 9+ messages in thread
From: John Baldwin @ 2023-08-21 16:10 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches; +Cc: Pedro Alves
On 8/21/23 2:31 AM, Andrew Burgess via Gdb-patches wrote:
> The point of this series is to fix the thread ... exited messages for
> remote targets. I made a previous attempt to fix this issue:
>
> https://inbox.sourceware.org/gdb-patches/0215c520c680334efac7f0544d834ec1fb7aff8e.1669634536.git.aburgess@redhat.com/
> https://inbox.sourceware.org/gdb-patches/d6d49338119d2a32e060f95219aacac31bdf4886.1674207665.git.aburgess@redhat.com/
>
> But Pedro hinted that he had a better solution as part of his step
> over clone/exit work, the latest version of which can be found here:
>
> https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-31-pedro@palves.net/
>
> Unfortunately, the larger series, of which that commit is a part,
> seems to have stalled. However, that particular patch doesn't really
> depend on anything else in the series, and so, in this series I've
> pulled out this one patch (3/3), along with one other patch on which
> the above sort-of depended (1/3 - this changes a similar area of GDB).
>
> The remaining patch (2/3) is my own work, and is an additional
> refactor which I think follows naturally from the first patch.
>
> Patches #1 and #3 are mostly Pedro's work, though, some updates were
> needed during the rebase. Any bugs introduced by the rebase are mine.
Thanks for this cleanup. The asymmetry of what targets had to do here
was kind of odd.
One question I have is why is the optional metadata specifically a
ULONGEST exit code rather than just a std::string suffix? If it were
a string then windows-nat.c could use something like:
std::string info = string_printf ("with code %u", (unsigned) exit_code).
delete_thread_with_info (to_del, info, silent);
--
John Baldwin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Fix thread exited messages for remote targets
2023-08-21 9:31 [PATCH 0/3] Fix thread exited messages for remote targets Andrew Burgess
` (3 preceding siblings ...)
2023-08-21 16:10 ` [PATCH 0/3] Fix thread exited messages for remote targets John Baldwin
@ 2023-08-22 13:02 ` Pedro Alves
2023-08-23 9:12 ` Andrew Burgess
4 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2023-08-22 13:02 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 2023-08-21 10:31, Andrew Burgess wrote:
> The point of this series is to fix the thread ... exited messages for
> remote targets. I made a previous attempt to fix this issue:
>
> https://inbox.sourceware.org/gdb-patches/0215c520c680334efac7f0544d834ec1fb7aff8e.1669634536.git.aburgess@redhat.com/
> https://inbox.sourceware.org/gdb-patches/d6d49338119d2a32e060f95219aacac31bdf4886.1674207665.git.aburgess@redhat.com/
>
> But Pedro hinted that he had a better solution as part of his step
> over clone/exit work, the latest version of which can be found here:
>
> https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-31-pedro@palves.net/
>
> Unfortunately, the larger series, of which that commit is a part,
> seems to have stalled.
Yeah, sorry, I saw your reviews a few weeks ago, so I started reviewing your infcalls series as
a way to "pay" for your reviews, before addressing your comments in my series, but
then summer vacations and other priorities got in the way. I still have every interest to get that
series merged.
> However, that particular patch doesn't really
> depend on anything else in the series, and so, in this series I've
> pulled out this one patch (3/3), along with one other patch on which
> the above sort-of depended (1/3 - this changes a similar area of GDB).
Thank you.
>
> The remaining patch (2/3) is my own work, and is an additional
> refactor which I think follows naturally from the first patch.
>
> Patches #1 and #3 are mostly Pedro's work, though, some updates were
> needed during the rebase. Any bugs introduced by the rebase are mine.
>
All looks good to me, please go ahead and merge it.
Pedro Alves
> ---
>
> Andrew Burgess (1):
> gdb: remove the silent parameter from exit_inferior_1 and cleanup
>
> Pedro Alves (2):
> gdb: make inferior::clear_thread_list always silent
> gdb: centralize "[Thread ...exited]" notifications
>
> gdb/annotate.c | 4 +-
> gdb/breakpoint.c | 4 +-
> gdb/bsd-kvm.c | 2 +-
> gdb/corelow.c | 2 +-
> gdb/fbsd-nat.c | 3 -
> gdb/gdbthread.h | 22 ++++++--
> gdb/inferior.c | 35 ++++--------
> gdb/inferior.h | 12 ++--
> gdb/infrun.c | 2 +-
> gdb/interps.c | 6 +-
> gdb/interps.h | 8 ++-
> gdb/linux-nat.c | 8 +--
> gdb/mi/mi-interp.c | 4 +-
> gdb/mi/mi-interp.h | 3 +-
> gdb/netbsd-nat.c | 3 -
> gdb/observable.h | 11 ++--
> gdb/procfs.c | 6 --
> gdb/python/py-inferior.c | 4 +-
> gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp | 14 +----
> .../gdb.threads/thread-bp-deleted.exp | 12 +---
> gdb/thread.c | 56 +++++++++++++------
> gdb/tracectf.c | 2 +-
> gdb/tracefile-tfile.c | 2 +-
> gdb/windows-nat.c | 20 ++-----
> 24 files changed, 121 insertions(+), 124 deletions(-)
>
>
> base-commit: b080fe54fb3414b488b8ef323c6c50def061f918
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Fix thread exited messages for remote targets
2023-08-21 16:10 ` [PATCH 0/3] Fix thread exited messages for remote targets John Baldwin
@ 2023-08-22 13:07 ` Pedro Alves
2023-08-22 16:54 ` John Baldwin
0 siblings, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2023-08-22 13:07 UTC (permalink / raw)
To: John Baldwin, Andrew Burgess, gdb-patches
On 2023-08-21 17:10, John Baldwin wrote:
> One question I have is why is the optional metadata specifically a
> ULONGEST exit code rather than just a std::string suffix? If it were
> a string then windows-nat.c could use something like:
>
> std::string info = string_printf ("with code %u", (unsigned) exit_code).
> delete_thread_with_info (to_del, info, silent);
>
The idea was that if other targets want to print something, it'll probably
be the thread exit code as well, so keeping the text formatting centralized
ensures any such target prints it in the same way. For example, the remote
target's "thread exit" stop reply is "wNN", but we're not printing that thread
exit code today. Remote debugging with Windows gdbserver ideally would want
to print it, I think. On Linux, we also have access to the lwp's exit code,
though I think that in practice it's always 0.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Fix thread exited messages for remote targets
2023-08-22 13:07 ` Pedro Alves
@ 2023-08-22 16:54 ` John Baldwin
0 siblings, 0 replies; 9+ messages in thread
From: John Baldwin @ 2023-08-22 16:54 UTC (permalink / raw)
To: Pedro Alves, Andrew Burgess, gdb-patches
On 8/22/23 6:07 AM, Pedro Alves wrote:
> On 2023-08-21 17:10, John Baldwin wrote:
>
>> One question I have is why is the optional metadata specifically a
>> ULONGEST exit code rather than just a std::string suffix? If it were
>> a string then windows-nat.c could use something like:
>>
>> std::string info = string_printf ("with code %u", (unsigned) exit_code).
>> delete_thread_with_info (to_del, info, silent);
>>
>
> The idea was that if other targets want to print something, it'll probably
> be the thread exit code as well, so keeping the text formatting centralized
> ensures any such target prints it in the same way. For example, the remote
> target's "thread exit" stop reply is "wNN", but we're not printing that thread
> exit code today. Remote debugging with Windows gdbserver ideally would want
> to print it, I think. On Linux, we also have access to the lwp's exit code,
> though I think that in practice it's always 0.
Ok, fair enough.
--
John Baldwin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Fix thread exited messages for remote targets
2023-08-22 13:02 ` Pedro Alves
@ 2023-08-23 9:12 ` Andrew Burgess
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2023-08-23 9:12 UTC (permalink / raw)
To: gdb-patches
Pedro Alves <pedro@palves.net> writes:
> On 2023-08-21 10:31, Andrew Burgess wrote:
>> The point of this series is to fix the thread ... exited messages for
>> remote targets. I made a previous attempt to fix this issue:
>>
>> https://inbox.sourceware.org/gdb-patches/0215c520c680334efac7f0544d834ec1fb7aff8e.1669634536.git.aburgess@redhat.com/
>> https://inbox.sourceware.org/gdb-patches/d6d49338119d2a32e060f95219aacac31bdf4886.1674207665.git.aburgess@redhat.com/
>>
>> But Pedro hinted that he had a better solution as part of his step
>> over clone/exit work, the latest version of which can be found here:
>>
>> https://inbox.sourceware.org/gdb-patches/20221212203101.1034916-31-pedro@palves.net/
>>
>> Unfortunately, the larger series, of which that commit is a part,
>> seems to have stalled.
>
> Yeah, sorry, I saw your reviews a few weeks ago, so I started reviewing your infcalls series as
> a way to "pay" for your reviews, before addressing your comments in my series, but
> then summer vacations and other priorities got in the way. I still have every interest to get that
> series merged.
>
>> However, that particular patch doesn't really
>> depend on anything else in the series, and so, in this series I've
>> pulled out this one patch (3/3), along with one other patch on which
>> the above sort-of depended (1/3 - this changes a similar area of GDB).
>
> Thank you.
>
>>
>> The remaining patch (2/3) is my own work, and is an additional
>> refactor which I think follows naturally from the first patch.
>>
>> Patches #1 and #3 are mostly Pedro's work, though, some updates were
>> needed during the rebase. Any bugs introduced by the rebase are mine.
>>
>
> All looks good to me, please go ahead and merge it.
Pushed.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-08-23 9:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-21 9:31 [PATCH 0/3] Fix thread exited messages for remote targets Andrew Burgess
2023-08-21 9:31 ` [PATCH 1/3] gdb: make inferior::clear_thread_list always silent Andrew Burgess
2023-08-21 9:31 ` [PATCH 2/3] gdb: remove the silent parameter from exit_inferior_1 and cleanup Andrew Burgess
2023-08-21 9:31 ` [PATCH 3/3] gdb: centralize "[Thread ...exited]" notifications Andrew Burgess
2023-08-21 16:10 ` [PATCH 0/3] Fix thread exited messages for remote targets John Baldwin
2023-08-22 13:07 ` Pedro Alves
2023-08-22 16:54 ` John Baldwin
2023-08-22 13:02 ` Pedro Alves
2023-08-23 9:12 ` Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).