Re: [PATCH with testcase] Bug 11568 - delete thread-specific breakpoint on the thread exit

Pedro Alves
2013-09-02T16:46:44Z

On 08/28/2013 01:26 PM, Muhammad Waqas wrote:
>>> >> + pass $full_name
>>> >> + }
>>> >> + -re ".*$gdb_prompt $" {
>>> >> + fail $full_name
>>> >> + }
>>> >> + timeout {
>>> >> + send_gdb "thread 1\n"
>> >
>> > Hmm, I don't really understand this. Can you explain, please?
>> >
> Some time in non-stop when running thread (which is "thre" in this case)
> exited GDB prompts for commands, instead of selecting thread 1 and
> continue, so in that case time-out happened and I select thread 1 and
> continue.

This sounds very much like a workaround. In any case, expecting a
timeout for regular test flow is just not right. We should instead
make sure to select thread 1 _before_ the "continue", so we don't
end up with that situation.

As discussed before, I got on native GNU/Linux:

FAIL: gdb.threads/thread-specific-bp.exp: All stop: continue to breakpoint: set break here

I've worked around the problem by setting a breakpoint
at a new function "end", rather than relying on "b 32"
to do the right thing.

> +
> +static void
> +remove_threaded_breakpoints (struct thread_info *tp, int silent)
> +{
> + struct breakpoint *b, *b_tmp;
> +
> + ALL_BREAKPOINTS_SAFE (b, b_tmp)
> + {
> + if (b->thread == tp->num)
> + {
> + int tmp = b->number;

There's really no need for this temporary.

> + b->disposition = disp_del_at_next_stop;
> + /* Hide it from the user. */
> + b->number = 0;
> +
> + printf_filtered (_("Breakpoint %d deleted\
> +because thread %d has been exited\non which this \
> +breakpoint is valid.\n"),

Awww, now that's broken...

Breakpoint 3 deletedbecause thread 2 has been exited
^^^^^^^^^^^^^^
on which this breakpoint is valid.
^^

In any case, I suggest instead:

"Thread-specific breakpoint 3 deleted - thread 2 is gone.\n"

"thread-specific" is how the manual refers to these
breakpoints - there's a section in the manual called that way.

"gone" instead of "exited" because we will do this even if we
e.g., detach from the process, or other similar situations when
the thread hasn't really exited.

It'd be nice if the "Thread-Specific Breakpoints" node in the
manual would be updated to mention what we now do when the thread
is gone.

> + tmp,tp->num);
> + }
> + }

The patch added some spurious trailing whitespace:

$ git am /tmp/mbox
Applying: Bug 11568 - delete thread-specific breakpoint on the thread exit
/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:58: trailing whitespace.

/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:60: trailing whitespace.

/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:65: trailing whitespace.

/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:70: trailing whitespace.

/home/pedro/gdb/mygit/src/.git/rebase-apply/patch:157: space before tab in indent.
send_gdb "thread 1\n"
warning: 5 lines add whitespace errors.

> diff --git a/gdb/testsuite/gdb.threads/thread-specific-bp.exp b/gdb/testsuite/gdb.threads/thread-specific-bp.exp
> new file mode 100644
> index 0000000..013bfcd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/thread-specific-bp.exp
> @@ -0,0 +1,98 @@
> +# Copyright (C) 2013 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/>.
> +#
> +# Verify that a thread-specific breakpoint is deleted when the
> +# corresponding thread is gone.
> +
> +standard_testfile
> +
> +if {[gdb_compile_pthreads \
> + "${srcdir}/${subdir}/${srcfile}" \
> + "${binfile}" executable {debug} ] != "" } {
> + return -1
> +}
> +
> +clean_restart ${binfile}
> +
> +proc check_threaded_breakpoint {mode} {
> + with_test_prefix "$mode" {
> + global gdb_prompt

It's good to leave an empty line after declarations.

> + gdb_breakpoint "start"
> + gdb_continue_to_breakpoint "start"
> + set thre 0
> +
> + gdb_test_multiple "info threads" "get thread 1 id" {
> + -re "(\[0-9\]+)(\[^\n\r\]*Thread\[^\n\r\]*start.*)($gdb_prompt $)" {

There's no need for all these parens, actually.

> + pass "thread created"
> + # Get the thread's id.
> + set thre $expect_out(1,string)
> + }
> + }
> + gdb_breakpoint "main thread $thre"
> + gdb_test "info break" ".*breakpoint.*thread $thre" "Breakpoint set"
> + gdb_breakpoint [gdb_get_line_number "set break here"]

This is the breakpoint I needed changing to work around PR gdb/15911.

> +
> + # Force GDB to update its knowledge on existing threads when this
> + # breakpoint is hit. Otherwise, GDB doesn't realize thread $thre
> + # has exited and doesn't remove the thread specific breakpoint.
> + gdb_test "commands\ninfo threads\nend" "End with.*" "add breakpoint commands"

I don't see much point in doing this in the breakpoint's command itself,
rather than simply after hitting the breakpoint. Seems more standard
that way.

> + gdb_test "thread $thre" "Switching to thread $thre.*" "Thread $thre selected"
> + set full_name "continue to breakpoint: set break here"
> +
> + gdb_test_multiple "continue" "$full_name" {
> + -re "(?:Breakpoint) .* (at) .*\r\n$gdb_prompt $" {
> + pass $full_name
> + }
> + -re ".*$gdb_prompt $" {
> + fail $full_name
> + }
> + timeout {
> + send_gdb "thread 1\n"
> + exp_continue
> + }
> + }

This I've done differently. The test is now switching to
thread 1, and issuing "continue -a" in non-stop mode, so to
make sure to resume thread 2 as well.

> +
> + set test "thread-specific breakpoint is deleted"
> + gdb_test_multiple "info breakpoint" $test {
> + -re "thread $thre\n$gdb_prompt $" {
> + fail $test
> + }
> + -re "$gdb_prompt $" {
> + pass $test
> + }
> + }
> + }
> +}
> +
> +if ![runto_main] {
> + untested "could not run to main"
> + return -1
> +}

This can be moved to the test routine... (1)

> +
> +# Testing in all stop mode.
> +check_threaded_breakpoint "All stop"

Mixup of uppercase and lowercase gdb.sum output. Let's stick
with lowercase.

Also note "All stop" vs "non-stop" (hyphen), vs "non stop" below.

> +
> +clean_restart ${binfile}
> +
> +gdb_test "set target-async on" ".*" "Set async mode"
> +gdb_test "set non-stop on" ".*" "Set non stop mode"
> +
> +if ![runto_main] {
> + untested "could not run to main"
> + return -1
> +}

(1) ... as both modes do it. Doing that also gets rid
of duplicated output in gdb.sum.

> +
> +# Testing in non-stop with async mode.
> +check_threaded_breakpoint "non-stop with async"

non-stop is always async.

Here's the diff I got on top of yours. I'll send out the
final diff in a bit.

---

gdb/breakpoint.c | 17 +--
gdb/testsuite/gdb.threads/thread-specific-bp.c | 16 ++-
gdb/testsuite/gdb.threads/thread-specific-bp.exp | 123 ++++++++++++++--------
3 files changed, 95 insertions(+), 61 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 724d847..734dfd6 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2940,15 +2940,14 @@ remove_threaded_breakpoints (struct thread_info *tp, int silent)
{
if (b->thread == tp->num)
{
- int tmp = b->number;
- b->disposition = disp_del_at_next_stop;
- /* Hide it from the user. */
- b->number = 0;
-
- printf_filtered (_("Breakpoint %d deleted\
-because thread %d has been exited\non which this \
-breakpoint is valid.\n"),
- tmp,tp->num);
+ b->disposition = disp_del_at_next_stop;
+
+ printf_filtered (_("\
+Thread-specific breakpoint %d deleted - thread %d is gone.\n"),
+ b->number, tp->num);
+
+ /* Hide it from the user. */
+ b->number = 0;
}
}
}
diff --git a/gdb/testsuite/gdb.threads/thread-specific-bp.c b/gdb/testsuite/gdb.threads/thread-specific-bp.c
index 474d667..5ca02c7 100644
--- a/gdb/testsuite/gdb.threads/thread-specific-bp.c
+++ b/gdb/testsuite/gdb.threads/thread-specific-bp.c
@@ -1,17 +1,17 @@
/* This testcase is part of GDB, the GNU debugger.
-
+
Copyright 2013 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/>. */

@@ -23,11 +23,17 @@ start (void *arg)
return NULL;
}

