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

[Muhammad Waqas <mwaqas at codesourcery dot com>]

On 08/28/2013 12:02 AM, Pedro Alves wrote:
> 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.
> 
> 
Fixed.
>> 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.  */
> 
Fixed.
>> +
>> +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?
> 
Fixed.
>> +       }
>> +    }
>> +}
>> +
> 
> 
> 
>> +
>> +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).
> 
Fixed.
>> +
>> +    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.
> 
Fixed.
>> +	    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?
> 
Replaced.
>> +	-re "(?:Breakpoint|Temporary breakpoint) .* (at|in) .*\r\n$gdb_prompt $" {
> 
> Why is this matching temporary breakpoints output?
> 
Removed.
>> +	    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.
>> +	    exp_continue
>> +	}
>> +    }
>> +
> 
> Thanks,
>