From: Pedro Alves <pedro@palves.net>
To: Andrew Burgess <aburgess@redhat.com>, gdb-patches@sourceware.org
Subject: [PATCH v2] gdb+gdbserver/Linux: avoid reading registers while going through shell (was: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection)
Date: Tue, 28 Jun 2022 13:42:51 +0100 [thread overview]
Message-ID: <d30eeb12-d222-cc95-1776-8a003ace9b05@palves.net> (raw)
In-Reply-To: <87a69xdqfo.fsf@redhat.com>
On 2022-06-28 11:37, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
>
>> On 2022-06-27 17:27, Andrew Burgess wrote:
>>> Pedro Alves <pedro@palves.net> writes:
>> So I gave the patch below a try, and it seems to works well -- readcache_read_pc
>> is no longer called before target_find_description, and I saw no testsuite
>> regressions (this is without your series, just pristine master).
>>
>> Now, this patch as is makes sense for linux-nat.c, but then I went to do the
>> same thing for gdbserver, which has an equivalent save_stop_reason in linux-low.cc,
>> and there this approach won't work as is for the simple fact that lwp->stop_pc
>> is used in a lot more places.
>>
>> So to avoid gdb vs gdbserver divergence, it may be better to not take this patch
>> as is, but instead tweak save_stop_reason to avoid reading the stop pc while
>> the inferior is going through the shell, based on some per-inferior flag or some
>> such. Maybe we can repurpose progspace->executing_startup for this? That
>> flag used to be used by Cell (which is gone), but seems unused nowadays. There's
>> also other similar flags in inferior itself, like needs_setup, and
>> in_initial_library_scan. So we'd add a flag like one of those, set it while the
>> inferior is going through the startup_inferior dance, and make save_stop_reason avoid
>> reading the PC while that flag is set.
...
>
> Thanks, this looks good. Maybe you should go ahead and merge this, and
> the attach problem can be solved separately.
There's the gdb vs gdbserver divergence issue I pointed out above, though.
I was thinking we'd instead fix it with the alternative patch below, which
works for both gdb and gdbserver.
Tested with no regressions on x86-64 Ubuntu 20.04, native and gdbserver.
WDYT?
From 1b7b50e7c8359553af8b8858a82f09d5389d4a1b Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 27 Jun 2022 20:41:50 +0100
Subject: [PATCH] gdb+gdbserver/Linux: avoid reading registers while going
through shell
For every stop, linux-nat.c saves the stopped thread's PC, in
lwp->stop_pc. This is done in save_stop_reason, in both
gdb/linux-nat.c and gdbserver/linux-low.cc. However, while we're
going through the shell after "run", in startup_inferior, we shouldn't
be reading registers, as we haven't yet determined the target's
architecture -- the shell's architecture may not even be the same as
the final inferior's.
In linux-nat.c, lwp->stop_pc is only needed when the thread has
stopped for a breakpoint, and since when going through the shell, no
breakpoint is going to hit, we could simply teach save_stop_reason to
only record the stop pc when the thread stopped for a breakpoint.
However, in gdbserver/linux-low.cc, lwp->stop_pc is used in more cases
than breakpoint hits (e.g., it's used in tracepoints & the
"while-stepping" feature).
So to avoid GDB vs GDBserver divergence, we apply the same approach to
both implementations.
We set a flag in the inferior (process in gdbserver) whenever it is
being nursed through the shell, and when that flag is set,
save_stop_reason bails out early. While going through the shell,
we'll only ever get process exits (normal or signalled), and exec
events, so nothing is lost.
Change-Id: If0f01831514d3a74d17efd102875de7d2c6401ad
---
gdb/fork-child.c | 3 +++
gdb/inferior.h | 7 +++++++
gdb/linux-nat.c | 4 ++++
gdbserver/fork-child.cc | 5 +++++
gdbserver/inferiors.h | 7 +++++++
gdbserver/linux-low.cc | 16 ++++++++++++----
6 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/gdb/fork-child.c b/gdb/fork-child.c
index 89003fa617d..66b523e118d 100644
--- a/gdb/fork-child.c
+++ b/gdb/fork-child.c
@@ -126,6 +126,9 @@ gdb_startup_inferior (pid_t pid, int num_traps)
inferior *inf = current_inferior ();
process_stratum_target *proc_target = inf->process_target ();
+ scoped_restore save_starting_up
+ = make_scoped_restore (&inf->starting_up, true);
+
ptid_t ptid = startup_inferior (proc_target, pid, num_traps, NULL, NULL);
/* Mark all threads non-executing. */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index f6e26a32feb..c376d780de0 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -551,6 +551,13 @@ class inferior : public refcounted_object,
architecture/description. */
bool needs_setup = false;
+ /* True if the inferior is starting up (inside startup_inferior),
+ and we're nursing it along (through the shell) until it is ready
+ to execute its first instruction. Until that is done, we must
+ not access inferior memory or registers, as we haven't determined
+ the target architecture/description. */
+ bool starting_up = false;
+
/* True when we are reading the library list of the inferior during an
attach or handling a fork child. */
bool in_initial_library_scan = false;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 008791c12dc..9ae02dfc093 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2523,6 +2523,10 @@ save_stop_reason (struct lwp_info *lp)
gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
gdb_assert (lp->status != 0);
+ inferior *inf = find_inferior_ptid (linux_target, lp->ptid);
+ if (inf->starting_up)
+ return;
+
if (!linux_target->low_status_is_event (lp->status))
return;
diff --git a/gdbserver/fork-child.cc b/gdbserver/fork-child.cc
index 96dd4d009ab..7ea66f2a435 100644
--- a/gdbserver/fork-child.cc
+++ b/gdbserver/fork-child.cc
@@ -18,6 +18,7 @@
#include "server.h"
#include "gdbsupport/job-control.h"
+#include "gdbsupport/scoped_restore.h"
#include "nat/fork-inferior.h"
#ifdef HAVE_SIGNAL_H
#include <signal.h>
@@ -103,6 +104,10 @@ post_fork_inferior (int pid, const char *program)
atexit (restore_old_foreground_pgrp);
#endif
+ process_info *proc = find_process_pid (pid);
+ scoped_restore save_starting_up
+ = make_scoped_restore (&proc->starting_up, true);
+
startup_inferior (the_target, pid,
START_INFERIOR_TRAPS_EXPECTED,
&cs.last_status, &cs.last_ptid);
diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h
index f3ba4d82f71..6de746cb228 100644
--- a/gdbserver/inferiors.h
+++ b/gdbserver/inferiors.h
@@ -75,6 +75,13 @@ struct process_info
/* Flag to mark that the DLL list has changed. */
bool dlls_changed = false;
+
+ /* True if the inferior is starting up (inside startup_inferior),
+ and we're nursing it along (through the shell) until it is ready
+ to execute its first instruction. Until that is done, we must
+ not access inferior memory or registers, as we haven't determined
+ the target architecture/description. */
+ bool starting_up = false;
};
/* Get the pid of PROC. */
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 9305928bbbf..547e56f6448 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -747,8 +747,8 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
CORE_ADDR
linux_process_target::get_pc (lwp_info *lwp)
{
- struct regcache *regcache;
- CORE_ADDR pc;
+ process_info *proc = get_thread_process (get_lwp_thread (lwp));
+ gdb_assert (!proc->starting_up);
if (!low_supports_breakpoints ())
return 0;
@@ -756,8 +756,8 @@ linux_process_target::get_pc (lwp_info *lwp)
scoped_restore_current_thread restore_thread;
switch_to_thread (get_lwp_thread (lwp));
- regcache = get_thread_regcache (current_thread, 1);
- pc = low_get_pc (regcache);
+ struct regcache *regcache = get_thread_regcache (current_thread, 1);
+ CORE_ADDR pc = low_get_pc (regcache);
threads_debug_printf ("pc is 0x%lx", (long) pc);
@@ -795,6 +795,14 @@ linux_process_target::save_stop_reason (lwp_info *lwp)
if (!low_supports_breakpoints ())
return false;
+ process_info *proc = get_thread_process (get_lwp_thread (lwp));
+ if (proc->starting_up)
+ {
+ /* Claim we have the stop PC so that the caller doesn't try to
+ fetch it itself. */
+ return true;
+ }
+
pc = get_pc (lwp);
sw_breakpoint_pc = pc - low_decr_pc_after_break ();
base-commit: dbcbf67ca565ec29f13a2302dcdf9b01ef7832ca
prerequisite-patch-id: 9656309f5c298727d3cf993bed0bb8e00c9e9d56
--
2.36.0
next prev parent reply other threads:[~2022-06-28 12:42 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-31 14:30 [PATCH 0/5] Handle trying to use a native target with the wrong binary Andrew Burgess
2022-05-31 14:30 ` [PATCH 1/5] gdb/arm: move fetch of arm_gdbarch_tdep to a more inner scope Andrew Burgess
2022-06-01 7:58 ` Luis Machado
2022-05-31 14:30 ` [PATCH 2/5] gdb/mips: rewrite show_mask_address Andrew Burgess
2022-05-31 14:30 ` [PATCH 3/5] gdb: move the type cast into gdbarch_tdep Andrew Burgess
2022-06-01 8:01 ` Luis Machado
2022-05-31 14:30 ` [PATCH 4/5] gdb: ensure the cast in gdbarch_tdep is valid Andrew Burgess
2022-05-31 16:04 ` John Baldwin
2022-05-31 17:22 ` Andrew Burgess
2022-05-31 14:30 ` [PATCH 5/5] gdb: native target invalid architecture detection Andrew Burgess
2022-05-31 16:08 ` John Baldwin
2022-05-31 16:51 ` Andrew Burgess
2022-06-01 8:25 ` Luis Machado
2022-06-01 21:06 ` John Baldwin
2022-06-01 21:21 ` Christophe Lyon
2022-06-02 14:56 ` John Baldwin
2022-06-06 14:38 ` Andrew Burgess
2022-06-06 17:48 ` Andrew Burgess
2022-06-07 11:03 ` Luis Machado
2022-06-07 18:42 ` Pedro Alves
2022-06-07 20:15 ` Pedro Alves
2022-06-08 8:18 ` Luis Machado
2022-06-08 10:17 ` Pedro Alves
2022-06-08 7:54 ` Luis Machado
2022-06-08 10:12 ` Pedro Alves
2022-06-08 11:20 ` [PATCH v2] aarch64: Add fallback if ARM_CC_FOR_TARGET not set (was: Re: [PATCH 5/5] gdb: native target invalid architecture detection) Pedro Alves
2022-06-08 12:50 ` Luis Machado
2022-06-08 13:23 ` Pedro Alves
2022-06-08 13:38 ` Andrew Burgess
2022-06-08 19:01 ` John Baldwin
2022-06-08 21:48 ` Pedro Alves
2022-06-09 16:31 ` John Baldwin
2022-06-10 13:08 ` [PATCHv2 0/6] Handle trying to use a native target with the wrong binary Andrew Burgess
2022-06-10 13:08 ` [PATCHv2 1/6] gdb/arm: move fetch of arm_gdbarch_tdep to a more inner scope Andrew Burgess
2022-06-10 13:08 ` [PATCHv2 2/6] gdb/mips: rewrite show_mask_address Andrew Burgess
2022-06-10 13:08 ` [PATCHv2 3/6] gdb/arm: avoid undefined behaviour in arm_frame_is_thumb Andrew Burgess
2022-06-10 15:21 ` Luis Machado
2022-06-10 15:49 ` Andrew Burgess
2022-06-10 16:29 ` Luis Machado
2022-06-10 13:08 ` [PATCHv2 4/6] gdb: move the type cast into gdbarch_tdep Andrew Burgess
2022-06-10 16:35 ` Luis Machado
2022-06-10 13:08 ` [PATCHv2 5/6] gdb: ensure the cast in gdbarch_tdep is valid Andrew Burgess
2022-06-10 13:08 ` [PATCHv2 6/6] gdb: native target invalid architecture detection Andrew Burgess
2022-06-10 16:20 ` John Baldwin
2022-06-10 16:31 ` Luis Machado
2022-06-13 16:15 ` [PATCHv3 0/6] Handle trying to use a native target with the wrong binary Andrew Burgess
2022-06-13 16:15 ` [PATCHv3 1/6] gdb/arm: move fetch of arm_gdbarch_tdep to a more inner scope Andrew Burgess
2022-06-13 16:15 ` [PATCHv3 2/6] gdb/mips: rewrite show_mask_address Andrew Burgess
2022-06-13 16:15 ` [PATCHv3 3/6] gdb: select suitable thread for gdbarch_adjust_breakpoint_address Andrew Burgess
2022-06-14 9:45 ` Luis Machado
2022-06-14 14:05 ` Andrew Burgess
2022-06-24 16:58 ` Pedro Alves
2022-06-13 16:15 ` [PATCHv3 4/6] gdb: move the type cast into gdbarch_tdep Andrew Burgess
2022-06-13 16:15 ` [PATCHv3 5/6] gdb: ensure the cast in gdbarch_tdep is valid Andrew Burgess
2022-06-24 18:15 ` Pedro Alves
2022-06-13 16:15 ` [PATCHv3 6/6] gdb: native target invalid architecture detection Andrew Burgess
2022-06-24 19:23 ` Pedro Alves
2022-06-27 16:27 ` Andrew Burgess
2022-06-27 21:38 ` Pedro Alves
2022-06-28 10:37 ` Andrew Burgess
2022-06-28 12:42 ` Pedro Alves [this message]
2022-06-28 14:21 ` [PATCH v2] gdb+gdbserver/Linux: avoid reading registers while going through shell (was: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection) Andrew Burgess
2022-06-29 15:17 ` Simon Marchi
2022-06-29 16:22 ` [PATCH] Fix GDBserver regression due to change to avoid reading shell registers Pedro Alves
2022-06-29 16:38 ` Simon Marchi
2022-06-30 9:33 ` [PATCHv3 6/6] gdb: native target invalid architecture detection Andrew Burgess
2022-06-30 11:44 ` Pedro Alves
2022-07-11 10:47 ` Andrew Burgess
2022-06-24 10:15 ` [PATCHv3 0/6] Handle trying to use a native target with the wrong binary Andrew Burgess
2022-06-28 14:28 ` [PATCHv4 0/6] Detect invalid casts of gdbarch_tdep structures Andrew Burgess
2022-06-28 14:28 ` [PATCHv4 1/6] gdb/arm: move fetch of arm_gdbarch_tdep to a more inner scope Andrew Burgess
2022-06-28 14:28 ` [PATCHv4 2/6] gdb/mips: rewrite show_mask_address Andrew Burgess
2022-06-28 14:28 ` [PATCHv4 3/6] gdb: select suitable thread for gdbarch_adjust_breakpoint_address Andrew Burgess
2022-06-28 14:28 ` [PATCHv4 4/6] gdb: move the type cast into gdbarch_tdep Andrew Burgess
2022-06-28 14:28 ` [PATCHv4 5/6] gdbsupport: add checked_static_cast Andrew Burgess
2022-06-28 14:28 ` [PATCHv4 6/6] gdb: ensure the cast in gdbarch_tdep is valid Andrew Burgess
2022-07-11 10:46 ` [PATCHv4 0/6] Detect invalid casts of gdbarch_tdep structures Andrew Burgess
2022-07-21 18:21 ` Andrew Burgess
2022-07-22 0:50 ` Luis Machado
2022-07-23 0:02 ` [PATCH] Rename gdbarch_tdep template function to gdbarch_tdep_cast for g++ 4.8 Mark Wielaard
2022-07-25 11:19 ` Andrew Burgess
2022-07-25 11:27 ` Mark Wielaard
2022-07-26 11:05 ` Andrew Burgess
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d30eeb12-d222-cc95-1776-8a003ace9b05@palves.net \
--to=pedro@palves.net \
--cc=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).