From 0703599a49d082a957ee233fe018fb6ea7864920 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 11 Feb 2015 09:45:41 +0000 Subject: [PATCH] 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 * 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 * gdb.threads/step-bg-decr-pc-switch-thread.c: New file. * gdb.threads/step-bg-decr-pc-switch-thread.exp: New file. --- gdb/ChangeLog | 5 + gdb/infrun.c | 2 - gdb/testsuite/ChangeLog | 5 + .../step-bg-decr-pc-switch-thread.c | 54 +++++++++++ .../step-bg-decr-pc-switch-thread.exp | 91 +++++++++++++++++++ 5 files changed, 155 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.c create mode 100644 gdb/testsuite/gdb.threads/step-bg-decr-pc-switch-thread.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 36889aa6e6..ac89f43345 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2015-02-11 Pedro Alves + + * 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 * gdbtypes.c (internal_type_self_type): If TYPE_SPECIFIC_FIELD hasn't diff --git a/gdb/infrun.c b/gdb/infrun.c index 5770d773e0..15589b6a76 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 a8ddf046e0..49075aeeac 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2015-02-11 Pedro Alves + + * 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 * 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 0000000000..92641f6d24 --- /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 . */ + +#include +#include +#include + +/* 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 0000000000..b19b1f7e96 --- /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 . + +# 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 + } +}