[binutils-gdb] Fix adjust_pc_after_break, remove still current thread check

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0703599a49d082a957ee233fe018fb6ea7864920

commit 0703599a49d082a957ee233fe018fb6ea7864920
Author: Pedro Alves <palves@redhat.com>
Date:   Wed Feb 11 09:45:41 2015 +0000

    Fix adjust_pc_after_break, remove still current thread check
    
    On decr_pc_after_break targets, GDB adjusts the PC incorrectly if a
    background single-step stops somewhere where PC-$decr_pc has a
    breakpoint, and the thread that finishes the step is not the current
    thread, like:
    
       ADDR1 nop <-- breakpoint here
       ADDR2 jmp PC
    
    IOW, say thread A is stepping ADDR2's line in the background (an
    infinite loop), and the user switches focus to thread B.  GDB's
    adjust_pc_after_break logic confuses the single-step stop of thread A
    for a hit of the breakpoint at ADDR1, and thus adjusts thread A's PC
    to point at ADDR1 when it should not, and reports a breakpoint hit,
    when thread A did not execute the instruction at ADDR1 at all.
    
    The test added by this patch exercises exactly that.
    
    I can't find any reason we'd need the "thread to be examined is still
    the current thread" condition in adjust_pc_after_break, at least
    nowadays; it might have made sense in the past.  Best just remove it,
    and rely on currently_stepping().
    
    Here's the test's log of a run with an unpatched GDB:
    
     35        while (1);
     (gdb) PASS: gdb.threads/step-bg-decr-pc-switch-thread.exp: next over nop
     next&
     (gdb) PASS: gdb.threads/step-bg-decr-pc-switch-thread.exp: next& over inf loop
     thread 1
     [Switching to thread 1 (Thread 0x7ffff7fc2740 (LWP 29027))](running)
     (gdb)
     PASS: gdb.threads/step-bg-decr-pc-switch-thread.exp: switch to main thread
     Breakpoint 2, thread_function (arg=0x0) at ...src/gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.c:34
     34        NOP; /* set breakpoint here */
     FAIL: gdb.threads/step-bg-decr-pc-switch-thread.exp: no output while stepping
    
    gdb/ChangeLog:
    2015-02-11  Pedro Alves  <pedro@codesourcery.com>
    
    	* infrun.c (adjust_pc_after_break): Don't adjust the PC just
    	because the event thread is not the current thread.
    
    gdb/testsuite/ChangeLog:
    2015-02-11  Pedro Alves  <pedro@codesourcery.com>
    
    	* gdb.threads/step-bg-decr-pc-switch-thread.c: New file.
    	* gdb.threads/step-bg-decr-pc-switch-thread.exp: New file.

Diff:
---
 gdb/ChangeLog                                      |  5 ++
 gdb/infrun.c                                       |  2 -
 gdb/testsuite/ChangeLog                            |  5 ++
 .../gdb.threads/step-bg-decr-pc-switch-thread.c    | 54 +++++++++++++
 .../gdb.threads/step-bg-decr-pc-switch-thread.exp  | 91 ++++++++++++++++++++++
 5 files changed, 155 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 36889aa..ac89f43 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-11  Pedro Alves  <pedro@codesourcery.com>
+
+	* infrun.c (adjust_pc_after_break): Don't adjust the PC just
+	because the event thread is not the current thread.
+
 2015-02-11  Doug Evans  <xdje42@gmail.com>
 
 	* gdbtypes.c (internal_type_self_type): If TYPE_SPECIFIC_FIELD hasn't
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 5770d77..15589b6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3487,7 +3487,6 @@ adjust_pc_after_break (struct execution_control_state *ecs)
 
 	 The SIGTRAP can be due to a completed hardware single-step only if 
 	  - we didn't insert software single-step breakpoints
-	  - the thread to be examined is still the current thread
 	  - this thread is currently being stepped
 
 	 If any of these events did not occur, we must have stopped due
