This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]