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] | |
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
Cheers, Mike
Attachment:
ChangeLog.txt
Description: Text document
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ab5f324..8838477 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2088,9 +2088,12 @@ static int
insert_bp_location (struct bp_location *bl,
struct ui_file *tmp_error_stream,
int *disabled_breaks,
- int *hw_breakpoint_error)
+ int *hw_breakpoint_error,
+ int *hw_bp_error_explained_already)
{
int val = 0;
+ char *hw_bp_err_string = NULL;
+ struct gdb_exception e;
if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update))
return 0;
@@ -2186,8 +2189,15 @@ insert_bp_location (struct bp_location *bl,
|| !(section_is_overlay (bl->section)))
{
/* No overlay handling: just set the breakpoint. */
-
- val = bl->owner->ops->insert_location (bl);
+ TRY_CATCH (e, RETURN_MASK_ALL)
+ {
+ val = bl->owner->ops->insert_location (bl);
+ }
+ if (e.reason < 0)
+ {
+ val = 1;
+ hw_bp_err_string = (char *) e.message;
+ }
}
else
{
@@ -2221,7 +2231,15 @@ insert_bp_location (struct bp_location *bl,
if (section_is_mapped (bl->section))
{
/* Yes. This overlay section is mapped into memory. */
- val = bl->owner->ops->insert_location (bl);
+ TRY_CATCH (e, RETURN_MASK_ALL)
+ {
+ val = bl->owner->ops->insert_location (bl);
+ }
+ if (e.reason < 0)
+ {
+ val = 1;
+ hw_bp_err_string = (char *) e.message;
+ }
}
else
{
@@ -2257,11 +2275,13 @@ 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);
+ *hw_breakpoint_error = 1;
+ *hw_bp_error_explained_already = hw_bp_err_string != NULL;
+ fprintf_unfiltered (tmp_error_stream,
+ "Cannot insert hardware breakpoint %d%s",
+ bl->owner->number, hw_bp_err_string ? ":" : ".\n");
+ if (hw_bp_err_string)
+ fprintf_unfiltered (tmp_error_stream, "%s.\n", hw_bp_err_string);
}
else
{
@@ -2453,6 +2473,7 @@ update_inserted_breakpoint_locations (void)
int val = 0;
int disabled_breaks = 0;
int hw_breakpoint_error = 0;
+ int hw_bp_details_reported = 0;
struct ui_file *tmp_error_stream = mem_fileopen ();
struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
@@ -2487,7 +2508,7 @@ update_inserted_breakpoint_locations (void)
continue;
val = insert_bp_location (bl, tmp_error_stream, &disabled_breaks,
- &hw_breakpoint_error);
+ &hw_breakpoint_error, &hw_bp_details_reported);
if (val)
error_flag = val;
}
@@ -2512,6 +2533,7 @@ insert_breakpoint_locations (void)
int val = 0;
int disabled_breaks = 0;
int hw_breakpoint_error = 0;
+ int hw_bp_error_explained_already = 0;
struct ui_file *tmp_error_stream = mem_fileopen ();
struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
@@ -2545,7 +2567,7 @@ insert_breakpoint_locations (void)
continue;
val = insert_bp_location (bl, tmp_error_stream, &disabled_breaks,
- &hw_breakpoint_error);
+ &hw_breakpoint_error, &hw_bp_error_explained_already);
if (val)
error_flag = val;
}
@@ -2590,7 +2612,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 && !hw_bp_error_explained_already)
{
fprintf_unfiltered (tmp_error_stream,
"Could not insert hardware breakpoints:\n\
@@ -2652,7 +2674,7 @@ reattach_breakpoints (int pid)
struct bp_location *bl, **blp_tmp;
int val;
struct ui_file *tmp_error_stream;
- int dummy1 = 0, dummy2 = 0;
+ int dummy1 = 0, dummy2 = 0, dummy3 = 0;
struct inferior *inf;
struct thread_info *tp;
@@ -2676,7 +2698,7 @@ reattach_breakpoints (int pid)
if (bl->inserted)
{
bl->inserted = 0;
- val = insert_bp_location (bl, tmp_error_stream, &dummy1, &dummy2);
+ val = insert_bp_location (bl, tmp_error_stream, &dummy1, &dummy2, &dummy3);
if (val != 0)
{
do_cleanups (old_chain);
diff --git a/gdb/remote.c b/gdb/remote.c
index 68864d1..525d0ff 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6979,6 +6979,7 @@ putpkt_binary (char *buf, int cnt)
int ch;
int tcount = 0;
char *p;
+ char *message;
/* Catch cases like trying to read memory or listing threads while
we're waiting for a stop reply. The remote server wouldn't be
@@ -8100,6 +8101,7 @@ remote_insert_hw_breakpoint (struct gdbarch *gdbarch,
CORE_ADDR addr;
struct remote_state *rs;
char *p, *endbuf;
+ char *message;
/* The length field should be set to the size of a breakpoint
instruction, even though we aren't inserting one ourselves. */
@@ -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 ("Remote failure reply: %s", message + 1);
+ }
+ return -1;
case PACKET_UNKNOWN:
return -1;
case PACKET_OK:
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |