[PATCH 0/2] Context-switch in finish-step-over flow

[PATCH 0/2] Context-switch in finish-step-over flow

[Tankut Baris Aktemur]

Hi,

While addressing Pedro's comments to a patch about 'stop_all_threads' at

  https://sourceware.org/pipermail/gdb-patches/2020-April/167664.html

the need for this two-part series arose.  This small series will act as
as a predecessor to the next revision of the patch linked above.

Regards.
Baris


Tankut Baris Aktemur (2):
  testsuite: introduce 'default_gdb_start_post_cmd' as an overridable
    function
  gdb/infrun: switch the context before 'displaced_step_restore'

 gdb/infrun.c                                  | 11 +++--
 gdb/testsuite/gdb.multi/run-only-second-inf.c | 22 +++++++++
 .../gdb.multi/run-only-second-inf.exp         | 49 +++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                     | 12 +++++
 4 files changed, 89 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/run-only-second-inf.c
 create mode 100644 gdb/testsuite/gdb.multi/run-only-second-inf.exp

-- 
2.17.1

[PATCH 1/2] testsuite: introduce 'default_gdb_start_post_cmd' as an overridable function

[Tankut Baris Aktemur]

Introduce a function named 'default_gdb_start_post_cmd' that is
invoked at the end of 'default_gdb_start'.  The function is by default
empty and can be overridden in a board file or a test to perform
setup right after GDB starts.

The need for this function arose from the fact that doing
'maint set target-non-stop on' was not possible when using the
native-extended-gdbserver board file.  This board file overrides
gdb_start as below:

  proc gdb_start { } {
      # Spawn GDB.
      default_gdb_start

      # And then GDBserver, ready for extended-remote mode.
      gdbserver_start_multi

      return 0
  }

That is, GDB is started and an extended-remote target is defined
immediately following it.  However, to set the remote target to
non-stop mode, we would have to issue the 'maint set target-non-stop on'
code ...

  proc gdb_start { } {
      # Spawn GDB.
      default_gdb_start

-->   ### ... right here.

      # And then GDBserver, ready for extended-remote mode.
      gdbserver_start_multi

      return 0
  }

If the target-non-stop mode is set after making the connection,
we see the following problem:

   $ gdb
   (gdb) file a.out
   ...
   (gdb) set remote exec-file a.out
   (gdb) target extended-remote | gdbserver --once --multi -
   ...
   (gdb) maint set target-non-stop on
   (gdb) run
   ...
   Unexpected vCont reply in non-stop mode: T05swbreak:;06:f0dfffffff7f0000;07:10deffffff7f0000;10:d194ddf7ff7f0000;thread:p4706.4706;core:d;

This patch provides a generalized solution and gives the ability to do
custom setup after starting GDB but before connecting to the target.

gdb/testsuite/ChangeLog:
2020-04-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* lib/gdb.exp (default_gdb_start_post_cmd): New overridable
	function.
	(default_gdb_start): Invoke 'default_gdb_start_post_cmd'
	at the end.
---
 gdb/testsuite/lib/gdb.exp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 8418c3d8753..626edee6265 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1849,6 +1849,17 @@ proc default_gdb_spawn { } {
     return 0
 }
 
+# Overridable procedure that is called at the end of
+# default_gdb_start.
+
+proc default_gdb_start_post_cmd { } {
+    # Do nothing by default.
+    # You can override this in your board file or your test.
+    #
+    # E.g. Do "maint set target-non-stop on" to start in
+    # all-stop-on-top-of-non-stop mode.
+}
+
 # Default gdb_start procedure.
 
 proc default_gdb_start { } {
@@ -1919,6 +1930,7 @@ proc default_gdb_start { } {
     }
 
     gdb_debug_init
+    default_gdb_start_post_cmd
     return 0
 }
 
-- 
2.17.1

[PATCH 2/2] gdb/infrun: switch the context before 'displaced_step_restore'

[Tankut Baris Aktemur]

In infrun.c's 'displaced_step_fixup', as part of the 'finish_step_over'
flow, switch to the eventing thread *before* calling
'displaced_step_restore', because down in the flow ptid-dependent
memory accesses are used via current_inferior() and current_top_target().

