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

[Muhammad Waqas <mwaqas@codesourcery.com>]

On 08/01/2013 04:57 PM, Pedro Alves wrote:
> On 07/30/2013 11:33 AM, Muhammad Waqas wrote:
>> On 07/29/2013 07:17 PM, Yao Qi wrote:
>>> On 07/29/2013 07:42 PM, Muhammad Waqas wrote:
>>>
>>> Please have a look at
>>> http://sourceware.org/gdb/wiki/ContributionChecklist , it is very
>>> helpful for you to avoid some issues in your patch.
>>>
>>>> 2013-07-24   Muhammad Waqas<mwaqas@codesourcery.com>
>>>              ^^^ Two spaces.
>>>
>>>>        PR gdb/11568
>>>>        * breakpoint.c (breakpoint_auto_delete): add new condition
>>>>        Remove thread related breakpoints if threads are exited.
>>> ^^^^^^^ Tab instead of spaces.
>>>
>>> The changelog entry is like this:
>>>
>>>      PR gdb/11568
>>>      * breakpoint.c (breakpoint_auto_delete): Remove thread related
>>>      breakpoints if threads are exited.
>>>
>>>>
>>>>
>>>> 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    29 Jul 2013 11:22:05 -0000
>>>> @@ -11910,6 +11910,15 @@ 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*/
>>> Please move the comments into the brackets below.  Period is missing
>>> in your comment, and we need two spaces after period at the end of
>>> comment.
>>>
>>>> +      {
>>>> +    struct thread_info * tp = find_thread_id (b->thread) ;
>>> A blank line is needed here.  Indentation looks wrong here.
>>>
>>>> +    if (tp != NULL && (tp->state == THREAD_EXITED
>>>> +               || !target_thread_alive (tp->ptid)))
>>>> +      delete_breakpoint (b);
>>> and here.
>>>
>>>> +      }
>>>>       }
>>>>     }
>>>>
>>> I run thread-specific-bp.exp in async/non-stop mode, and I get a fail.
>>> I am afraid your patch only works in all-stop mode.
>>>
>>>> testsuite/Changlog
>>>>
>>>> 2013-07-29  Muhammad Waqas<mwaqas@codesourcery.com>
>>>>            Jan Kratochvil<jan.kratochvil@redhat.com>
>>>   ^^^^^^^^^^
>>> Need a tab instead of spaces.  Please reference existing entries in
>>> ChangeLog to see how to list two authors.
>>>
>>>>        * gdb.threads/thread-specific-bp.c: New file.
>>>>        * gdb.threads/thread-specific-bp.exp: New file.
>>> Please include these new added files into your patch, so that people
>>> can review them.  Here is an example
>>> (http://sourceware.org/ml/gdb-patches/2013-07/msg00671.html) I include
>>> new added file in the patch.  I use git, but you can get the similar
>>> patch in cvs.
>>>
>>>> # Copyright (C) 1996-2013 Free Software Foundation, Inc.
>>> It should be 2013 only.
>>>
>>>>          gdb_test_multiple "info breakpoint" "get info" {
>>>>          -re "(\[0-9\]+)(\[^\n\r\]*breakpoint\[^\n\r\]*main.*thread
>>>> $thre)" {
>>> I don't really understand this pattern.  Probably, we only want to
>>> match "thread $thre"
>>>
>>> ".*$gdb_prompt $" is missing at the end of the pattern.  Please add it.
>>>
>>>>              fail "threaded breakpoint not deleted"
>>>>          }
>>>>          -re "(\[0-9\]+)(\[^\n\r\]*breakpoint\[^\n\r\]*main.*\n)" {
>>>>                  pass "threaded breakpoint deleted"
>>> fail/pass should be the same.
>>>
>>>>          }
>>>>      }
>>> Something like this:
>>>
>>>      set test "thread-specific breakpoint is deleted"
>>>      gdb_test_multiple "info breakpoint" $test {
>>>      -re "thread $thre.*$gdb_prompt $" {
>>>          fail $test
>>>      }
>>>      -re "$gdb_prompt $" {
>>>          pass $test
>>>      }
>>>      }
>>>
>>> There are some trailing spaces in the test.  Please remove them.
>>>
>> Index: ChangeLog
>> ===================================================================
>> RCS file: /cvs/src/src/ChangeLog,v
>> retrieving revision 1.1075
>> diff -u -p -r1.1075 ChangeLog
>> --- ChangeLog	22 Jul 2013 15:17:20 -0000	1.1075
>> +++ ChangeLog	30 Jul 2013 10:20:09 -0000
>> @@ -1,3 +1,9 @@
>> +2013-07-24  Muhammad Waqas  <mwaqas@codesourcery.com>
>> +
>> +	PR gdb/11568
>> +	* breakpoint.c (breakpoint_auto_delete): Remove thread related
>> +	breakpoints if threads are exited.
>> +
>>    2013-07-22  Joel Brobecker  <brobecker@adacore.com>
>>    
>>    	* src-release (VER): Use $(TOOL)/common/create-version.sh
>> Index: gdb/breakpoint.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/breakpoint.c,v
>> retrieving revision 1.773
>> diff -u -p -r1.773 breakpoint.c
>> --- gdb/breakpoint.c	24 Jul 2013 19:50:32 -0000	1.773
>> +++ gdb/breakpoint.c	30 Jul 2013 10:20:49 -0000
>> @@ -11941,6 +11941,17 @@ 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.  */
> "Check" is capitalized in the middle of the sentence.  But instead
> you'd just say:
>
> 	/* Delete thread specific breakpoints whose thread is done.  */
>
>> +	struct thread_info *tp = find_thread_id (b->thread);
>> +
>> +	if ( tp != NULL && (tp->state == THREAD_EXITED
>              ^
> spurious space.
>
>> +			    || !target_thread_alive (tp->ptid)))
>> +	if ( tp != NULL && (tp->state == THREAD_EXITED
>> +			    || !target_thread_alive (tp->ptid)))
> Do we really need this target_thread_alive call?  It
> means we have extra remote traffic whenever we have a thread
> specific breakpoint in the list.  If the user (or GDB itself)
> hasn't refreshed the thread list, is there any harm in delaying
> deleting the breakpoint?
>
> But, I think I agree with Yao in that this doesn't look like
> the right way to do this.
>
> In fact, breakpoint_auto_delete is only called when the
> target reports some stop.  If you're trying to delete
> stale thread specific breakpoints, then you'd want to
> do that even if the target hasn't reported a stop, right?
>
> Say, in non-stop mode, w/ the remote target, if you have two
> threads, set a thread-specific break on thread 2, and while
> all threads are running free in busy loops, not reporting
> any event, keep doing "info threads", and "info break".  At some
> point thread #2 exits.  You can see that from "info threads".
> But "info break" will still show you the stale breakpoint...

