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

Muhammad Waqas
2013-08-28T12:26:36Z

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,
>