Without this patch, the problem is exposed with the scenario below:

   $ gdb -q
   (gdb) maint set target-non-stop on
   (gdb) file a.out
   Reading symbols from a.out...
   (gdb) set remote exec-file a.out
   (gdb) target extended-remote | gdbserver --once --multi -
   ...
   (gdb) add-inferior
   [New inferior 2]
   Added inferior 2 on connection 1 (extended-remote ...)
   (gdb) inferior 2
   [Switching to inferior 2 [<null>] (<noexec>)]
   (gdb) file a.out
   Reading symbols from a.out...
   (gdb) set remote exec-file a.out
   (gdb) run
   ...
   Cannot access memory at address 0x555555555042
   (gdb)

The problem is, down inside 'displaced_step_restore', GDB wants to
access the memory for inferior 2 because of an internal breakpoint.
However, the current inferior and inferior_ptid are out of sync.
While inferior_ptid correctly points to the process of inf 2 that was
just started, current_inferior points to inf 1.  Then, the attempt to
access the memory fails, because target_has_execution results in false
since inf 1 was not started.  I was not able to simplify the failing
scenario, but it shows the problem.

After this patch, we get

  ... same steps above...
  (gdb) run
  ...
  [Inferior 2 (process 28652) exited normally]
  (gdb)

Regression-tested on X86_64 Linux with `make check`s default board file
and also `--target_board=native-extended-gdbserver`.  In fact, the bug
fixed by this patch was exposed when using the native-extended-gdbserver
board file.

gdb/ChangeLog:
2020-04-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* infrun.c (displaced_step_fixup): Switch to the event_thread
	before calling displaced_step_restore, not after.

gdb/testsuite/ChangeLog:
2020-04-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	* gdb.multi/run-only-second-inf.c: New file.
	* gdb.multi/run-only-second-inf.exp: New file.
---
 gdb/infrun.c                                  | 11 +++--
 gdb/testsuite/gdb.multi/run-only-second-inf.c | 22 +++++++++
 .../gdb.multi/run-only-second-inf.exp         | 49 +++++++++++++++++++
 3 files changed, 77 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/run-only-second-inf.c
 create mode 100644 gdb/testsuite/gdb.multi/run-only-second-inf.exp

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5d60e64230b..0e1ba6986b1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1884,15 +1884,16 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
   if (displaced->step_thread != event_thread)
     return 0;
 
-  displaced_step_reset_cleanup cleanup (displaced);
-
-  displaced_step_restore (displaced, displaced->step_thread->ptid);
-
   /* Fixup may need to read memory/registers.  Switch to the thread
      that we're fixing up.  Also, target_stopped_by_watchpoint checks
-     the current thread.  */
+     the current thread, and displaced_step_restore performs ptid-dependent
+     memory accesses using current_inferior() and current_top_target().  */
   switch_to_thread (event_thread);
 