@@ -3499,7 +3498,6 @@ adjust_pc_after_break (struct execution_control_state *ecs)
 	 we also need to back up to the breakpoint address.  */
 
       if (thread_has_single_step_breakpoints_set (ecs->event_thread)
-	  || !ptid_equal (ecs->ptid, inferior_ptid)
 	  || !currently_stepping (ecs->event_thread)
 	  || (ecs->event_thread->stepped_breakpoint
 	      && ecs->event_thread->prev_pc == breakpoint_pc))
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index a8ddf04..49075ae 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2015-02-11  Pedro Alves  <pedro@codesourcery.com>
+
+	* gdb.threads/step-bg-decr-pc-switch-thread.c: New file.
+	* gdb.threads/step-bg-decr-pc-switch-thread.exp: New file.
+
 2015-02-10  Doug Evans  <xdje42@gmail.com>
 
 	* lib/gdb.exp (gdb_load): Always return a result.
diff --git a/gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.c b/gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.c
new file mode 100644
index 0000000..92641f6
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.c
@@ -0,0 +1,54 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 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/>.  */
+
+#include <unistd.h>
+#include <pthread.h>
+#include <assert.h>
+
+/* NOP instruction: must have the same size as the breakpoint
+   instruction for the test to be effective.  */
+
+#if defined (__s390__) || defined (__s390x__)
+# define NOP asm ("nopr 0")
+#else
+# define NOP asm ("nop")
+#endif
+
+void *
+thread_function (void *arg)
+{
+  NOP; /* set breakpoint here */
+  while (1);
+}
+
+int
+main (void)
+{
+  int res;
+  pthread_t thread;
+
+  alarm (300);
+
+  res = pthread_create (&thread,
+		       NULL,
+		       thread_function,
+		       NULL);
+  assert (res == 0);
+
+  pthread_join (thread, NULL);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.exp b/gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.exp
new file mode 100644
index 0000000..b19b1f7
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.exp
@@ -0,0 +1,91 @@
+# Copyright (C) 2014-2015 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/>.
+
+# On decr_pc_after_break targets, GDB used to adjust the PC
+# incorrectly if a background single-step stopped somewhere where
+# PC-$decr_pc had a breakpoint, and the thread was not the current
+# thread, like:
+#
+#   ADDR1 nop <-- breakpoint here
+#   ADDR2 jmp PC
+#
+#  IOW, say thread A is stepping ADDR2's line in the background (an
+#  infinite loop), and the user switches focus to thread B.  GDB's
+#  adjust_pc_after_break logic would confuse the single-step stop of
+#  thread A for a hit of the breakpoint at ADDR1, and thus adjust
+#  thread A's PC to point at ADDR1 when it should not: the thread had
+#  been single-stepped, not continued.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] == -1} {
+    return -1
+}
+
+clean_restart $binfile
+
+if ![runto_main] {
+    continue
+}
+
+# Make sure it's GDB's decr_pc logic that's being tested, not the
+# target's.
+gdb_test_no_output "set range-stepping off"
+
+delete_breakpoints
+
+gdb_breakpoint [gdb_get_line_number "set breakpoint here"]
+gdb_continue_to_breakpoint "run to nop breakpoint"
+gdb_test "info threads" "\\\* 2 .* 1.*" "info threads shows all threads"
+
+gdb_test "next" "while.*" "next over nop"
+
+gdb_test_no_output "next&" "next& over inf loop"
+
+set test "switch to main thread"
+gdb_test_multiple "thread 1" $test {
+    -re "Cannot execute this command while the target is running.*$gdb_prompt $" {
+	unsupported $test
+
+	# With remote targets, we can't send any other remote packet
+	# until the target stops.  Switching thread wants to ask the
+	# remote side whether the thread is alive.
+	return
+    }
+    -re "Switching to thread 1.*\\(running\\)\r\n$gdb_prompt " {
+	# Prefer to match the prompt without an anchor.  If there's a
+	# bug and output comes after the prompt immediately, it's
+	# faster to handle that in the following test, instead of
+	# waiting for a timeout here.
+	pass $test
+    }
+}
+
+# Wait a bit.  Use gdb_expect instead of sleep so that any (bad) GDB
+# output is visible in the log.
+gdb_expect 4 {}
+
+set test "no output while stepping"
+gdb_test_multiple "" $test {
+    -timeout 1
+    timeout {
+	pass $test
+    }
+    -re "." {
+	# If we see any output, it's a failure.  On the original bug,
+	# this would be a breakpoint hit.
+	fail $test
+    }
+}