[RFA/commit] Minor cleanup in breakpoint_thread_match

[RFA/commit] Minor cleanup in breakpoint_thread_match

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

This is in preparation for adding handling of (Ada) task-specific
breakpoints in this routine.  I could have just modified the condition
inside the loop, but it was getting too nested for my taste, making
the condition hard to read.  So here's a cleanup that would provide
two improvements: readability, and computing of the ptid's thread-id
in a lazy way.  Most of the time, at least for myself, I don't use
thread-specific breakpoints, so might as well not compute the thread ID.

Reference: http://www.sourceware.org/ml/gdb-patches/2009-03/msg00551.html

2009-03-25  Joel Brobecker  <brobecker@adacore.com>

        * breakpoint.c (breakpoint_thread_match): Split a large condition
        into several smaller conditions.  No behavior change.

Tested on x86_64-linux. I made this change in a pretty procedural and
mechanical way.  But a second pair of eyes would be appreciated. Plus,
I'd like to know if others also prefer splitting this condition like
I did.

Thanks,
-- 
Joel

[-- Attachment #2: bp-cleanup.diff --]
[-- Type: text/x-diff, Size: 1273 bytes --]

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7e50342..6b88601 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1893,20 +1893,28 @@ int
 breakpoint_thread_match (CORE_ADDR pc, ptid_t ptid)
 {
   const struct bp_location *bpt;
-  int thread;
-
-  thread = pid_to_thread_id (ptid);
-
+  /* The thread ID associated to PTID, computed lazily.  */
+  int thread = -1;
+  
   ALL_BP_LOCATIONS (bpt)
     {
       if (bpt->loc_type != bp_loc_software_breakpoint
 	  && bpt->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      if ((breakpoint_enabled (bpt->owner)
-	   || bpt->owner->enable_state == bp_permanent)
-	  && bpt->address == pc
-	  && (bpt->owner->thread == -1 || bpt->owner->thread == thread))
+      if (!breakpoint_enabled (bpt->owner)
+          && bpt->owner->enable_state != bp_permanent)
+        continue;
+
+      if (bpt->address != pc)
+        continue;
+
+      /* If this is a thread-specific breakpoint, and we haven't
+         computed the ptid's thread ID yet, compute it now.  */
+      if (bpt->owner->thread != -1 && thread == -1)
+        thread = pid_to_thread_id (ptid);
+
+      if (bpt->owner->thread == -1 || bpt->owner->thread == thread)
 	{
 	  if (overlay_debugging 
 	      && section_is_overlay (bpt->section) 

Re: [RFA/commit] Minor cleanup in breakpoint_thread_match

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

> 2009-03-25  Joel Brobecker  <brobecker@adacore.com>
> 
>         * breakpoint.c (breakpoint_thread_match): Split a large condition
>         into several smaller conditions.  No behavior change.
> 
> Tested on x86_64-linux. I made this change in a pretty procedural and
> mechanical way.  But a second pair of eyes would be appreciated. Plus,
> I'd like to know if others also prefer splitting this condition like
> I did.

Actually, upon rewriting the Ada patch, it occured to me that the
following is even more helpful in inserting support for task-specific
breakpoints... I'm currently testing the attached patch.

-- 
Joel

[-- Attachment #2: bp-cleanup.diff --]
[-- Type: text/x-diff, Size: 2098 bytes --]

commit 485117c6c4ed62aed31cc919f435494eb6faad49
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Wed Mar 25 10:38:22 2009 -0700

        Minor cleanup of breakpoint_thread_match (splittin one condition a bit)
    
        * breakpoint.c (breakpoint_thread_match): Splitt a large condition
        into several smaller conditions.  No behavior change.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7e50342..258a550 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1893,28 +1893,39 @@ int
 breakpoint_thread_match (CORE_ADDR pc, ptid_t ptid)
 {
   const struct bp_location *bpt;
-  int thread;
-
-  thread = pid_to_thread_id (ptid);
-
+  /* The thread ID associated to PTID, computed lazily.  */
+  int thread = -1;
+  
   ALL_BP_LOCATIONS (bpt)
     {
       if (bpt->loc_type != bp_loc_software_breakpoint
 	  && bpt->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      if ((breakpoint_enabled (bpt->owner)
-	   || bpt->owner->enable_state == bp_permanent)
-	  && bpt->address == pc
-	  && (bpt->owner->thread == -1 || bpt->owner->thread == thread))
-	{
-	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
-	    continue;		/* unmapped overlay -- can't be a match */
-	  else
-	    return 1;
-	}
+      if (!breakpoint_enabled (bpt->owner)
+          && bpt->owner->enable_state != bp_permanent)
+        continue;
+
+      if (bpt->address != pc)
+        continue;
+
+      if (bpt->owner->thread != -1)
+        {
+          /* This is a thread-specific breakpoint.  Check that ptid
+             matches that thread.  If thread hasn't been computed yet,
+             it is now time to do so.  */
+          if (thread == -1)
+            thread = pid_to_thread_id (ptid);
+          if (bpt->owner->thread != thread)
+            continue;
+        }
+
+      if (overlay_debugging 
+          && section_is_overlay (bpt->section) 
+          && !section_is_mapped (bpt->section))
+        continue;           /* unmapped overlay -- can't be a match */
+
+      return 1;
     }
 
   return 0;

Re: [RFA/commit] Minor cleanup in breakpoint_thread_match

[Pedro Alves]

On Wednesday 25 March 2009 18:00:33, Joel Brobecker wrote:
> > 2009-03-25  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * breakpoint.c (breakpoint_thread_match): Split a large condition
> >         into several smaller conditions.  No behavior change.
> > 
> > Tested on x86_64-linux. I made this change in a pretty procedural and
> > mechanical way.  But a second pair of eyes would be appreciated. Plus,
> > I'd like to know if others also prefer splitting this condition like
> > I did.
> 
> Actually, upon rewriting the Ada patch, it occured to me that the
> following is even more helpful in inserting support for task-specific
> breakpoints... I'm currently testing the attached patch.
> 

Looks sane to me.

-- 
Pedro Alves

Re: [RFA/commit] Minor cleanup in breakpoint_thread_match

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

> > > 2009-03-25  Joel Brobecker  <brobecker@adacore.com>
> > > 
> > >         * breakpoint.c (breakpoint_thread_match): Split a large condition
> > >         into several smaller conditions.  No behavior change.
> > > 
> Looks sane to me.

Thanks, much, Pedro. Here is what I ended checking in. It's identical
to the above, except for some whitespace screwups.

-- 
Joel

[-- Attachment #2: cleanup.diff --]
[-- Type: text/x-diff, Size: 1621 bytes --]

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7e50342..7ffdf77 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1893,28 +1893,39 @@ int
 breakpoint_thread_match (CORE_ADDR pc, ptid_t ptid)
 {
   const struct bp_location *bpt;
-  int thread;
-
-  thread = pid_to_thread_id (ptid);
-
+  /* The thread ID associated to PTID, computed lazily.  */
+  int thread = -1;
+  
   ALL_BP_LOCATIONS (bpt)
     {
       if (bpt->loc_type != bp_loc_software_breakpoint
 	  && bpt->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      if ((breakpoint_enabled (bpt->owner)
-	   || bpt->owner->enable_state == bp_permanent)
-	  && bpt->address == pc
-	  && (bpt->owner->thread == -1 || bpt->owner->thread == thread))
+      if (!breakpoint_enabled (bpt->owner)
+	  && bpt->owner->enable_state != bp_permanent)
+	continue;
+
+      if (bpt->address != pc)
+	continue;
+
+      if (bpt->owner->thread != -1)
 	{
-	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
-	    continue;		/* unmapped overlay -- can't be a match */
-	  else
-	    return 1;
+	  /* This is a thread-specific breakpoint.  Check that ptid
+	     matches that thread.  If thread hasn't been computed yet,
+	     it is now time to do so.  */
+	  if (thread == -1)
+	    thread = pid_to_thread_id (ptid);
+	  if (bpt->owner->thread != thread)
+	    continue;
 	}
+
+      if (overlay_debugging 
+	  && section_is_overlay (bpt->section) 
+	  && !section_is_mapped (bpt->section))
+	continue;	    /* unmapped overlay -- can't be a match */
+
+      return 1;
     }
 
   return 0;