+  displaced_step_reset_cleanup cleanup (displaced);
+
+  displaced_step_restore (displaced, displaced->step_thread->ptid);
+
   /* Did the instruction complete successfully?  */
   if (signal == GDB_SIGNAL_TRAP
       && !(target_stopped_by_watchpoint ()
diff --git a/gdb/testsuite/gdb.multi/run-only-second-inf.c b/gdb/testsuite/gdb.multi/run-only-second-inf.c
new file mode 100644
index 00000000000..f4825c8a7c1
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/run-only-second-inf.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/run-only-second-inf.exp b/gdb/testsuite/gdb.multi/run-only-second-inf.exp
new file mode 100644
index 00000000000..874cdcd13e9
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/run-only-second-inf.exp
@@ -0,0 +1,49 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Create two inferiors where the first one is not running.  Then run the
+# second one.  Expect it to start normally.
+
+standard_testfile
+
+if {[use_gdb_stub]} {
+    return 0
+}
+
+proc default_gdb_start_post_cmd { } {
+    # This setting revealed a bug where the context
+    # was not being switched before making ptid-dependent
+    # memory access.
+    gdb_test_no_output "maint set target-non-stop on"
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
+    return -1
+}
+
+# Add and start the second inferior.
+gdb_test "add-inferior" "Added inferior 2.*" \
+    "add empty inferior 2"
+gdb_test "inferior 2" "Switching to inferior 2.*" \
+    "switch to inferior 2"
+gdb_load $binfile
+
+if {[gdb_start_cmd] < 0} {
+    fail "start the second inf"
+} else {
+    gdb_test "" ".*reakpoint ., main .*${srcfile}.*" "start the second inf"
+}
-- 
2.17.1

Re: [PATCH 2/2] gdb/infrun: switch the context before 'displaced_step_restore'

[Pedro Alves]

On 4/20/20 5:24 PM, Tankut Baris Aktemur via Gdb-patches wrote:

> gdb/ChangeLog:
> 2020-04-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* infrun.c (displaced_step_fixup): Switch to the event_thread
> 	before calling displaced_step_restore, not after.

Thanks.  This part looks good to me.

> 
> gdb/testsuite/ChangeLog:
> 2020-04-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* gdb.multi/run-only-second-inf.c: New file.
> 	* gdb.multi/run-only-second-inf.exp: New file.

Some comments on the testcase below.


> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Create two inferiors where the first one is not running.  Then run the
> +# second one.  

If you create two inferiors, and the first one is not running and _then_
you run the second one, then it means that the second one wasn't 
running either.  Seems like some minor rephrasing here could help.

> +
> +standard_testfile
> +
> +if {[use_gdb_stub]} {
> +    return 0
> +}
> +
> +proc default_gdb_start_post_cmd { } {
> +    # This setting revealed a bug where the context
> +    # was not being switched before making ptid-dependent
> +    # memory access.
> +    gdb_test_no_output "maint set target-non-stop on"
> +}

Sorry, but I don't really like this -- the problem is that
it's not guaranteed that each testcase runs on its own
expect/runtest instance, and thus this default_gdb_start_post_cmd
procedure can leak to other testcases.

A number of non-stop tests have a similar issue, which
is resolved by starting GDB with '-ex "set non-stop on"'.
For example, see gdb.threads/non-ldr-exc-2.exp.

I think doing the same here should work.  Something like:

        save_vars { GDBFLAGS } {
          append GDBFLAGS " -ex \"maint set target-non-stop on\""
          clean_restart ${executable}
        }

Otherwise this LGTM.

Thanks,
Pedro Alves

RE: [PATCH 2/2] gdb/infrun: switch the context before 'displaced_step_restore'

[Aktemur, Tankut Baris]

On Tuesday, April 21, 2020 4:38 PM, Pedro Alves wrote:
> On 4/20/20 5:24 PM, Tankut Baris Aktemur via Gdb-patches wrote:
> 
> > gdb/ChangeLog:
> > 2020-04-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> >
> > 	* infrun.c (displaced_step_fixup): Switch to the event_thread
> > 	before calling displaced_step_restore, not after.
> 
> Thanks.  This part looks good to me.
> 
> >
> > gdb/testsuite/ChangeLog:
> > 2020-04-20  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> >
> > 	* gdb.multi/run-only-second-inf.c: New file.
> > 	* gdb.multi/run-only-second-inf.exp: New file.
> 
> Some comments on the testcase below.
> 
> 
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +# Create two inferiors where the first one is not running.  Then run the
> > +# second one.
> 
> If you create two inferiors, and the first one is not running and _then_
> you run the second one, then it means that the second one wasn't
> running either.  Seems like some minor rephrasing here could help.
> 
> > +
> > +standard_testfile
> > +
> > +if {[use_gdb_stub]} {
> > +    return 0
> > +}
> > +
> > +proc default_gdb_start_post_cmd { } {
> > +    # This setting revealed a bug where the context
> > +    # was not being switched before making ptid-dependent
> > +    # memory access.
> > +    gdb_test_no_output "maint set target-non-stop on"
> > +}
> 
> Sorry, but I don't really like this -- the problem is that
> it's not guaranteed that each testcase runs on its own
> expect/runtest instance, and thus this default_gdb_start_post_cmd
> procedure can leak to other testcases.

Hmm, that would certainly not be desirable.

> A number of non-stop tests have a similar issue, which
> is resolved by starting GDB with '-ex "set non-stop on"'.
> For example, see gdb.threads/non-ldr-exc-2.exp.
> 
> I think doing the same here should work.  Something like:
> 
>         save_vars { GDBFLAGS } {
>           append GDBFLAGS " -ex \"maint set target-non-stop on\""
>           clean_restart ${executable}
>         }

Thanks for the hint.  I discarded the first patch, updated the second per
your comments, and pushed.

> Otherwise this LGTM.
> 
> Thanks,
> Pedro Alves

Regards.
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928