+static void
+end (void)
+{
+}
+
int
main (void)
{
pthread_t thread;
pthread_create (&thread, NULL, start, NULL);
pthread_join (thread, NULL);
- return 0; /*set break here*/
+ end ();
+ return 0;
}
diff --git a/gdb/testsuite/gdb.threads/thread-specific-bp.exp b/gdb/testsuite/gdb.threads/thread-specific-bp.exp
index 013bfcd..9f7d142 100644
--- a/gdb/testsuite/gdb.threads/thread-specific-bp.exp
+++ b/gdb/testsuite/gdb.threads/thread-specific-bp.exp
@@ -26,47 +26,87 @@ if {[gdb_compile_pthreads \

clean_restart ${binfile}

-proc check_threaded_breakpoint {mode} {
+# Extract and return the thread ID of the thread stopped at function
+# FUNC.
+
+proc get_thread_id {func} {
+ global gdb_prompt
+
+ set thre -1
+ set test "get $func thread id"
+ gdb_test_multiple "info threads" $test {
+ -re "(\[0-9\]+)\[^\n\r\]*Thread\[^\n\r\]*$func.*$gdb_prompt $" {
+ # Get the thread's id.
+ set thre $expect_out(1,string)
+ pass $test
+ }
+ }
+
+ return $thre
+}
+
+proc check_thread_specific_breakpoint {mode} {
with_test_prefix "$mode" {
global gdb_prompt
+
+ if ![runto_main] {
+ untested "could not run to main"
+ return -1
+ }
+
+ set main_thre [get_thread_id "main"]
+ if { $main_thre < 0 } {
+ return -1
+ }
+
gdb_breakpoint "start"
gdb_continue_to_breakpoint "start"
- set thre 0

- gdb_test_multiple "info threads" "get thread 1 id" {
- -re "(\[0-9\]+)(\[^\n\r\]*Thread\[^\n\r\]*start.*)($gdb_prompt $)" {
- pass "thread created"
- # Get the thread's id.
- set thre $expect_out(1,string)
- }
+ set start_thre [get_thread_id "start"]
+ if { $start_thre < 0 } {
+ return -1
}
- gdb_breakpoint "main thread $thre"
- gdb_test "info break" ".*breakpoint.*thread $thre" "Breakpoint set"
- gdb_breakpoint [gdb_get_line_number "set break here"]
-
- # Force GDB to update its knowledge on existing threads when this
- # breakpoint is hit. Otherwise, GDB doesn't realize thread $thre
- # has exited and doesn't remove the thread specific breakpoint.
- gdb_test "commands\ninfo threads\nend" "End with.*" "add breakpoint commands"
- gdb_test "thread $thre" "Switching to thread $thre.*" "Thread $thre selected"
- set full_name "continue to breakpoint: set break here"
-
- gdb_test_multiple "continue" "$full_name" {
- -re "(?:Breakpoint) .* (at) .*\r\n$gdb_prompt $" {
- pass $full_name
- }
- -re ".*$gdb_prompt $" {
- fail $full_name
+
+ # Set a thread-specific breakpoint at "main". This can't ever
+ # be hit, but that's OK.
+ gdb_breakpoint "main thread $start_thre"
+ gdb_test "info break" ".*breakpoint.*thread $start_thre" "breakpoint set"
+
+ # Set breakpoint at a place only reacheable after the "start"
+ # thread exits.
+ gdb_breakpoint "end"
+
+ # Switch back to the main thread, and resume all threads. The
+ # "start" thread exits, and the main thread reaches "end".
+ gdb_test "thread $main_thre" \
+ "Switching to thread $main_thre.*" \
+ "thread $main_thre selected"
+
+ if { $mode == "non-stop" } {
+ set cmd "continue -a"
+ } else {
+ set cmd "continue"
+ }
+ gdb_test "$cmd" \
+ "Breakpoint .* end .* at .*" \
+ "continue to end"
+
+ # Force GDB to update the thread list. Otherwise, depending
+ # on target, GDB may not realize that the start thread has
+ # exited and thus not remove the thread specific breakpoint.
+ set test "thread start is gone"
+ gdb_test_multiple "info threads" $test {
+ -re "\[0-9\]+.*start.*$gdb_prompt $" {
+ fail $test
}
- timeout {
- send_gdb "thread 1\n"
- exp_continue
+ -re "$gdb_prompt $" {
+ pass $test
}
}

- set test "thread-specific breakpoint is deleted"
+ set test "thread-specific breakpoint was deleted"
gdb_test_multiple "info breakpoint" $test {
- -re "thread $thre\n$gdb_prompt $" {
+ -re "thread $start_thre\n$gdb_prompt $" {
fail $test
}
-re "$gdb_prompt $" {
@@ -76,23 +116,12 @@ proc check_threaded_breakpoint {mode} {
}
}

-if ![runto_main] {
- untested "could not run to main"
- return -1
-}
-
-# Testing in all stop mode.
-check_threaded_breakpoint "All stop"
+# Test all-stop mode.
+check_thread_specific_breakpoint "all-stop"

clean_restart ${binfile}

-gdb_test "set target-async on" ".*" "Set async mode"
-gdb_test "set non-stop on" ".*" "Set non stop mode"
-
-if ![runto_main] {
- untested "could not run to main"
- return -1
-}
-
-# Testing in non-stop with async mode.
-check_threaded_breakpoint "non-stop with async"
+# Test non-stop mode.
+gdb_test_no_output "set target-async on" "set async mode"
+gdb_test_no_output "set non-stop on" "set non-stop mode"
+check_thread_specific_breakpoint "non-stop"