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] Hardware breakpoint error reporting


On 5/3/12 9:25 AM, Mike Wrighton wrote:
Hi,

I've attached a patch to improve error reporting for hardware breakpoint insertion errors on remote targets. Currently the error response coming back over RSP is discarded, and instead the default error:

"... You may have requested too many hardware breakpoints/watchpoints."

is printed, which is often not helpful to users.

Note my copyright assignment is currently in progress.


I understand the assignment is now done, which is great!


Conceptually the patch is good, I just have a couple coding quibbles:

@@ -2257,11 +2274,20 @@ insert_bp_location (struct bp_location *bl,
 	    {
 	      if (bl->loc_type == bp_loc_hardware_breakpoint)
 		{
-		  *hw_breakpoint_error = 1;
-		  fprintf_unfiltered (tmp_error_stream,
-				      "Cannot insert hardware "
-				      "breakpoint %d.\n",
-				      bl->owner->number);
+                  if (hw_bp_err_string)
+                    {
+                      *hw_breakpoint_error = 2;
+                      fprintf_unfiltered (tmp_error_stream,
+                                          "Cannot insert hardware breakpoint %d: %s.\n",
+                                          bl->owner->number, hw_bp_err_string);
+                    }
+                  else
+                    {
+                      *hw_breakpoint_error = 1;
+                      fprintf_unfiltered (tmp_error_stream,
+                                          "Cannot insert hardware breakpoint %d.\n",
+                                          bl->owner->number);
+                    }
 		}
 	      else
 		{
@@ -2590,7 +2616,7 @@ insert_breakpoint_locations (void)
     {
       /* If a hardware breakpoint or watchpoint was inserted, add a
          message about possibly exhausted resources.  */
-      if (hw_breakpoint_error)
+      if (hw_breakpoint_error == 1)
 	{
 	  fprintf_unfiltered (tmp_error_stream,
 			      "Could not insert hardware breakpoints:\n\


In general, we'd rather not have obscurely-meaningful integer values running around, and certainly not without documentation in the code.


Thinking about what's really going on here, the lower-level insertion has gotten an explanation of what went wrong, and so you want the higher-level code not to speculate on possibilities that are obviously irrelevant. So maybe another flag, like "hw_bp_error_explained_already"? I'd also try to do a single "Cannot insert..." printf, and have the explanation as a second print that is an addendum.


@@ -8131,6 +8133,13 @@ remote_insert_hw_breakpoint (struct gdbarch *gdbarch, switch (packet_ok (rs->buf,&remote_protocol_packets[PACKET_Z1])) { case PACKET_ERROR: + if (rs->buf[1] == '.') + { + message = strchr (rs->buf + 2, '.'); + if (message) + error ("%s", message + 1);


Should have a "Remote failure reply: " at the front of the message, similar to other cases in remote.c; there's no assurance that a bad target won't send back massive line noise as its, ahem, message; so we need the explanatory phrase.


Stan




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]