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

[Yao Qi <yao at codesourcery dot com>]

On 07/29/2013 03:29 PM, Muhammad Waqas wrote:
> 2013-07-24   Muhammad Waqas<waqas.jamil47@gmail.com>

Wrong e-mail address?
Should mention PR number in ChangeLog.

>       * breakpoint.c (breakpoint_auto_delete): Remove breakpoint
>       Remove thread related breakpoints if threads are exited.

"Remove breakpoint" can be removed, IMO.


> cvs diff -up gdb/breakpoint.c
>
>
> Index: gdb/breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.772
> diff -u -p -r1.772 breakpoint.c
> --- gdb/breakpoint.c    6 Jul 2013 07:14:54 -0000    1.772
> +++ gdb/breakpoint.c    24 Jul 2013 05:18:24 -0000
> @@ -11910,6 +11910,12 @@ breakpoint_auto_delete (bpstat bs)
>      {
>        if (b->disposition == disp_del_at_next_stop)
>          delete_breakpoint (b);
> +    else if (b->thread > 0)    /* If breakpoint relates to user created
> thread Check if it's not alive then delete it*/

This line is too long, and your mailer wrap the patch.

> +      {
> +    struct thread_info * tp = find_thread_id (b->thread) ;
> +    if (tp != NULL && (tp->state == THREAD_EXITED ||
> !target_thread_alive (tp->ptid)))

Likewise.

> +      delete_breakpoint (b);
> +      }
>      }
>    }

I am wondering why don't we register to thread_exit observer, and remove 
breakpoints when thread exits.  arm-linux-nat.c and ppc-linux-nat.c did 
something similar to delete thread hw breakpoint.  However, I am not 
sure it is safe to do so considering async/non-stop mode.

> gdb/testcase/Changlog
>
> 2013-07-29  Muhammad Waqas<mwaqas@codesourcery.com>
>
>       * gdb.threads/thread-specific-bp.c: New file

It is copied from Jan's example in bugzilla, better to list his name 
here too.

>       * gdb.threads/thread-specific-bp.exp: New file.



> gdb.threads/thread-specific-bp.c

Please include these new added files into the patch.  So that reviewers 
can apply your patch to get these new files.

We need a copyright header for test files too.


> gdb.threads/thread-specific-bp.exp
>
> #This test verfiy that Breakpoint on a spacific thread is deleted
>
> standard_testfile
> if {[gdb_compile_pthreads \
>        "${srcdir}/${subdir}/${srcfile}" \
>        "${binfile}" executable {debug} ] != "" } {
>       return -1
> }
> clean_restart ${binfile}
>
> if [runto main] then {
>       gdb_breakpoint "start"
>       gdb_continue_to_breakpoint "start"
>       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
>       set thre $expect_out(1,string)
>       gdb_breakpoint [gdb_get_line_number "line # 13"]
>       gdb_breakpoint "main thread $thre"
>       gdb_test "info break" ".*breakpoint.*(thread $thre)"
>       gdb_continue_to_breakpoint "line # 13"
>       gdb_test "info break" ".*breakpoint.*(thread \[^$thre\])*"
> }

This test passes even without your patch to breakpoint_auto_delete 
above.  Please make sure the test fails without your patch, and fails go 
away when your patch is applied.


> Running target unix
> Running ./gdb.threads/thread-specific-bp.exp ...
> PASS: gdb.threads/thread-specific-bp.exp: successfully compiled posix
> threads test case
> PASS: gdb.threads/thread-specific-bp.exp: continue to breakpoint: start
> PASS: gdb.threads/thread-specific-bp.exp: thread created
> PASS: gdb.threads/thread-specific-bp.exp: info break
                                            ^^^^^^^^^^
> PASS: gdb.threads/thread-specific-bp.exp: continue to breakpoint: line # 13
> PASS: gdb.threads/thread-specific-bp.exp: info break
                                            ^^^^^^^^^^^

Duplicated test results.  Please make them unique.  Like,

PASS: gdb.threads/thread-specific-bp.exp: info break 1
...
PASS: gdb.threads/thread-specific-bp.exp: info break 2

--
Yao (éå)