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

Message ID 1fb90a91b0adbbc61cdbc4e1a6420763a005a93b.1587399322.git.tankut.baris.aktemur@intel.com
State New
Headers show
Series
  • Context-switch in finish-step-over flow
Related show

Commit Message

Simon Marchi via Gdb-patches April 20, 2020, 4:24 p.m.
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

-- 
2.17.1

Comments

Simon Marchi via Gdb-patches April 21, 2020, 2:38 p.m. | #1
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
Simon Marchi via Gdb-patches April 21, 2020, 3:29 p.m. | #2
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

Patch

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"
+}