If breakpoint will be deleted when thread list is updated through
user or GDB and also when info break command is executed
by user, Will it work? What will you say about this technique?

>> +	  delete_breakpoint(b);
> Missing space before parens.
>
> This deletes the breakpoint silently, which may be surprising.
> We're not silent when we delete watchpoints on local scopes.
>
>> +
>> +      }
>>      }
>>    }
>>
>> Problem was with my test case, now It works with async/non-stop mode.
> What would be interesting is whether it works with both native
> and remote targets, as with remote targets we only know whether
> threads are gone when we refresh the thread list, while with
> native GNU/Linux, presently, the core knows when a thread is
> gone as soon as it exits.  As I said in the other email, if
> this is supposed to run in non-stop mode too, then let's make
> the test test both all-stop and non-stop modes explicitly.
>
>> +# This file was written by Muhammad Waqas <mwaqas@codesourcery.com>
> Can I kindly ask you to consider removing this?  From the ChangeLog,
> it's already wrong from the onset.
>
>> +#This test verfiy that Breakpoint on a spacific thread is deleted
> What the typos, missing periods, etc.  I'd write:
>
> # Verify that a thread-specific breakpoint is deleted when the
> # corresponding thread is gone.
>
>> +
>> +standard_testfile
>> +
>> +if {[gdb_compile_pthreads \
>> +	 "${srcdir}/${subdir}/${srcfile}" \
>> +	 "${binfile}" executable {debug} ] != "" } {
>> +    return -1
>> +}
>> +
>> +clean_restart ${binfile}
>> +if [runto main] then {
> It's more idiomatic to do
>
>   if [runto main] then {
>     return
>   }
>
>   <rest of code here>
>
> and avoid the indentation.
>
>> +
>> +    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)
> If the previous test fails, "thre" will hold something unrelated.
> Initialize "thre" outside the gdb_test_multiple, and extract the
> id close to the "pass".
>
>> +    gdb_breakpoint [gdb_get_line_number "line # 13"]
>> +    gdb_breakpoint "main thread $thre"
>> +    gdb_test "info break" ".*breakpoint.*(thread $thre)" "Breakpoint set"
> Why the parens?
>
>> +    gdb_test "thread $thre" "Switching to thread $thre.*" "Thread $thre selected"
>> +    gdb_continue_to_breakpoint "line # 13"
> Why "line # 13" ?  Does "13" have any significance?  Let's
> avoid such confusions in the minds of whoever reads the code.
> It's more usual to write "set break here" or some such in the
> code instead, and look for that.
>
>> Index: gdb/testsuite/gdb.threads/thread-specific-bp.c
>> ===================================================================
>> RCS file: gdb/testsuite/gdb.threads/thread-specific-bp.c
>> diff -N gdb/testsuite/gdb.threads/thread-specific-bp.c
>> --- /dev/null	1 Jan 1970 00:00:00 -0000
>> +++ gdb/testsuite/gdb.threads/thread-specific-bp.c	30 Jul 2013 10:21:38 -0000
>> +#include <stdio.h>
> This include looks unnecessary.
>