[v2,02/24] Don't rely on inferior_ptid in record_full_wait

Message ID 20191017225026.30496-3-palves@redhat.com
State New
Headers show
Series
  • Multi-target support
Related show

Commit Message

Pedro Alves Oct. 17, 2019, 10:50 p.m.
The multi-target patch sets inferior_ptid to null_ptid before handling
a target event, and thus before calling target_wait, in order to catch
places in target_ops::wait implementations that are incorrectly
relying on inferior_ptid (which could otherwise be a ptid of a
different target, for example).  That caught this instance in
record-full.c.

Fix it by saving the last resumed ptid, and then using it in
record_full_wait_1, just like how the last "step" argument passed to
record_full_target::resume is handled too.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* record-full.c (record_full_resume_ptid): New global.
	(record_full_target::resume): Set it.
	(record_full_wait_1): Use record_full_resume_ptid instead of
	inferior_ptid.
---
 gdb/record-full.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.14.5

Comments

Tom Tromey Nov. 1, 2019, 2:54 p.m. | #1
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> The multi-target patch sets inferior_ptid to null_ptid before handling
Pedro> a target event, and thus before calling target_wait, in order to catch
Pedro> places in target_ops::wait implementations that are incorrectly
Pedro> relying on inferior_ptid (which could otherwise be a ptid of a
Pedro> different target, for example).

I think it would be good to add a comment before target_ops::wait
explaining what is required from its implementation.  If other
target_ops methods also cannot rely on inferior_ptid, then that
documentation should be updated as well.  This would make it simpler to
know how to update an existing target, or to write a new target.

Tom
Pedro Alves Dec. 20, 2019, 5:49 p.m. | #2
On 11/1/19 2:54 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> The multi-target patch sets inferior_ptid to null_ptid before handling

> Pedro> a target event, and thus before calling target_wait, in order to catch

> Pedro> places in target_ops::wait implementations that are incorrectly

> Pedro> relying on inferior_ptid (which could otherwise be a ptid of a

> Pedro> different target, for example).

> 

> I think it would be good to add a comment before target_ops::wait

> explaining what is required from its implementation.  If other

> target_ops methods also cannot rely on inferior_ptid, then that

> documentation should be updated as well.  This would make it simpler to

> know how to update an existing target, or to write a new target.


How does this sound?

diff --git i/gdb/target.h w/gdb/target.h
index 1bb7276673..14a7d3e61f 100644
--- i/gdb/target.h
+++ w/gdb/target.h
@@ -478,6 +478,13 @@ struct target_ops
       TARGET_DEFAULT_NORETURN (noprocess ());
     virtual void commit_resume ()
       TARGET_DEFAULT_IGNORE ();
+    /* See target_wait's description.  Note that implementations of
+       this method must not assume that inferior_ptid on entry is
+       pointing at the thread or inferior that ends up reporting an
+       event.  The reported event could be for some other thread in
+       the current inferior or even for a different process of the
+       current target.  inferior_ptid may also be null_ptid on
+       entry.  */
     virtual ptid_t wait (ptid_t, struct target_waitstatus *,
                         int TARGET_DEBUG_PRINTER (target_debug_print_options))
       TARGET_DEFAULT_FUNC (default_target_wait);


Thanks,
Pedro Alves
Tom Tromey Dec. 20, 2019, 6:57 p.m. | #3
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> How does this sound?

Pedro> diff --git i/gdb/target.h w/gdb/target.h
Pedro> index 1bb7276673..14a7d3e61f 100644
Pedro> --- i/gdb/target.h
Pedro> +++ w/gdb/target.h
Pedro> @@ -478,6 +478,13 @@ struct target_ops
Pedro>        TARGET_DEFAULT_NORETURN (noprocess ());
Pedro>      virtual void commit_resume ()
Pedro>        TARGET_DEFAULT_IGNORE ();
Pedro> +    /* See target_wait's description.  Note that implementations of
Pedro> +       this method must not assume that inferior_ptid on entry is
Pedro> +       pointing at the thread or inferior that ends up reporting an
Pedro> +       event.  The reported event could be for some other thread in
Pedro> +       the current inferior or even for a different process of the
Pedro> +       current target.  inferior_ptid may also be null_ptid on
Pedro> +       entry.  */
Pedro>      virtual ptid_t wait (ptid_t, struct target_waitstatus *,
Pedro>                          int TARGET_DEBUG_PRINTER (target_debug_print_options))
Pedro>        TARGET_DEFAULT_FUNC (default_target_wait);

Looks good, thanks.

Tom

Patch

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 0c6cb62163..5168b0b61c 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1036,6 +1036,9 @@  record_full_base_target::async (int enable)
   beneath ()->async (enable);
 }
 
+/* The PTID and STEP arguments last passed to
+   record_full_target::resume.  */
+static ptid_t record_full_resume_ptid = null_ptid;
 static int record_full_resume_step = 0;
 
 /* True if we've been resumed, and so each record_full_wait call should
@@ -1064,6 +1067,7 @@  static enum exec_direction_kind record_full_execution_dir = EXEC_FORWARD;
 void
 record_full_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
 {
+  record_full_resume_ptid = inferior_ptid;
   record_full_resume_step = step;
   record_full_resumed = 1;
   record_full_execution_dir = ::execution_direction;
@@ -1190,7 +1194,8 @@  record_full_wait_1 (struct target_ops *ops,
 	  /* This is not a single step.  */
 	  ptid_t ret;
 	  CORE_ADDR tmp_pc;
-	  struct gdbarch *gdbarch = target_thread_architecture (inferior_ptid);
+	  struct gdbarch *gdbarch
+	    = target_thread_architecture (record_full_resume_ptid);
 
 	  while (1)
 	    {