This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Hardware breakpoint error reporting
- From: Stan Shebs <stanshebs at earthlink dot net>
- To: gdb-patches at sourceware dot org
- Date: Wed, 09 May 2012 10:50:36 -0700
- Subject: Re: [PATCH] Hardware breakpoint error reporting
- References: <4FA2B177.3080200@mentor.com>
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