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

[Pedro Alves <palves at redhat dot com>]

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