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

Pedro Alves
2013-08-27T19:02:47Z

On 08/27/2013 12:29 PM, Muhammad Waqas wrote:
> 2013-08-05 Muhammad Waqas <mwaqas@codesourcery.com>
>
> PR gdb/11568
> * breakpoint.c (remove_threaded_breakpoints): New function.
> * breakpoint.c (_initialize_breakpoint): function
> remove_threaded_breakpoints registers with thread_exit.

Indentation isn't right.
See <https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog>.

"Function" should be uppercase.

* breakpoint.c (_initialize_breakpoint): Function
remove_threaded_breakpoints registers with thread_exit.

> 2013-07-24 Muhammad Waqas <mwaqas@codesourccery.com>
> Jan Kratochvil <jan.kartochvil@redhat.com>
>
> PR gdb/11568
> * gdb.thread/thread-specific-bp.c: New file.
> * gdb.thread/thread-specific-bp.exp: New file.
> ---
> gdb/breakpoint.c | 19 +++++
> gdb/testsuite/gdb.threads/thread-specific-bp.c | 33 ++++++++
> gdb/testsuite/gdb.threads/thread-specific-bp.exp | 98 ++++++++++++++++++++++
> 3 files changed, 150 insertions(+)
> create mode 100644 gdb/testsuite/gdb.threads/thread-specific-bp.c
> create mode 100644 gdb/testsuite/gdb.threads/thread-specific-bp.exp
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 44bb7a8..588a9b0 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -2928,6 +2928,24 @@ remove_breakpoints (void)
> return val;
> }
>
> +/* Used when a thread exits, it will remove breakpoints which
> + are related to that thread. */

/* When a thread exits, remove breakpoints that are related to
that thread. */

> +
> +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)
> + {
> + b->disposition = disp_del_at_next_stop;
> + b->number = 0;

Add a comment:

/* Hide it from the user. */
b->number = 0;

In an earlier review I remarked that this is deleting a breakpoint
silently, while on other similar occasions (local watchpoint, for e.g.)
we're verbose about it. What do people think we should do here?

> + }
> + }
> +}
> +

> +
> +if {[gdb_compile_pthreads \
> + "${srcdir}/${subdir}/${srcfile}" \
> + "${binfile}" executable {debug} ] != "" } {
> + return -1
> +}
> +
> +clean_restart ${binfile}
> +
> +proc check_threaded_breakpoint {mode} { with_test_prefix "$mode" {

Please move the with_test_prefix to its own line, and re-indent the
rest to follow. (Existing cases that look like that appeared due to
a wholesale conversion to with_test_prefix; done that way to avoid
touching much else).

> +
> + global gdb_prompt
> + 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 id of thread

Write full sentences please. Uppercase, period. Suggest:

# 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"]
> +
> + # 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"
> +
> + send_gdb "continue\n"
> + gdb_expect 10 {

Why not gdb_test_multiple?

> + -re "(?:Breakpoint|Temporary breakpoint) .* (at|in) .*\r\n$gdb_prompt $" {

Why is this matching temporary breakpoints output?

> + 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?

> + exp_continue
> + }
> + }
> +

Thanks,
--
Pedro Alves