* [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet @ 2013-10-21 10:30 Hui Zhu 2013-10-21 15:37 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Hui Zhu @ 2013-10-21 10:30 UTC (permalink / raw) To: gdb-patches ml [-- Attachment #1: Type: text/plain, Size: 1334 bytes --] We found that in powerpc arch board, it cannot pass some dprintf test: FAIL: gdb.base/dprintf.exp: dprintf info 2 (pattern 6) FAIL: gdb.mi/mi-dprintf.exp: mi expect stop (unknown output after running) FAIL: gdb.mi/mi-dprintf.exp: mi 1st dprintf, agent (unknown output after running) This because the gdbserver will always tell GDB that it support target-side breakpoint conditions and commands. So "set dprintf-style agent" will always got success. But target-side breakpoint conditions and commands are depend on 'Z' packet because GDB just can post target-side breakpoint conditions and commands with 'Z' packet. The test will check if "set dprintf-style agent" success or not. Because it will always succes. So GDB change the commands to agent-printf that will make test get fail. So I make a patch to check if the low-target support insert_point before return support target-side breakpoint conditions and commands. Tested in powerpc-linux target. 2013-10-21 Hui Zhu <yao@codesourcery.com> * linux-low.c (linux_supports_insert_point): New. (linux_target_op): Add linux_supports_insert_point. * server.c (handle_query): Check target_supports_insert_point before return support target-side breakpoint conditions and commands. * target.h (target_ops): Add supports_insert_point. (target_supports_insert_point): New. [-- Attachment #2: fix-dprintf.txt --] [-- Type: text/plain, Size: 2222 bytes --] --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -4929,6 +4929,15 @@ linux_supports_range_stepping (void) return (*the_low_target.supports_range_stepping) (); } +static int +linux_supports_insert_point (void) +{ + if (the_low_target.insert_point != NULL) + return 1; + + return 0; +} + /* Enumerate spufs IDs for process PID. */ static int spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len) @@ -5825,6 +5834,7 @@ static struct target_ops linux_target_op NULL, #endif linux_supports_range_stepping, + linux_supports_insert_point, }; static void --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -1806,9 +1806,16 @@ handle_query (char *own_buf, int packet_ strcat (own_buf, ";tracenz+"); } - /* Support target-side breakpoint conditions and commands. */ - strcat (own_buf, ";ConditionalBreakpoints+"); - strcat (own_buf, ";BreakpointCommands+"); + /* GDB will send target-side breakpoint conditions and commands packets + with insert point packets. So if target doesn't support insert + point, it should not support target-side breakpoint conditions + and commands packets. */ + if (target_supports_insert_point ()) + { + /* Support target-side breakpoint conditions and commands. */ + strcat (own_buf, ";ConditionalBreakpoints+"); + strcat (own_buf, ";BreakpointCommands+"); + } if (target_supports_agent ()) strcat (own_buf, ";QAgent+"); --- a/gdb/gdbserver/target.h +++ b/gdb/gdbserver/target.h @@ -365,6 +365,9 @@ struct target_ops /* Return true if target supports range stepping. */ int (*supports_range_stepping) (void); + + /* Return true if target supports insert_point. */ + int (*supports_insert_point) (void); }; extern struct target_ops *the_target; @@ -504,6 +507,10 @@ int kill_inferior (int); (the_target->supports_range_stepping ? \ (*the_target->supports_range_stepping) () : 0) +#define target_supports_insert_point() \ + (the_target->supports_insert_point ? \ + (*the_target->supports_insert_point) () : 0) + /* Start non-stop mode, returns 0 on success, -1 on failure. */ int start_non_stop (int nonstop); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-10-21 10:30 [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Hui Zhu @ 2013-10-21 15:37 ` Pedro Alves 2013-11-28 10:56 ` Hui Zhu 0 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-10-21 15:37 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml On 10/21/2013 11:30 AM, Hui Zhu wrote: > We found that in powerpc arch board, it cannot pass some dprintf test: > FAIL: gdb.base/dprintf.exp: dprintf info 2 (pattern 6) > FAIL: gdb.mi/mi-dprintf.exp: mi expect stop (unknown output after running) > FAIL: gdb.mi/mi-dprintf.exp: mi 1st dprintf, agent (unknown output after running) > > This because the gdbserver will always tell GDB that it support target-side breakpoint conditions and commands. So "set dprintf-style agent" will always got success. > But target-side breakpoint conditions and commands are depend on 'Z' packet because GDB just can post target-side breakpoint conditions and commands with 'Z' packet. > The test will check if "set dprintf-style agent" success or not. Because it will always succes. So GDB change the commands to agent-printf that will make test get fail. There happens to be a single "the_low_target.insert_point" entry point in gdbserver for all sorts of Z packets. But Z0, Z1, Z2, etc., they're all different packets (software breakpoints, hardware breakpoints, watchpoints, etc.). A target might well support Z1 but not Z0. Or it may support Z2/Z3/Z4, but not Z0..Z1 -- that's the case of MIPS gdbserver. So, having a "the_low_target.insert_point" hook installed does not actually mean that dprintf will work. (Consider what the patch would need to do if instead of a single "the_low_target.insert_point" entry point, we had one for each Zx packet.) In the general case, gdbserver can't possibly know what packet GDB will want to send with target conditions or target commands in. GDB could end up sending either a Z0, or a Z1. The target might support Z1, but not Z0, leaving gdb to handle memory breakpoints. The concept of target-side conditions and commands is broader than dprintf. I think that first, GDB should be taught to handle this scenario itself. That is, if we try this against gdbserver: (gdb) set dprintf-style agent (gdb) set remote Z-packet off (gdb) dprint main,"foo" Dprintf 1 at 0x410776: file foo.c, line 10. (gdb) c GDB should realize that the dprintf won't work, right at insertion time, but, it currently does not. -- Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-10-21 15:37 ` Pedro Alves @ 2013-11-28 10:56 ` Hui Zhu 2013-11-28 17:38 ` Maciej W. Rozycki ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Hui Zhu @ 2013-11-28 10:56 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches ml On 10/21/13 23:37, Pedro Alves wrote: > On 10/21/2013 11:30 AM, Hui Zhu wrote: >> We found that in powerpc arch board, it cannot pass some dprintf test: >> FAIL: gdb.base/dprintf.exp: dprintf info 2 (pattern 6) >> FAIL: gdb.mi/mi-dprintf.exp: mi expect stop (unknown output after running) >> FAIL: gdb.mi/mi-dprintf.exp: mi 1st dprintf, agent (unknown output after running) >> >> This because the gdbserver will always tell GDB that it support target-side breakpoint conditions and commands. So "set dprintf-style agent" will always got success. >> But target-side breakpoint conditions and commands are depend on 'Z' packet because GDB just can post target-side breakpoint conditions and commands with 'Z' packet. >> The test will check if "set dprintf-style agent" success or not. Because it will always succes. So GDB change the commands to agent-printf that will make test get fail. > > There happens to be a single "the_low_target.insert_point" entry > point in gdbserver for all sorts of Z packets. But Z0, Z1, Z2, etc., > they're all different packets (software breakpoints, hardware breakpoints, > watchpoints, etc.). A target might well support Z1 but not Z0. Or it > may support Z2/Z3/Z4, but not Z0..Z1 -- that's the case of MIPS gdbserver. > > So, having a "the_low_target.insert_point" hook installed does not > actually mean that dprintf will work. (Consider what the patch > would need to do if instead of a single "the_low_target.insert_point" > entry point, we had one for each Zx packet.) > > In the general case, gdbserver can't possibly know what packet GDB > will want to send with target conditions or target commands in. GDB > could end up sending either a Z0, or a Z1. The target might support > Z1, but not Z0, leaving gdb to handle memory breakpoints. > The concept of target-side conditions and commands is broader > than dprintf. > > I think that first, GDB should be taught to handle this scenario > itself. That is, if we try this against gdbserver: > > (gdb) set dprintf-style agent > (gdb) set remote Z-packet off > (gdb) dprint main,"foo" > Dprintf 1 at 0x410776: file foo.c, line 10. > (gdb) c > > GDB should realize that the dprintf won't work, right > at insertion time, but, it currently does not. > Hi Pedro, According to your comments. I make a new patch for it. It add a check in remote_insert_breakpoint. If this breakpoint have target breakpoint but it is not handled by "Z" packet, function will return error. Please help me review it. Because mi-dprintf.exp and dprintf.exp use "set dprintf-style agent" check if target support target-side breakpoint, they are still got fail after this patch. If you think this patch's idea is OK, I will make patch for them. Thanks, Hui 2013-11-28 Hui Zhu <hui@codesourcery.com> * remote.c (remote_insert_breakpoint): Add have_target_target_side_commands. --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8187,6 +8187,14 @@ static int remote_insert_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { + int have_target_target_side_commands = 0; + + /* Check bp_tgt->tcommands in this place because + remote_add_target_side_commands will release bp_tgt->tcommands. */ + + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) + have_target_target_side_commands = 1; + /* Try the "Z" s/w breakpoint packet if it is not already disabled. If it succeeds, then set the support to PACKET_ENABLE. If it fails, and the user has explicitly requested the Z support then @@ -8240,6 +8248,13 @@ remote_insert_breakpoint (struct gdbarch } } + if (have_target_target_side_commands) + { + warning (_("\ +Target doesn't support breakpoints that have target side commands.")); + return -1; + } + return memory_insert_breakpoint (gdbarch, bp_tgt); } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-11-28 10:56 ` Hui Zhu @ 2013-11-28 17:38 ` Maciej W. Rozycki 2013-11-29 9:41 ` Hui Zhu 2013-11-29 15:27 ` [pushed] Plug target side conditions and commands leaks (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) Pedro Alves 2013-11-29 16:05 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves 2 siblings, 1 reply; 27+ messages in thread From: Maciej W. Rozycki @ 2013-11-28 17:38 UTC (permalink / raw) To: Hui Zhu; +Cc: Pedro Alves, gdb-patches ml On Thu, 28 Nov 2013, Hui Zhu wrote: > According to your comments. I make a new patch for it. > It add a check in remote_insert_breakpoint. If this breakpoint have target > breakpoint but it is not handled by "Z" packet, function will return error. > > Please help me review it. > > Because mi-dprintf.exp and dprintf.exp use "set dprintf-style agent" check if > target support target-side breakpoint, they are still got fail after this > patch. > If you think this patch's idea is OK, I will make patch for them. > > Thanks, > Hui > > 2013-11-28 Hui Zhu <hui@codesourcery.com> > > * remote.c (remote_insert_breakpoint): Add > have_target_target_side_commands. Please note that this is PR gdb/16101 now and refer to it in the ChangeLog entry. Maciej ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-11-28 17:38 ` Maciej W. Rozycki @ 2013-11-29 9:41 ` Hui Zhu 0 siblings, 0 replies; 27+ messages in thread From: Hui Zhu @ 2013-11-29 9:41 UTC (permalink / raw) To: Maciej W. Rozycki; +Cc: Pedro Alves, gdb-patches ml On 11/29/13 00:18, Maciej W. Rozycki wrote: > On Thu, 28 Nov 2013, Hui Zhu wrote: > >> According to your comments. I make a new patch for it. >> It add a check in remote_insert_breakpoint. If this breakpoint have target >> breakpoint but it is not handled by "Z" packet, function will return error. >> >> Please help me review it. >> >> Because mi-dprintf.exp and dprintf.exp use "set dprintf-style agent" check if >> target support target-side breakpoint, they are still got fail after this >> patch. >> If you think this patch's idea is OK, I will make patch for them. >> >> Thanks, >> Hui >> >> 2013-11-28 Hui Zhu <hui@codesourcery.com> >> >> * remote.c (remote_insert_breakpoint): Add >> have_target_target_side_commands. > > Please note that this is PR gdb/16101 now and refer to it in the > ChangeLog entry. > > Maciej > Hi Maciej, Thanks for your remind. Update changelog to: 2013-11-28 Hui Zhu <hui@codesourcery.com> PR gdb/16101 * remote.c (remote_insert_breakpoint): Add have_target_target_side_commands. Thanks, Hui ^ permalink raw reply [flat|nested] 27+ messages in thread
* [pushed] Plug target side conditions and commands leaks (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) 2013-11-28 10:56 ` Hui Zhu 2013-11-28 17:38 ` Maciej W. Rozycki @ 2013-11-29 15:27 ` Pedro Alves 2013-11-29 16:05 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves 2 siblings, 0 replies; 27+ messages in thread From: Pedro Alves @ 2013-11-29 15:27 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml On 11/28/2013 09:07 AM, Hui Zhu wrote: > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -8187,6 +8187,14 @@ static int > remote_insert_breakpoint (struct gdbarch *gdbarch, > struct bp_target_info *bp_tgt) > { > + int have_target_target_side_commands = 0; > + > + /* Check bp_tgt->tcommands in this place because > + remote_add_target_side_commands will release bp_tgt->tcommands. */ Well, that's quite fragile, isn't it. If Z packets have been found to not be supported, then nothing is releasing bp_tgt->tcommands (and bp_tgt->conditions). > + > + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) > + have_target_target_side_commands = 1; > + I've pushed this patch below. ---------- Plug target side conditions and commands leaks. The memory management of bp_location->target_info.conditions|tcommands is currently a little fragile. If the target reports support for target conditions or commands, and then target side breakpoint support is disabled, or some error is thrown before remote_add_target_side_XXX is called, we'll leak these lists. This patch makes us free these lists when the locations are deleted, and also, just before recreating the commands|conditions lists. Tested on x86_64 Fedora 17, native and gdbserver. gdb/ 2013-11-29 Pedro Alves <palves@redhat.com> * breakpoint.c (build_target_condition_list): Release previous conditions. (build_target_command_list): Release previous commands. (bp_location_dtor): Release target conditions and commands. * remote.c (remote_add_target_side_condition): Don't release conditions. (remote_add_target_side_commands): Don't release commands. --- gdb/ChangeLog | 10 ++++++++++ gdb/breakpoint.c | 9 +++++++++ gdb/remote.c | 4 ---- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index e86f5c3..5f0626c 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,13 @@ +2013-11-29 Pedro Alves <palves@redhat.com> + + * breakpoint.c (build_target_condition_list): Release previous + conditions. + (build_target_command_list): Release previous commands. + (bp_location_dtor): Release target conditions and commands. + * remote.c (remote_add_target_side_condition): Don't release + conditions. + (remote_add_target_side_commands): Don't release commands. + 2013-11-29 Yao Qi <yao@codesourcery.com> Pedro Alves <palves@redhat.com> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 897b664..111660f 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2096,6 +2096,9 @@ build_target_condition_list (struct bp_location *bl) int modified = bl->needs_update; struct bp_location *loc; + /* Release conditions left over from a previous insert. */ + VEC_free (agent_expr_p, bl->target_info.conditions); + /* This is only meaningful if the target is evaluating conditions and if the user has opted for condition evaluation on the target's @@ -2287,6 +2290,9 @@ build_target_command_list (struct bp_location *bl) int modified = bl->needs_update; struct bp_location *loc; + /* Release commands left over from a previous insert. */ + VEC_free (agent_expr_p, bl->target_info.tcommands); + /* For now, limit to agent-style dprintf breakpoints. */ if (bl->owner->type != bp_dprintf || strcmp (dprintf_style, dprintf_style_agent) != 0) @@ -12734,6 +12740,9 @@ bp_location_dtor (struct bp_location *self) if (self->cond_bytecode) free_agent_expr (self->cond_bytecode); xfree (self->function_name); + + VEC_free (agent_expr_p, self->target_info.conditions); + VEC_free (agent_expr_p, self->target_info.tcommands); } static const struct bp_location_ops bp_location_ops = diff --git a/gdb/remote.c b/gdb/remote.c index 186c058..be54450 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8151,8 +8151,6 @@ remote_add_target_side_condition (struct gdbarch *gdbarch, buf = pack_hex_byte (buf, aexpr->buf[i]); *buf = '\0'; } - - VEC_free (agent_expr_p, bp_tgt->conditions); return 0; } @@ -8183,8 +8181,6 @@ remote_add_target_side_commands (struct gdbarch *gdbarch, buf = pack_hex_byte (buf, aexpr->buf[i]); *buf = '\0'; } - - VEC_free (agent_expr_p, bp_tgt->tcommands); } /* Insert a breakpoint. On targets that have software breakpoint ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-11-28 10:56 ` Hui Zhu 2013-11-28 17:38 ` Maciej W. Rozycki 2013-11-29 15:27 ` [pushed] Plug target side conditions and commands leaks (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) Pedro Alves @ 2013-11-29 16:05 ` Pedro Alves 2013-12-02 12:45 ` Hui Zhu 2 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-11-29 16:05 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml On 11/28/2013 09:07 AM, Hui Zhu wrote: > + if (have_target_target_side_commands) This can now just be: if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) OK with that change. > + { > + warning (_("\ > +Target doesn't support breakpoints that have target side commands.")); I was doing to suggest making this an error instead, that insert_bp_location would print the error string, but that's only true for hw breakpoints... insert_bp_location's error handling is quite messy. For instance, if this breakpoint is in a a shared library, this will disable the breakpoint, even though the cause of the error is clearly not that the shared library disappeared (i.e., not a memory error). > + return -1; > + } -- Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-11-29 16:05 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves @ 2013-12-02 12:45 ` Hui Zhu 2013-12-02 14:38 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Hui Zhu @ 2013-12-02 12:45 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches ml [-- Attachment #1: Type: text/plain, Size: 1571 bytes --] On 11/29/13 23:10, Pedro Alves wrote: > On 11/28/2013 09:07 AM, Hui Zhu wrote: > >> + if (have_target_target_side_commands) > > This can now just be: > > if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) > > OK with that change. > >> + { >> + warning (_("\ >> +Target doesn't support breakpoints that have target side commands.")); > > I was doing to suggest making this an error instead, that > insert_bp_location would print the error string, but that's > only true for hw breakpoints... insert_bp_location's error > handling is quite messy. For instance, if this breakpoint > is in a a shared library, this will disable the breakpoint, > even though the cause of the error is clearly not that the > shared library disappeared (i.e., not a memory error). > >> + return -1; >> + } Updated the patch according to your comments. And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets. The patches were tested and pass regression test on X86_64 and PPC. Please help me review it. Thanks, Hui 2013-12-02 Hui Zhu <hui@codesourcery.com> PR gdb/16101 * remote.c (remote_insert_breakpoint): If this breakpoint has target-side commands but this stub doesn't support Z0 packets, throw error. 2013-12-02 Hui Zhu <hui@codesourcery.com> PR gdb/16101 * gdb.base/dprintf.exp: Add check for the the gdbserver of some architecture doesn't support some "Zx" doesn't support some "Zx" packets. * gdb.mi/mi-dprintf.exp: Ditto. * lib/mi-support.exp: Add check for continue get error. [-- Attachment #2: fix-dprintf-test-v2.txt --] [-- Type: text/plain, Size: 433 bytes --] --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8244,6 +8244,12 @@ remote_insert_breakpoint (struct gdbarch } } + /* If this breakpoint has target-side commands but this stub doesn't + support Z0 packets, throw error. */ + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) + error (_("\ +Target doesn't support breakpoints that have target side commands.")); + return memory_insert_breakpoint (gdbarch, bp_tgt); } [-- Attachment #3: dprintf-test.txt --] [-- Type: text/plain, Size: 2033 bytes --] --- a/gdb/testsuite/gdb.base/dprintf.exp +++ b/gdb/testsuite/gdb.base/dprintf.exp @@ -108,6 +108,26 @@ gdb_test_multiple "set dprintf-style age } } +# Continue with target-side breakpoint commands will get error if GDB +# work with the gdbserver of some architecture doesn't support some "Zx" +# packets. +if $target_can_dprintf { + gdb_run_cmd + + gdb_test "" "Breakpoint" + + set msg "continue with target-side breakpoint commands" + gdb_test_multiple "continue" $msg { + -re "Cannot insert breakpoint $decimal\.\r\n.*\r\n$gdb_prompt $" { + set target_can_dprintf 0 + pass "$msg - cannot do" + } + -re ".*$gdb_prompt $" { + pass "$msg - can do" + } + } +} + if $target_can_dprintf { gdb_run_cmd @@ -120,7 +140,7 @@ if $target_can_dprintf { gdb_test_sequence "info breakpoints" "dprintf info 2" { "\[\r\n\]Num Type Disp Enb Address +What" "\[\r\n\]2 breakpoint" - "\[\r\n\]\tbreakpoint already hit 2 times" + "\[\r\n\]\tbreakpoint already hit 3 times" "\[\r\n\]3 dprintf" "\[\r\n\]\tbreakpoint already hit 2 times" "\[\r\n\] agent-printf \"At foo entry\\\\n\"" --- a/gdb/testsuite/gdb.mi/mi-dprintf.exp +++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp @@ -139,6 +139,16 @@ gdb_expect { } } +# Continue with target-side breakpoint commands will get error if GDB +# work with The gdbserver of some architecture doesn't support some "Zx" +# packets. +if $target_can_dprintf { + if {[mi_run_cmd] == -1} { + pass "continue with target-side breakpoint commands - cannot do" + set target_can_dprintf 0 + } +} + if $target_can_dprintf { mi_run_cmd --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -843,6 +843,7 @@ proc mi_run_cmd_full {use_mi_command arg send_gdb "${run_prefix}continue\n" gdb_expect 60 { -re "${run_match}\\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\r\n$mi_gdb_prompt" {} + -re "\\^error.*\r\n$mi_gdb_prompt" { return -1 } default {} } return 0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-02 12:45 ` Hui Zhu @ 2013-12-02 14:38 ` Pedro Alves 2013-12-03 4:50 ` Hui Zhu 0 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-12-02 14:38 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml On 12/02/2013 12:45 PM, Hui Zhu wrote: > On 11/29/13 23:10, Pedro Alves wrote: >> > On 11/28/2013 09:07 AM, Hui Zhu wrote: >> > >>> >> + if (have_target_target_side_commands) >> > >> > This can now just be: >> > >> > if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) >> > >> > OK with that change. >> > >>> >> + { >>> >> + warning (_("\ >>> >> +Target doesn't support breakpoints that have target side commands.")); >> > >> > I was doing to suggest making this an error instead, that >> > insert_bp_location would print the error string, but that's >> > only true for hw breakpoints... insert_bp_location's error >> > handling is quite messy. For instance, if this breakpoint >> > is in a a shared library, this will disable the breakpoint, >> > even though the cause of the error is clearly not that the >> > shared library disappeared (i.e., not a memory error). >> > >>> >> + return -1; >>> >> + } > Updated the patch according to your comments. But you switched to "error" anyway? Above I was saying that I was going that suggest it, but then explained why I didn't think it would work. Was I wrong? > > And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets. > > The patches were tested and pass regression test on X86_64 and PPC. (It's best to stick to one patch per email, otherwise things end up confusing. I suggest looking into git send-email.) -- Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-02 14:38 ` Pedro Alves @ 2013-12-03 4:50 ` Hui Zhu 2013-12-03 4:54 ` Hui Zhu 2013-12-06 19:29 ` Pedro Alves 0 siblings, 2 replies; 27+ messages in thread From: Hui Zhu @ 2013-12-03 4:50 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches ml On 12/02/13 22:37, Pedro Alves wrote: > On 12/02/2013 12:45 PM, Hui Zhu wrote: >> On 11/29/13 23:10, Pedro Alves wrote: >>>> On 11/28/2013 09:07 AM, Hui Zhu wrote: >>>> >>>>>> + if (have_target_target_side_commands) >>>> >>>> This can now just be: >>>> >>>> if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) >>>> >>>> OK with that change. >>>> >>>>>> + { >>>>>> + warning (_("\ >>>>>> +Target doesn't support breakpoints that have target side commands.")); >>>> >>>> I was doing to suggest making this an error instead, that >>>> insert_bp_location would print the error string, but that's >>>> only true for hw breakpoints... insert_bp_location's error >>>> handling is quite messy. For instance, if this breakpoint >>>> is in a a shared library, this will disable the breakpoint, >>>> even though the cause of the error is clearly not that the >>>> shared library disappeared (i.e., not a memory error). >>>> >>>>>> + return -1; >>>>>> + } >> Updated the patch according to your comments. > > But you switched to "error" anyway? Above I was saying that I > was going that suggest it, but then explained why I didn't think > it would work. Was I wrong? The reason is insert_bp_location doesn't have code to handle this error. I make a new patch that include this code. Please help me review it. > >> >> And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets. >> >> The patches were tested and pass regression test on X86_64 and PPC. > > (It's best to stick to one patch per email, otherwise things > end up confusing. I suggest looking into git send-email.) > OK. I will post new version of this patch in another mail. Thanks, Hui 2013-12-03 Hui Zhu <hui@codesourcery.com> PR gdb/16101 * breakpoint.c (insert_bp_location): output error message of software breakpoints. * remote.c (remote_insert_breakpoint): If this breakpoint has target-side commands but this stub doesn't support Z0 packets, throw error. --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2588,6 +2588,12 @@ insert_bp_location (struct bp_location * if (hw_bp_err_string) fprintf_unfiltered (tmp_error_stream, "%s.\n", hw_bp_err_string); } + else if (hw_bp_err_string != NULL) + { + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d:%s\n", + bl->owner->number, hw_bp_err_string); + } else { char *message = memory_error_message (TARGET_XFER_E_IO, --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8244,6 +8244,12 @@ remote_insert_breakpoint (struct gdbarch } } + /* If this breakpoint has target-side commands but this stub doesn't + support Z0 packets, throw error. */ + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) + error (_("\ +Target doesn't support breakpoints that have target side commands.")); + return memory_insert_breakpoint (gdbarch, bp_tgt); } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-03 4:50 ` Hui Zhu @ 2013-12-03 4:54 ` Hui Zhu 2013-12-06 19:29 ` Pedro Alves 1 sibling, 0 replies; 27+ messages in thread From: Hui Zhu @ 2013-12-03 4:54 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches ml On 12/03/13 12:49, Hui Zhu wrote: > On 12/02/13 22:37, Pedro Alves wrote: >> On 12/02/2013 12:45 PM, Hui Zhu wrote: >>> On 11/29/13 23:10, Pedro Alves wrote: >>>>> On 11/28/2013 09:07 AM, Hui Zhu wrote: >>>>> >>>>>>> + if (have_target_target_side_commands) >>>>> >>>>> This can now just be: >>>>> >>>>> if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) >>>>> >>>>> OK with that change. >>>>> >>>>>>> + { >>>>>>> + warning (_("\ >>>>>>> +Target doesn't support breakpoints that have target side commands.")); >>>>> >>>>> I was doing to suggest making this an error instead, that >>>>> insert_bp_location would print the error string, but that's >>>>> only true for hw breakpoints... insert_bp_location's error >>>>> handling is quite messy. For instance, if this breakpoint >>>>> is in a a shared library, this will disable the breakpoint, >>>>> even though the cause of the error is clearly not that the >>>>> shared library disappeared (i.e., not a memory error). >>>>> >>>>>>> + return -1; >>>>>>> + } >>> Updated the patch according to your comments. >> >> But you switched to "error" anyway? Above I was saying that I >> was going that suggest it, but then explained why I didn't think >> it would work. Was I wrong? > > The reason is insert_bp_location doesn't have code to handle this error. I make a new patch that include this code. > > Please help me review it. > >> >>> >>> And I make a patch for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets. >>> >>> The patches were tested and pass regression test on X86_64 and PPC. >> >> (It's best to stick to one patch per email, otherwise things >> end up confusing. I suggest looking into git send-email.) >> > > OK. I will post new version of this patch in another mail. > > Thanks, > Hui > This patch is for dprintf.exp and mi-dprintf.exp to make test OK on the target that doesn't support "Zx" packets. Thanks, Hui 2013-12-03 Hui Zhu <hui@codesourcery.com> PR gdb/16101 * gdb.base/dprintf.exp: Add check for the the gdbserver of some architecture doesn't support some "Zx" doesn't support some "Zx" packets. * gdb.mi/mi-dprintf.exp: Ditto. * lib/mi-support.exp: Add check for continue get error. --- a/gdb/testsuite/gdb.base/dprintf.exp +++ b/gdb/testsuite/gdb.base/dprintf.exp @@ -108,6 +108,26 @@ gdb_test_multiple "set dprintf-style age } } +# Continue with target-side breakpoint commands will get error if GDB +# work with The gdbserver of some architecture doesn't support some "Zx" +# packets. +if $target_can_dprintf { + gdb_run_cmd + + gdb_test "" "Breakpoint" + + set msg "continue with target-side breakpoint commands" + gdb_test_multiple "continue" $msg { + -re "Cannot insert breakpoint $decimal:.*\r\n.*\r\n$gdb_prompt $" { + set target_can_dprintf 0 + pass "$msg - cannot do" + } + -re ".*$gdb_prompt $" { + pass "$msg - can do" + } + } +} + if $target_can_dprintf { gdb_run_cmd @@ -120,7 +140,7 @@ if $target_can_dprintf { gdb_test_sequence "info breakpoints" "dprintf info 2" { "\[\r\n\]Num Type Disp Enb Address +What" "\[\r\n\]2 breakpoint" - "\[\r\n\]\tbreakpoint already hit 2 times" + "\[\r\n\]\tbreakpoint already hit 3 times" "\[\r\n\]3 dprintf" "\[\r\n\]\tbreakpoint already hit 2 times" "\[\r\n\] agent-printf \"At foo entry\\\\n\"" --- a/gdb/testsuite/gdb.mi/mi-dprintf.exp +++ b/gdb/testsuite/gdb.mi/mi-dprintf.exp @@ -139,6 +139,16 @@ gdb_expect { } } +# Continue with target-side breakpoint commands will get error if GDB +# work with The gdbserver of some architecture doesn't support some "Zx" +# packets. +if $target_can_dprintf { + if {[mi_run_cmd] == -1} { + pass "continue with target-side breakpoint commands - cannot do" + set target_can_dprintf 0 + } +} + if $target_can_dprintf { mi_run_cmd --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -843,6 +843,7 @@ proc mi_run_cmd_full {use_mi_command arg send_gdb "${run_prefix}continue\n" gdb_expect 60 { -re "${run_match}\\^running\[\r\n\]+\\*running,thread-id=\"\[^\"\]+\"\r\n$mi_gdb_prompt" {} + -re "\\^error.*\r\n$mi_gdb_prompt" { return -1 } default {} } return 0 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-03 4:50 ` Hui Zhu 2013-12-03 4:54 ` Hui Zhu @ 2013-12-06 19:29 ` Pedro Alves 2013-12-08 5:19 ` Hui Zhu 1 sibling, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-12-06 19:29 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml Please analyze the insert_bp_location's error handling carefully: - if solib_name_from_address is true (the dprintf is set in a shared library), we won't see this new error message, while we should. - 'hw_bp_err_string' ends up misnamed after this patch. -- Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-06 19:29 ` Pedro Alves @ 2013-12-08 5:19 ` Hui Zhu 2013-12-08 8:34 ` Doug Evans 2013-12-09 19:48 ` Pedro Alves 0 siblings, 2 replies; 27+ messages in thread From: Hui Zhu @ 2013-12-08 5:19 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches ml On 12/07/13 03:29, Pedro Alves wrote: > Please analyze the insert_bp_location's error handling carefully: > > - if solib_name_from_address is true (the dprintf is set in a > shared library), we won't see this new error message, while > we should. I change all this part to: First, output error message. Second, if it is solib breakpoint, disable breakpoint. > - 'hw_bp_err_string' ends up misnamed after this patch. Change it to "bp_err_string". > Post a new patch according to your comments. This patch is tested and pass the regression test in x86_64 linux. Please help me review it. Thanks, Hui 2013-12-08 Hui Zhu <hui@codesourcery.com> PR gdb/16101 * breakpoint.c (insert_bp_location): Change hw_bp_err_string to bp_err_string. Output error message of software breakpoints. Make solib error message output use same code with hardware breakpoints and software breakpoints. * remote.c (remote_insert_breakpoint): If this breakpoint has target-side commands but this stub doesn't support Z0 packets, throw error. --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2396,7 +2396,7 @@ insert_bp_location (struct bp_location * int *hw_bp_error_explained_already) { int val = 0; - char *hw_bp_err_string = NULL; + char *bp_err_string = NULL; struct gdb_exception e; if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update)) @@ -2501,7 +2501,7 @@ insert_bp_location (struct bp_location * if (e.reason < 0) { val = 1; - hw_bp_err_string = (char *) e.message; + bp_err_string = (char *) e.message; } } else @@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location * if (e.reason < 0) { val = 1; - hw_bp_err_string = (char *) e.message; + bp_err_string = (char *) e.message; } } else @@ -2557,6 +2557,36 @@ insert_bp_location (struct bp_location * if (val) { /* Can't set the breakpoint. */ + if (bl->loc_type == bp_loc_hardware_breakpoint) + { + *hw_breakpoint_error = 1; + *hw_bp_error_explained_already = bp_err_string != NULL; + fprintf_unfiltered (tmp_error_stream, + "Cannot insert hardware breakpoint %d%s", + bl->owner->number, bp_err_string ? ":" : ".\n"); + if (bp_err_string) + fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_string); + } + else if (bp_err_string != NULL) + { + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d:%s\n", + bl->owner->number, bp_err_string); + } + else + { + char *message = memory_error_message (TARGET_XFER_E_IO, + bl->gdbarch, bl->address); + struct cleanup *old_chain = make_cleanup (xfree, message); + + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d.\n" + "%s\n", + bl->owner->number, message); + + do_cleanups (old_chain); + } + if (solib_name_from_address (bl->pspace, bl->address)) { /* See also: disable_breakpoints_in_shlibs. */ @@ -2564,45 +2594,13 @@ insert_bp_location (struct bp_location * bl->shlib_disabled = 1; observer_notify_breakpoint_modified (bl->owner); if (!*disabled_breaks) - { - fprintf_unfiltered (tmp_error_stream, - "Cannot insert breakpoint %d.\n", - bl->owner->number); - fprintf_unfiltered (tmp_error_stream, - "Temporarily disabling shared " - "library breakpoints:\n"); - } + fprintf_unfiltered (tmp_error_stream, + "Temporarily disabling shared " + "library breakpoints:\n"); *disabled_breaks = 1; fprintf_unfiltered (tmp_error_stream, "breakpoint #%d\n", bl->owner->number); } - else - { - if (bl->loc_type == bp_loc_hardware_breakpoint) - { - *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 - { - char *message = memory_error_message (TARGET_XFER_E_IO, - bl->gdbarch, bl->address); - struct cleanup *old_chain = make_cleanup (xfree, message); - - fprintf_unfiltered (tmp_error_stream, - "Cannot insert breakpoint %d.\n" - "%s\n", - bl->owner->number, message); - - do_cleanups (old_chain); - } - - } } else bl->inserted = 1; --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8260,6 +8260,12 @@ remote_insert_breakpoint (struct gdbarch } } + /* If this breakpoint has target-side commands but this stub doesn't + support Z0 packets, throw error. */ + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) + error (_("\ +Target doesn't support breakpoints that have target side commands.")); + return memory_insert_breakpoint (gdbarch, bp_tgt); } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-08 5:19 ` Hui Zhu @ 2013-12-08 8:34 ` Doug Evans 2013-12-08 14:18 ` Hui Zhu 2013-12-09 19:48 ` Pedro Alves 1 sibling, 1 reply; 27+ messages in thread From: Doug Evans @ 2013-12-08 8:34 UTC (permalink / raw) To: Hui Zhu; +Cc: Pedro Alves, gdb-patches ml On Sat, Dec 7, 2013 at 9:13 PM, Hui Zhu <hui_zhu@mentor.com> wrote: > 2013-12-08 Hui Zhu <hui@codesourcery.com> > > PR gdb/16101 > * breakpoint.c (insert_bp_location): Change hw_bp_err_string to > bp_err_string. > > Output error message of software breakpoints. > Make solib error message output use same code with hardware > breakpoints and software breakpoints. > > * remote.c (remote_insert_breakpoint): If this breakpoint has > target-side commands but this stub doesn't support Z0 packets, > throw error. Nit. This changelog is badly formatted. Please fix. I suspect, though this is only based on perusal of the patch, and not close analysis, that you meant to write: PR gdb/16101 * breakpoint.c (insert_bp_location): Change hw_bp_err_string to bp_err_string. Output error message of software breakpoints. Make solib error message output use same code with hardware breakpoints and software breakpoints. * remote.c (remote_insert_breakpoint): If this breakpoint has target-side commands but this stub doesn't support Z0 packets, throw error. [I may have gotten the indentation wrong, cut-n-paste in gmail, blech. The point is the blank line before "Output error ..." is wrong. ISTM.] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-08 8:34 ` Doug Evans @ 2013-12-08 14:18 ` Hui Zhu 0 siblings, 0 replies; 27+ messages in thread From: Hui Zhu @ 2013-12-08 14:18 UTC (permalink / raw) To: Doug Evans; +Cc: Pedro Alves, gdb-patches ml On 12/08/13 16:34, Doug Evans wrote: > On Sat, Dec 7, 2013 at 9:13 PM, Hui Zhu <hui_zhu@mentor.com> wrote: >> 2013-12-08 Hui Zhu <hui@codesourcery.com> >> >> PR gdb/16101 >> * breakpoint.c (insert_bp_location): Change hw_bp_err_string to >> bp_err_string. >> >> Output error message of software breakpoints. >> Make solib error message output use same code with hardware >> breakpoints and software breakpoints. >> >> * remote.c (remote_insert_breakpoint): If this breakpoint has >> target-side commands but this stub doesn't support Z0 packets, >> throw error. > > Nit. This changelog is badly formatted. > Please fix. > > I suspect, though this is only based on perusal of the patch, > and not close analysis, that you meant to write: > > PR gdb/16101 > * breakpoint.c (insert_bp_location): Change hw_bp_err_string to > bp_err_string. Output error message of software breakpoints. > Make solib error message output use same code with hardware > breakpoints and software breakpoints. > > * remote.c (remote_insert_breakpoint): If this breakpoint has > target-side commands but this stub doesn't support Z0 packets, > throw error. > Thanks for your remind. 2013-12-08 Hui Zhu <hui@codesourcery.com> PR gdb/16101 * breakpoint.c (insert_bp_location): Change hw_bp_err_string to bp_err_string. Output error message of software breakpoints. Make solib error message output use same code with hardware breakpoints and software breakpoints. * remote.c (remote_insert_breakpoint): If this breakpoint has target-side commands but this stub doesn't support Z0 packets, throw error. > [I may have gotten the indentation wrong, cut-n-paste in gmail, blech. > The point is the blank line before "Output error ..." is wrong. ISTM.] > I got some indentation issue with web gmail in a few weeks ago. But this time it looks OK in my part. I think they has fixed the issue. Thanks, Hui ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-08 5:19 ` Hui Zhu 2013-12-08 8:34 ` Doug Evans @ 2013-12-09 19:48 ` Pedro Alves 2013-12-09 21:07 ` Doug Evans 1 sibling, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-12-09 19:48 UTC (permalink / raw) To: Hui Zhu; +Cc: gdb-patches ml On 12/08/2013 05:13 AM, Hui Zhu wrote: > On 12/07/13 03:29, Pedro Alves wrote: >> > Please analyze the insert_bp_location's error handling carefully: >> > >> > - if solib_name_from_address is true (the dprintf is set in a >> > shared library), we won't see this new error message, while >> > we should. > I change all this part to: > First, output error message. > Second, if it is solib breakpoint, disable breakpoint. But, the current code makes sure that we only see an error for a shared library breakpoint once (see disabled_breaks). After the patch, we'll see an error each time we'll try to insert such a breakpoint. We suppress errors when inserting/removing breakpoints in shared libraries because: /* In some cases, we might not be able to remove a breakpoint in a shared library that has already been removed, but we have not yet processed the shlib unload event. */ and it'd be annoying to see a bunch of errors whenever that happens. I think breakpoint.c needs to be able to distinguish what happened. We already need to handle exceptions thrown from with ops->insert_location / target_insert_foo, so we might as well go the exception way, and add a new error code. There's already something like means "error, unsupported": /* Feature is not supported in this copy of GDB. */ UNSUPPORTED_ERROR, but looking at what it's used for -- if sourcing a python script fails because Python was not configured into the gdb build -- it doesn't look like a good idea to reuse that error code as is: /* Load script FILE, which has already been opened as STREAM. */ static void source_script_from_stream (FILE *stream, const char *file) { if (script_ext_mode != script_ext_off && strlen (file) > 3 && !strcmp (&file[strlen (file) - 3], ".py")) { volatile struct gdb_exception e; TRY_CATCH (e, RETURN_MASK_ERROR) { source_python_script (stream, file); } if (e.reason < 0) { /* Should we fallback to ye olde GDB script mode? */ if (script_ext_mode == script_ext_soft && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) { fseek (stream, 0, SEEK_SET); script_from_file (stream, (char*) file); } else { /* Nope, just punt. */ throw_exception (e); } } } else script_from_file (stream, file); } If GDB does support python, and the sourced script happens to do something that triggers that error for some other reason, the above mistakes the error for Python not being supported. Actually, this looks fragile to me. We really can't reuse UNSUPPORTED_ERROR for anything else but "source_python_script is not implemented in this build". (Even in a multi-extension language world, if say, the Python script happens to run something in Scheme, and that raises a UNSUPPORTED_ERROR, we still wouldn't want the fallback code to trigger above.) So I though about renaming UNSUPPORTED_ERROR to something less generic, and then add a new error code (or repurpose the UNSUPPORTED_ERROR name for the new error). But, I don't see why we need to implement this source_python_script case with an exception/error code at all. We can just as well have source_python_script return a boolean indication. Then we're again free to repurpose UNSUPPORTED_ERROR. I'm testing the below. WDYT? --- gdb/breakpoint.c | 100 +++++++++++++++++++++++++++++++++++++--------------- gdb/cli/cli-cmds.c | 14 ++------ gdb/exceptions.h | 5 ++- gdb/python/python.c | 14 ++++---- gdb/python/python.h | 10 +++++- gdb/remote.c | 6 ++++ 6 files changed, 100 insertions(+), 49 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 111660f..c99d0ee 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2395,8 +2395,8 @@ insert_bp_location (struct bp_location *bl, int *hw_breakpoint_error, int *hw_bp_error_explained_already) { - int val = 0; - char *hw_bp_err_string = NULL; + enum errors bp_err = GDB_NO_ERROR; + char *bp_err_message = NULL; struct gdb_exception e; if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update)) @@ -2496,12 +2496,16 @@ insert_bp_location (struct bp_location *bl, /* No overlay handling: just set the breakpoint. */ TRY_CATCH (e, RETURN_MASK_ALL) { + int val; + val = bl->owner->ops->insert_location (bl); + if (val) + bp_err = GENERIC_ERROR; } if (e.reason < 0) { - val = 1; - hw_bp_err_string = (char *) e.message; + bp_err = e.error; + bp_err_message = (char *) e.message; } } else @@ -2523,9 +2527,24 @@ insert_bp_location (struct bp_location *bl, /* Set a software (trap) breakpoint at the LMA. */ bl->overlay_target_info = bl->target_info; bl->overlay_target_info.placed_address = addr; - val = target_insert_breakpoint (bl->gdbarch, - &bl->overlay_target_info); - if (val != 0) + + /* No overlay handling: just set the breakpoint. */ + TRY_CATCH (e, RETURN_MASK_ALL) + { + int val; + + val = target_insert_breakpoint (bl->gdbarch, + &bl->overlay_target_info); + if (val) + bp_err = GENERIC_ERROR; + } + if (e.reason < 0) + { + bp_err = e.error; + bp_err_message = (char *) e.message; + } + + if (bp_err != GDB_NO_ERROR) fprintf_unfiltered (tmp_error_stream, "Overlay breakpoint %d " "failed: in ROM?\n", @@ -2538,12 +2557,16 @@ insert_bp_location (struct bp_location *bl, /* Yes. This overlay section is mapped into memory. */ TRY_CATCH (e, RETURN_MASK_ALL) { + int val; + val = bl->owner->ops->insert_location (bl); + if (val) + bp_err = GENERIC_ERROR; } if (e.reason < 0) { - val = 1; - hw_bp_err_string = (char *) e.message; + bp_err = e.error; + bp_err_message = (char *) e.message; } } else @@ -2554,13 +2577,18 @@ insert_bp_location (struct bp_location *bl, } } - if (val) + if (bp_err != GDB_NO_ERROR) { /* Can't set the breakpoint. */ - if (solib_name_from_address (bl->pspace, bl->address)) + + /* In some cases, we might not be able to insert a + breakpoint in a shared library that has already been + removed, but we have not yet processed the shlib unload + event. */ + if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) + && solib_name_from_address (bl->pspace, bl->address)) { /* See also: disable_breakpoints_in_shlibs. */ - val = 0; bl->shlib_disabled = 1; observer_notify_breakpoint_modified (bl->owner); if (!*disabled_breaks) @@ -2575,39 +2603,51 @@ insert_bp_location (struct bp_location *bl, *disabled_breaks = 1; fprintf_unfiltered (tmp_error_stream, "breakpoint #%d\n", bl->owner->number); + return 0; } else { if (bl->loc_type == bp_loc_hardware_breakpoint) { - *hw_breakpoint_error = 1; - *hw_bp_error_explained_already = hw_bp_err_string != NULL; + *hw_breakpoint_error = 1; + *hw_bp_error_explained_already = bp_err_message != 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); + bl->owner->number, bp_err_message ? ":" : ".\n"); + if (bp_err_message != NULL) + fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_message); } else { - char *message = memory_error_message (TARGET_XFER_E_IO, - bl->gdbarch, bl->address); - struct cleanup *old_chain = make_cleanup (xfree, message); - - fprintf_unfiltered (tmp_error_stream, - "Cannot insert breakpoint %d.\n" - "%s\n", - bl->owner->number, message); - - do_cleanups (old_chain); + if (bp_err_message == NULL) + { + char *message + = memory_error_message (TARGET_XFER_E_IO, + bl->gdbarch, bl->address); + struct cleanup *old_chain = make_cleanup (xfree, message); + + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d.\n" + "%s\n", + bl->owner->number, message); + do_cleanups (old_chain); + } + else + { + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d:%s.\n", + bl->owner->number, + bp_err_message); + } } + return 1; } } else bl->inserted = 1; - return val; + return 0; } else if (bl->loc_type == bp_loc_hardware_watchpoint @@ -2615,6 +2655,8 @@ insert_bp_location (struct bp_location *bl, watchpoints. It's not clear that it's necessary... */ && bl->owner->disposition != disp_del_at_next_stop) { + int val; + gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert_location != NULL); @@ -2658,6 +2700,8 @@ insert_bp_location (struct bp_location *bl, else if (bl->owner->type == bp_catchpoint) { + int val; + gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert_location != NULL); diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 52a6bc9..ff988d2 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -527,24 +527,16 @@ source_script_from_stream (FILE *stream, const char *file) { volatile struct gdb_exception e; - TRY_CATCH (e, RETURN_MASK_ERROR) - { - source_python_script (stream, file); - } - if (e.reason < 0) + if (!source_python_script (stream, file)) { /* Should we fallback to ye olde GDB script mode? */ - if (script_ext_mode == script_ext_soft - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) + if (script_ext_mode == script_ext_soft) { fseek (stream, 0, SEEK_SET); script_from_file (stream, (char*) file); } else - { - /* Nope, just punt. */ - throw_exception (e); - } + error (_("Python scripting is not supported in this copy of GDB.")); } } else diff --git a/gdb/exceptions.h b/gdb/exceptions.h index 705f1a1..fd772b6 100644 --- a/gdb/exceptions.h +++ b/gdb/exceptions.h @@ -79,7 +79,7 @@ enum errors { /* Error accessing memory. */ MEMORY_ERROR, - /* Feature is not supported in this copy of GDB. */ + /* Requested feature, method, mechanism, etc. is not supported. */ UNSUPPORTED_ERROR, /* Value not available. E.g., a register was not collected in a @@ -100,6 +100,9 @@ enum errors { /* An undefined command was executed. */ UNDEFINED_COMMAND_ERROR, + /* Feature is not supported in this copy of GDB. */ + NOT_SUPPORTED_ERROR, + /* Add more errors here. */ NR_ERRORS }; diff --git a/gdb/python/python.c b/gdb/python/python.c index 1873936..6e8cbfa 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -764,12 +764,9 @@ gdbpy_find_pc_line (PyObject *self, PyObject *args) return result; } -/* Read a file as Python code. - FILE is the file to run. FILENAME is name of the file FILE. - This does not throw any errors. If an exception occurs python will print - the traceback and clear the error indicator. */ +/* See python.h. */ -void +int source_python_script (FILE *file, const char *filename) { struct cleanup *cleanup; @@ -777,6 +774,8 @@ source_python_script (FILE *file, const char *filename) cleanup = ensure_python_env (get_current_arch (), current_language); python_run_simple_file (file, filename); do_cleanups (cleanup); + + return 1; } \f @@ -1387,11 +1386,10 @@ eval_python_from_control_command (struct command_line *cmd) error (_("Python scripting is not supported in this copy of GDB.")); } -void +int source_python_script (FILE *file, const char *filename) { - throw_error (UNSUPPORTED_ERROR, - _("Python scripting is not supported in this copy of GDB.")); + return 0; } int diff --git a/gdb/python/python.h b/gdb/python/python.h index c07b2aa..2d37d2d 100644 --- a/gdb/python/python.h +++ b/gdb/python/python.h @@ -93,7 +93,15 @@ extern void finish_python_initialization (void); void eval_python_from_control_command (struct command_line *); -void source_python_script (FILE *file, const char *filename); +/* Read a file as Python code. + FILE is the file to run. FILENAME is name of the file FILE. + This does not throw any errors. If an exception occurs python will print + the traceback and clear the error indicator. + + Returns false if GDB was configured without Python support, + otherwise returns true. */ + +int source_python_script (FILE *file, const char *filename); int apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr, int embedded_offset, CORE_ADDR address, diff --git a/gdb/remote.c b/gdb/remote.c index 2ac8c36..453d06c 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8260,6 +8260,12 @@ remote_insert_breakpoint (struct gdbarch *gdbarch, } } + /* If this breakpoint has target-side commands but this stub doesn't + support Z0 packets, throw error. */ + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) + throw_error (UNSUPPORTED_ERROR, _("\ +Target doesn't support breakpoints that have target side commands.")); + return memory_insert_breakpoint (gdbarch, bp_tgt); } -- 1.7.11.7 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-09 19:48 ` Pedro Alves @ 2013-12-09 21:07 ` Doug Evans 2013-12-10 17:34 ` Pedro Alves ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Doug Evans @ 2013-12-09 21:07 UTC (permalink / raw) To: Pedro Alves; +Cc: Hui Zhu, gdb-patches ml Pedro Alves <palves@redhat.com> writes: > On 12/08/2013 05:13 AM, Hui Zhu wrote: >> On 12/07/13 03:29, Pedro Alves wrote: >>> > Please analyze the insert_bp_location's error handling carefully: >>> > >>> > - if solib_name_from_address is true (the dprintf is set in a >>> > shared library), we won't see this new error message, while >>> > we should. >> I change all this part to: >> First, output error message. >> Second, if it is solib breakpoint, disable breakpoint. > > But, the current code makes sure that we only see an error > for a shared library breakpoint once (see disabled_breaks). > After the patch, we'll see an error each time we'll try to > insert such a breakpoint. We suppress errors when > inserting/removing breakpoints in shared libraries because: > > /* In some cases, we might not be able to remove a breakpoint > in a shared library that has already been removed, but we > have not yet processed the shlib unload event. */ > > and it'd be annoying to see a bunch of errors whenever that happens. > > I think breakpoint.c needs to be able to distinguish what happened. > We already need to handle exceptions thrown from with > ops->insert_location / target_insert_foo, so we might as > well go the exception way, and add a new error code. > There's already something like means "error, unsupported": > > /* Feature is not supported in this copy of GDB. */ > UNSUPPORTED_ERROR, > > but looking at what it's used for -- if sourcing a python script > fails because Python was not configured into the gdb build -- > it doesn't look like a good idea to reuse that error code as is: > > /* Load script FILE, which has already been opened as STREAM. */ > > static void > source_script_from_stream (FILE *stream, const char *file) > { > if (script_ext_mode != script_ext_off > && strlen (file) > 3 && !strcmp (&file[strlen (file) - 3], ".py")) > { > volatile struct gdb_exception e; > > TRY_CATCH (e, RETURN_MASK_ERROR) > { > source_python_script (stream, file); > } > if (e.reason < 0) > { > /* Should we fallback to ye olde GDB script mode? */ > if (script_ext_mode == script_ext_soft > && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) > { > fseek (stream, 0, SEEK_SET); > script_from_file (stream, (char*) file); > } > else > { > /* Nope, just punt. */ > throw_exception (e); > } > } > } > else > script_from_file (stream, file); > } > > > If GDB does support python, and the sourced script happens to > do something that triggers that error for some other reason, > the above mistakes the error for Python not being supported. > Actually, this looks fragile to me. We really can't reuse > UNSUPPORTED_ERROR for anything else but "source_python_script > is not implemented in this build". > (Even in a multi-extension language world, if say, the Python > script happens to run something in Scheme, and that raises > a UNSUPPORTED_ERROR, we still wouldn't want the fallback code > to trigger above.) > > So I though about renaming UNSUPPORTED_ERROR to something > less generic, and then add a new error code (or repurpose > the UNSUPPORTED_ERROR name for the new error). But, > I don't see why we need to implement this source_python_script > case with an exception/error code at all. We can just as > well have source_python_script return a boolean indication. > Then we're again free to repurpose UNSUPPORTED_ERROR. > > I'm testing the below. WDYT? Hi. Various comments in line. > --- > gdb/breakpoint.c | 100 +++++++++++++++++++++++++++++++++++++--------------- > gdb/cli/cli-cmds.c | 14 ++------ > gdb/exceptions.h | 5 ++- > gdb/python/python.c | 14 ++++---- > gdb/python/python.h | 10 +++++- > gdb/remote.c | 6 ++++ > 6 files changed, 100 insertions(+), 49 deletions(-) > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 111660f..c99d0ee 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -2395,8 +2395,8 @@ insert_bp_location (struct bp_location *bl, > int *hw_breakpoint_error, > int *hw_bp_error_explained_already) > { > - int val = 0; > - char *hw_bp_err_string = NULL; > + enum errors bp_err = GDB_NO_ERROR; > + char *bp_err_message = NULL; > struct gdb_exception e; > > if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update)) > @@ -2496,12 +2496,16 @@ insert_bp_location (struct bp_location *bl, > /* No overlay handling: just set the breakpoint. */ > TRY_CATCH (e, RETURN_MASK_ALL) > { > + int val; > + > val = bl->owner->ops->insert_location (bl); > + if (val) > + bp_err = GENERIC_ERROR; > } > if (e.reason < 0) > { > - val = 1; > - hw_bp_err_string = (char *) e.message; > + bp_err = e.error; > + bp_err_message = (char *) e.message; Presumably there's a sufficient reason to keep them, but the question must be asked. :-) Are the casts necessary? [does bp_err_message have to be a char *] > } > } > else > @@ -2523,9 +2527,24 @@ insert_bp_location (struct bp_location *bl, > /* Set a software (trap) breakpoint at the LMA. */ > bl->overlay_target_info = bl->target_info; > bl->overlay_target_info.placed_address = addr; > - val = target_insert_breakpoint (bl->gdbarch, > - &bl->overlay_target_info); > - if (val != 0) > + > + /* No overlay handling: just set the breakpoint. */ > + TRY_CATCH (e, RETURN_MASK_ALL) > + { > + int val; > + > + val = target_insert_breakpoint (bl->gdbarch, > + &bl->overlay_target_info); > + if (val) > + bp_err = GENERIC_ERROR; > + } > + if (e.reason < 0) > + { > + bp_err = e.error; > + bp_err_message = (char *) e.message; > + } > + > + if (bp_err != GDB_NO_ERROR) > fprintf_unfiltered (tmp_error_stream, > "Overlay breakpoint %d " > "failed: in ROM?\n", > @@ -2538,12 +2557,16 @@ insert_bp_location (struct bp_location *bl, > /* Yes. This overlay section is mapped into memory. */ > TRY_CATCH (e, RETURN_MASK_ALL) > { > + int val; > + > val = bl->owner->ops->insert_location (bl); > + if (val) > + bp_err = GENERIC_ERROR; > } > if (e.reason < 0) > { > - val = 1; > - hw_bp_err_string = (char *) e.message; > + bp_err = e.error; > + bp_err_message = (char *) e.message; > } > } > else > @@ -2554,13 +2577,18 @@ insert_bp_location (struct bp_location *bl, > } > } > > - if (val) > + if (bp_err != GDB_NO_ERROR) > { > /* Can't set the breakpoint. */ > - if (solib_name_from_address (bl->pspace, bl->address)) > + > + /* In some cases, we might not be able to insert a > + breakpoint in a shared library that has already been > + removed, but we have not yet processed the shlib unload > + event. */ > + if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) It's not readily clear that the code will DTRT if a GENERIC_ERROR is thrown (instead of being assigned to bp_err manually). [are we introducing a fragility akin to source_python_script/UNSUPPORTED_ERROR - presumably not] A comment affirming this is ok would be welcome. > + && solib_name_from_address (bl->pspace, bl->address)) > { > /* See also: disable_breakpoints_in_shlibs. */ > - val = 0; > bl->shlib_disabled = 1; > observer_notify_breakpoint_modified (bl->owner); > if (!*disabled_breaks) > @@ -2575,39 +2603,51 @@ insert_bp_location (struct bp_location *bl, > *disabled_breaks = 1; > fprintf_unfiltered (tmp_error_stream, > "breakpoint #%d\n", bl->owner->number); > + return 0; > } > else > { > if (bl->loc_type == bp_loc_hardware_breakpoint) > { > - *hw_breakpoint_error = 1; > - *hw_bp_error_explained_already = hw_bp_err_string != NULL; > + *hw_breakpoint_error = 1; > + *hw_bp_error_explained_already = bp_err_message != 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); > + bl->owner->number, bp_err_message ? ":" : ".\n"); > + if (bp_err_message != NULL) > + fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_message); > } > else > { > - char *message = memory_error_message (TARGET_XFER_E_IO, > - bl->gdbarch, bl->address); > - struct cleanup *old_chain = make_cleanup (xfree, message); > - > - fprintf_unfiltered (tmp_error_stream, > - "Cannot insert breakpoint %d.\n" > - "%s\n", > - bl->owner->number, message); > - > - do_cleanups (old_chain); > + if (bp_err_message == NULL) > + { > + char *message > + = memory_error_message (TARGET_XFER_E_IO, > + bl->gdbarch, bl->address); > + struct cleanup *old_chain = make_cleanup (xfree, message); > + > + fprintf_unfiltered (tmp_error_stream, > + "Cannot insert breakpoint %d.\n" > + "%s\n", > + bl->owner->number, message); > + do_cleanups (old_chain); > + } > + else > + { > + fprintf_unfiltered (tmp_error_stream, > + "Cannot insert breakpoint %d:%s.\n", > + bl->owner->number, > + bp_err_message); > + } > } > + return 1; > > } > } > else > bl->inserted = 1; > > - return val; > + return 0; > } > > else if (bl->loc_type == bp_loc_hardware_watchpoint > @@ -2615,6 +2655,8 @@ insert_bp_location (struct bp_location *bl, > watchpoints. It's not clear that it's necessary... */ > && bl->owner->disposition != disp_del_at_next_stop) > { > + int val; > + > gdb_assert (bl->owner->ops != NULL > && bl->owner->ops->insert_location != NULL); > > @@ -2658,6 +2700,8 @@ insert_bp_location (struct bp_location *bl, > > else if (bl->owner->type == bp_catchpoint) > { > + int val; > + > gdb_assert (bl->owner->ops != NULL > && bl->owner->ops->insert_location != NULL); > > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index 52a6bc9..ff988d2 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -527,24 +527,16 @@ source_script_from_stream (FILE *stream, const char *file) > { > volatile struct gdb_exception e; > > - TRY_CATCH (e, RETURN_MASK_ERROR) > - { > - source_python_script (stream, file); > - } > - if (e.reason < 0) > + if (!source_python_script (stream, file)) If we must change things, I would prefer having a predicate and call that first. > { > /* Should we fallback to ye olde GDB script mode? */ > - if (script_ext_mode == script_ext_soft > - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) > + if (script_ext_mode == script_ext_soft) > { > fseek (stream, 0, SEEK_SET); > script_from_file (stream, (char*) file); > } > else > - { > - /* Nope, just punt. */ > - throw_exception (e); > - } > + error (_("Python scripting is not supported in this copy of GDB.")); > } > } > else > diff --git a/gdb/exceptions.h b/gdb/exceptions.h > index 705f1a1..fd772b6 100644 > --- a/gdb/exceptions.h > +++ b/gdb/exceptions.h > @@ -79,7 +79,7 @@ enum errors { > /* Error accessing memory. */ > MEMORY_ERROR, > > - /* Feature is not supported in this copy of GDB. */ > + /* Requested feature, method, mechanism, etc. is not supported. */ > UNSUPPORTED_ERROR, > > /* Value not available. E.g., a register was not collected in a > @@ -100,6 +100,9 @@ enum errors { > /* An undefined command was executed. */ > UNDEFINED_COMMAND_ERROR, > > + /* Feature is not supported in this copy of GDB. */ > + NOT_SUPPORTED_ERROR, > + Left over from an editing pass? [It's not used in the patch, and would be confusing with UNSUPPORTED_ERROR.] > /* Add more errors here. */ > NR_ERRORS > }; > diff --git a/gdb/python/python.c b/gdb/python/python.c > index 1873936..6e8cbfa 100644 > --- a/gdb/python/python.c > +++ b/gdb/python/python.c > @@ -764,12 +764,9 @@ gdbpy_find_pc_line (PyObject *self, PyObject *args) > return result; > } > > -/* Read a file as Python code. > - FILE is the file to run. FILENAME is name of the file FILE. > - This does not throw any errors. If an exception occurs python will print > - the traceback and clear the error indicator. */ > +/* See python.h. */ This is a change not related to the patch in question (moving the comment to python.h). This community can get massively nitpicky about such things. [It's fine with me. I just want to point that out -- more clarity would be nice.] > > -void > +int > source_python_script (FILE *file, const char *filename) > { > struct cleanup *cleanup; > @@ -777,6 +774,8 @@ source_python_script (FILE *file, const char *filename) > cleanup = ensure_python_env (get_current_arch (), current_language); > python_run_simple_file (file, filename); > do_cleanups (cleanup); > + > + return 1; > } > > \f > @@ -1387,11 +1386,10 @@ eval_python_from_control_command (struct command_line *cmd) > error (_("Python scripting is not supported in this copy of GDB.")); > } > > -void > +int > source_python_script (FILE *file, const char *filename) > { > - throw_error (UNSUPPORTED_ERROR, > - _("Python scripting is not supported in this copy of GDB.")); > + return 0; > } > > int > diff --git a/gdb/python/python.h b/gdb/python/python.h > index c07b2aa..2d37d2d 100644 > --- a/gdb/python/python.h > +++ b/gdb/python/python.h > @@ -93,7 +93,15 @@ extern void finish_python_initialization (void); > > void eval_python_from_control_command (struct command_line *); > > -void source_python_script (FILE *file, const char *filename); > +/* Read a file as Python code. > + FILE is the file to run. FILENAME is name of the file FILE. > + This does not throw any errors. If an exception occurs python will print > + the traceback and clear the error indicator. > + > + Returns false if GDB was configured without Python support, > + otherwise returns true. */ > + > +int source_python_script (FILE *file, const char *filename); > > int apply_val_pretty_printer (struct type *type, const gdb_byte *valaddr, > int embedded_offset, CORE_ADDR address, > diff --git a/gdb/remote.c b/gdb/remote.c > index 2ac8c36..453d06c 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -8260,6 +8260,12 @@ remote_insert_breakpoint (struct gdbarch *gdbarch, > } > } > > + /* If this breakpoint has target-side commands but this stub doesn't > + support Z0 packets, throw error. */ > + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) > + throw_error (UNSUPPORTED_ERROR, _("\ > +Target doesn't support breakpoints that have target side commands.")); > + > return memory_insert_breakpoint (gdbarch, bp_tgt); > } ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-09 21:07 ` Doug Evans @ 2013-12-10 17:34 ` Pedro Alves 2013-12-10 18:14 ` [PATCH] Eliminate UNSUPPORTED_ERROR Pedro Alves 2013-12-11 16:40 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Doug Evans 2013-12-12 10:55 ` breakpoint.c:insert_bp_location: Constify local. (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) Pedro Alves 2013-12-12 12:55 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves 2 siblings, 2 replies; 27+ messages in thread From: Pedro Alves @ 2013-12-10 17:34 UTC (permalink / raw) To: Doug Evans; +Cc: Hui Zhu, gdb-patches ml On 12/09/2013 09:07 PM, Doug Evans wrote: > Pedro Alves <palves@redhat.com> writes: >> > On 12/08/2013 05:13 AM, Hui Zhu wrote: >>> >> On 12/07/13 03:29, Pedro Alves wrote: >>>>> >>> > Please analyze the insert_bp_location's error handling carefully: >>>>> >>> > >>>>> >>> > - if solib_name_from_address is true (the dprintf is set in a >>>>> >>> > shared library), we won't see this new error message, while >>>>> >>> > we should. >>> >> I change all this part to: >>> >> First, output error message. >>> >> Second, if it is solib breakpoint, disable breakpoint. >> > >> > But, the current code makes sure that we only see an error >> > for a shared library breakpoint once (see disabled_breaks). >> > After the patch, we'll see an error each time we'll try to >> > insert such a breakpoint. We suppress errors when >> > inserting/removing breakpoints in shared libraries because: >> > >> > /* In some cases, we might not be able to remove a breakpoint >> > in a shared library that has already been removed, but we >> > have not yet processed the shlib unload event. */ >> > >> > and it'd be annoying to see a bunch of errors whenever that happens. >> > >> > I think breakpoint.c needs to be able to distinguish what happened. >> > We already need to handle exceptions thrown from with >> > ops->insert_location / target_insert_foo, so we might as >> > well go the exception way, and add a new error code. >> > There's already something like means "error, unsupported": >> > >> > /* Feature is not supported in this copy of GDB. */ >> > UNSUPPORTED_ERROR, >> > >> > but looking at what it's used for -- if sourcing a python script >> > fails because Python was not configured into the gdb build -- >> > it doesn't look like a good idea to reuse that error code as is: >> > >> > /* Load script FILE, which has already been opened as STREAM. */ >> > >> > static void >> > source_script_from_stream (FILE *stream, const char *file) >> > { >> > if (script_ext_mode != script_ext_off >> > && strlen (file) > 3 && !strcmp (&file[strlen (file) - 3], ".py")) >> > { >> > volatile struct gdb_exception e; >> > >> > TRY_CATCH (e, RETURN_MASK_ERROR) >> > { >> > source_python_script (stream, file); >> > } >> > if (e.reason < 0) >> > { >> > /* Should we fallback to ye olde GDB script mode? */ >> > if (script_ext_mode == script_ext_soft >> > && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) >> > { >> > fseek (stream, 0, SEEK_SET); >> > script_from_file (stream, (char*) file); >> > } >> > else >> > { >> > /* Nope, just punt. */ >> > throw_exception (e); >> > } >> > } >> > } >> > else >> > script_from_file (stream, file); >> > } >> > >> > >> > If GDB does support python, and the sourced script happens to >> > do something that triggers that error for some other reason, >> > the above mistakes the error for Python not being supported. >> > Actually, this looks fragile to me. We really can't reuse >> > UNSUPPORTED_ERROR for anything else but "source_python_script >> > is not implemented in this build". >> > (Even in a multi-extension language world, if say, the Python >> > script happens to run something in Scheme, and that raises >> > a UNSUPPORTED_ERROR, we still wouldn't want the fallback code >> > to trigger above.) >> > >> > So I though about renaming UNSUPPORTED_ERROR to something >> > less generic, and then add a new error code (or repurpose >> > the UNSUPPORTED_ERROR name for the new error). But, >> > I don't see why we need to implement this source_python_script >> > case with an exception/error code at all. We can just as >> > well have source_python_script return a boolean indication. >> > Then we're again free to repurpose UNSUPPORTED_ERROR. >> > >> > I'm testing the below. WDYT? > Hi. Various comments in line. > > >> > --- >> > gdb/breakpoint.c | 100 +++++++++++++++++++++++++++++++++++++--------------- >> > gdb/cli/cli-cmds.c | 14 ++------ >> > gdb/exceptions.h | 5 ++- >> > gdb/python/python.c | 14 ++++---- >> > gdb/python/python.h | 10 +++++- >> > gdb/remote.c | 6 ++++ >> > 6 files changed, 100 insertions(+), 49 deletions(-) >> > >> > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c >> > index 111660f..c99d0ee 100644 >> > --- a/gdb/breakpoint.c >> > +++ b/gdb/breakpoint.c >> > @@ -2395,8 +2395,8 @@ insert_bp_location (struct bp_location *bl, >> > int *hw_breakpoint_error, >> > int *hw_bp_error_explained_already) >> > { >> > - int val = 0; >> > - char *hw_bp_err_string = NULL; >> > + enum errors bp_err = GDB_NO_ERROR; >> > + char *bp_err_message = NULL; >> > struct gdb_exception e; >> > >> > if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update)) >> > @@ -2496,12 +2496,16 @@ insert_bp_location (struct bp_location *bl, >> > /* No overlay handling: just set the breakpoint. */ >> > TRY_CATCH (e, RETURN_MASK_ALL) >> > { >> > + int val; >> > + >> > val = bl->owner->ops->insert_location (bl); >> > + if (val) >> > + bp_err = GENERIC_ERROR; >> > } >> > if (e.reason < 0) >> > { >> > - val = 1; >> > - hw_bp_err_string = (char *) e.message; >> > + bp_err = e.error; >> > + bp_err_message = (char *) e.message; > Presumably there's a sufficient reason to keep them, > but the question must be asked. :-) > Are the casts necessary? > [does bp_err_message have to be a char *] > >> > } >> > } >> > else >> > @@ -2523,9 +2527,24 @@ insert_bp_location (struct bp_location *bl, >> > /* Set a software (trap) breakpoint at the LMA. */ >> > bl->overlay_target_info = bl->target_info; >> > bl->overlay_target_info.placed_address = addr; >> > - val = target_insert_breakpoint (bl->gdbarch, >> > - &bl->overlay_target_info); >> > - if (val != 0) >> > + >> > + /* No overlay handling: just set the breakpoint. */ >> > + TRY_CATCH (e, RETURN_MASK_ALL) >> > + { >> > + int val; >> > + >> > + val = target_insert_breakpoint (bl->gdbarch, >> > + &bl->overlay_target_info); >> > + if (val) >> > + bp_err = GENERIC_ERROR; >> > + } >> > + if (e.reason < 0) >> > + { >> > + bp_err = e.error; >> > + bp_err_message = (char *) e.message; >> > + } >> > + >> > + if (bp_err != GDB_NO_ERROR) >> > fprintf_unfiltered (tmp_error_stream, >> > "Overlay breakpoint %d " >> > "failed: in ROM?\n", >> > @@ -2538,12 +2557,16 @@ insert_bp_location (struct bp_location *bl, >> > /* Yes. This overlay section is mapped into memory. */ >> > TRY_CATCH (e, RETURN_MASK_ALL) >> > { >> > + int val; >> > + >> > val = bl->owner->ops->insert_location (bl); >> > + if (val) >> > + bp_err = GENERIC_ERROR; >> > } >> > if (e.reason < 0) >> > { >> > - val = 1; >> > - hw_bp_err_string = (char *) e.message; >> > + bp_err = e.error; >> > + bp_err_message = (char *) e.message; >> > } >> > } >> > else >> > @@ -2554,13 +2577,18 @@ insert_bp_location (struct bp_location *bl, >> > } >> > } >> > >> > - if (val) >> > + if (bp_err != GDB_NO_ERROR) >> > { >> > /* Can't set the breakpoint. */ >> > - if (solib_name_from_address (bl->pspace, bl->address)) >> > + >> > + /* In some cases, we might not be able to insert a >> > + breakpoint in a shared library that has already been >> > + removed, but we have not yet processed the shlib unload >> > + event. */ >> > + if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) > It's not readily clear that the code will DTRT if a GENERIC_ERROR > is thrown (instead of being assigned to bp_err manually). > [are we introducing a fragility akin to > source_python_script/UNSUPPORTED_ERROR - presumably not] > A comment affirming this is ok would be welcome. > >> > + && solib_name_from_address (bl->pspace, bl->address)) >> > { >> > /* See also: disable_breakpoints_in_shlibs. */ >> > - val = 0; >> > bl->shlib_disabled = 1; >> > observer_notify_breakpoint_modified (bl->owner); >> > if (!*disabled_breaks) >> > @@ -2575,39 +2603,51 @@ insert_bp_location (struct bp_location *bl, >> > *disabled_breaks = 1; >> > fprintf_unfiltered (tmp_error_stream, >> > "breakpoint #%d\n", bl->owner->number); >> > + return 0; >> > } >> > else >> > { >> > if (bl->loc_type == bp_loc_hardware_breakpoint) >> > { >> > - *hw_breakpoint_error = 1; >> > - *hw_bp_error_explained_already = hw_bp_err_string != NULL; >> > + *hw_breakpoint_error = 1; >> > + *hw_bp_error_explained_already = bp_err_message != 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); >> > + bl->owner->number, bp_err_message ? ":" : ".\n"); >> > + if (bp_err_message != NULL) >> > + fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_message); >> > } >> > else >> > { >> > - char *message = memory_error_message (TARGET_XFER_E_IO, >> > - bl->gdbarch, bl->address); >> > - struct cleanup *old_chain = make_cleanup (xfree, message); >> > - >> > - fprintf_unfiltered (tmp_error_stream, >> > - "Cannot insert breakpoint %d.\n" >> > - "%s\n", >> > - bl->owner->number, message); >> > - >> > - do_cleanups (old_chain); >> > + if (bp_err_message == NULL) >> > + { >> > + char *message >> > + = memory_error_message (TARGET_XFER_E_IO, >> > + bl->gdbarch, bl->address); >> > + struct cleanup *old_chain = make_cleanup (xfree, message); >> > + >> > + fprintf_unfiltered (tmp_error_stream, >> > + "Cannot insert breakpoint %d.\n" >> > + "%s\n", >> > + bl->owner->number, message); >> > + do_cleanups (old_chain); >> > + } >> > + else >> > + { >> > + fprintf_unfiltered (tmp_error_stream, >> > + "Cannot insert breakpoint %d:%s.\n", >> > + bl->owner->number, >> > + bp_err_message); >> > + } >> > } >> > + return 1; >> > >> > } >> > } >> > else >> > bl->inserted = 1; >> > >> > - return val; >> > + return 0; >> > } >> > >> > else if (bl->loc_type == bp_loc_hardware_watchpoint >> > @@ -2615,6 +2655,8 @@ insert_bp_location (struct bp_location *bl, >> > watchpoints. It's not clear that it's necessary... */ >> > && bl->owner->disposition != disp_del_at_next_stop) >> > { >> > + int val; >> > + >> > gdb_assert (bl->owner->ops != NULL >> > && bl->owner->ops->insert_location != NULL); >> > >> > @@ -2658,6 +2700,8 @@ insert_bp_location (struct bp_location *bl, >> > >> > else if (bl->owner->type == bp_catchpoint) >> > { >> > + int val; >> > + >> > gdb_assert (bl->owner->ops != NULL >> > && bl->owner->ops->insert_location != NULL); >> > >> > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c >> > index 52a6bc9..ff988d2 100644 >> > --- a/gdb/cli/cli-cmds.c >> > +++ b/gdb/cli/cli-cmds.c >> > @@ -527,24 +527,16 @@ source_script_from_stream (FILE *stream, const char *file) >> > { >> > volatile struct gdb_exception e; >> > >> > - TRY_CATCH (e, RETURN_MASK_ERROR) >> > - { >> > - source_python_script (stream, file); >> > - } >> > - if (e.reason < 0) >> > + if (!source_python_script (stream, file)) > If we must change things, I would prefer having a predicate > and call that first. I can try that. Does your script API series already have something like that? I'd guess it's probably touching this code too. > >> > { >> > /* Should we fallback to ye olde GDB script mode? */ >> > - if (script_ext_mode == script_ext_soft >> > - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) >> > + if (script_ext_mode == script_ext_soft) >> > { >> > fseek (stream, 0, SEEK_SET); >> > script_from_file (stream, (char*) file); >> > } >> > else >> > - { >> > - /* Nope, just punt. */ >> > - throw_exception (e); >> > - } >> > + error (_("Python scripting is not supported in this copy of GDB.")); >> > } >> > } >> > else >> > diff --git a/gdb/exceptions.h b/gdb/exceptions.h >> > index 705f1a1..fd772b6 100644 >> > --- a/gdb/exceptions.h >> > +++ b/gdb/exceptions.h >> > @@ -79,7 +79,7 @@ enum errors { >> > /* Error accessing memory. */ >> > MEMORY_ERROR, >> > >> > - /* Feature is not supported in this copy of GDB. */ >> > + /* Requested feature, method, mechanism, etc. is not supported. */ >> > UNSUPPORTED_ERROR, >> > >> > /* Value not available. E.g., a register was not collected in a >> > @@ -100,6 +100,9 @@ enum errors { >> > /* An undefined command was executed. */ >> > UNDEFINED_COMMAND_ERROR, >> > >> > + /* Feature is not supported in this copy of GDB. */ >> > + NOT_SUPPORTED_ERROR, >> > + > Left over from an editing pass? > [It's not used in the patch, and would be confusing with > UNSUPPORTED_ERROR.] Yeah. I wrote that before noticing UNSUPPORTED_ERROR, then forgot to remove. >> > diff --git a/gdb/python/python.c b/gdb/python/python.c >> > index 1873936..6e8cbfa 100644 >> > --- a/gdb/python/python.c >> > +++ b/gdb/python/python.c >> > @@ -764,12 +764,9 @@ gdbpy_find_pc_line (PyObject *self, PyObject *args) >> > return result; >> > } >> > >> > -/* Read a file as Python code. >> > - FILE is the file to run. FILENAME is name of the file FILE. >> > - This does not throw any errors. If an exception occurs python will print >> > - the traceback and clear the error indicator. */ >> > +/* See python.h. */ > This is a change not related to the patch in question > (moving the comment to python.h). Yeah, the previous post was just an RFC, I didn't mean to apply all of it as a single commit. In this case, there are two implementations of that function (the real one, and then the dummy one for when Python isn't configured in), but only of them is documented, and I needed to document the return code, which affects the dummy version too. Moving to the header sorted that out. BTW, I realize this is probably conflicting with your scripts API series. ISTR that removes the dummy functions anyway, right? In any case, I'll try the predicate way. -- Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] Eliminate UNSUPPORTED_ERROR. 2013-12-10 17:34 ` Pedro Alves @ 2013-12-10 18:14 ` Pedro Alves 2013-12-11 16:33 ` Doug Evans 2013-12-11 16:40 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Doug Evans 1 sibling, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-12-10 18:14 UTC (permalink / raw) Cc: Doug Evans, Hui Zhu, gdb-patches ml On 12/10/2013 05:34 PM, Pedro Alves wrote: > On 12/09/2013 09:07 PM, Doug Evans wrote: >>>> --- a/gdb/cli/cli-cmds.c >>>> +++ b/gdb/cli/cli-cmds.c >>>> @@ -527,24 +527,16 @@ source_script_from_stream (FILE *stream, const char *file) >>>> { >>>> volatile struct gdb_exception e; >>>> >>>> - TRY_CATCH (e, RETURN_MASK_ERROR) >>>> - { >>>> - source_python_script (stream, file); >>>> - } >>>> - if (e.reason < 0) >>>> + if (!source_python_script (stream, file)) >> If we must change things, I would prefer having a predicate >> and call that first. > > I can try that. OK? ---------- Subject: [PATCH] Eliminate UNSUPPORTED_ERROR. I have a case that could use an exception for "unsupported feature". I found UNSUPPORTED_ERROR, but looking deeper, I think as is, reusing it for other things would be fragile. E.g., if the Python script sourced by source_script_from_stream triggers any other missing functionality that would result in UNSUPPORTED_ERROR being propagated out to source_script_from_stream, that would confuse the error for Python not being built into GDB. This patch thus redoes things a little. Instead of using an exception for the "No Python" scenario, check whether Python is configured in before actually trying to source the file. It adds a new function instead of using #ifdef HAVE_PYTHON directly, as that is better at avoiding bitrot, as both Python and !Python paths are visible to the compiler this way. Tested on Fedora 17, with and without Python. gdb/ 2013-12-10 Pedro Alves <palves@redhat.com> * cli/cli-cmds.c (source_script_from_stream) Use have_python instead of catching UNSUPPORTED_ERROR. * exceptions.h (UNSUPPORTED_ERROR): Delete. * python/python.c (source_python_script) [!HAVE_PYTHON]: Internal error if called. * python/python.h (have_python): New static inline function. --- gdb/cli/cli-cmds.c | 27 ++++++++------------------- gdb/exceptions.h | 3 --- gdb/python/python.c | 5 +++-- gdb/python/python.h | 13 +++++++++++++ 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 52a6bc9..a0586ff 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -525,27 +525,16 @@ source_script_from_stream (FILE *stream, const char *file) if (script_ext_mode != script_ext_off && strlen (file) > 3 && !strcmp (&file[strlen (file) - 3], ".py")) { - volatile struct gdb_exception e; - - TRY_CATCH (e, RETURN_MASK_ERROR) + if (have_python ()) + source_python_script (stream, file); + else if (script_ext_mode == script_ext_soft) { - source_python_script (stream, file); - } - if (e.reason < 0) - { - /* Should we fallback to ye olde GDB script mode? */ - if (script_ext_mode == script_ext_soft - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) - { - fseek (stream, 0, SEEK_SET); - script_from_file (stream, (char*) file); - } - else - { - /* Nope, just punt. */ - throw_exception (e); - } + /* Fallback to GDB script mode. */ + fseek (stream, 0, SEEK_SET); + script_from_file (stream, (char*) file); } + else + error (_("Python scripting is not supported in this copy of GDB.")); } else script_from_file (stream, file); diff --git a/gdb/exceptions.h b/gdb/exceptions.h index 705f1a1..2cb8242 100644 --- a/gdb/exceptions.h +++ b/gdb/exceptions.h @@ -79,9 +79,6 @@ enum errors { /* Error accessing memory. */ MEMORY_ERROR, - /* Feature is not supported in this copy of GDB. */ - UNSUPPORTED_ERROR, - /* Value not available. E.g., a register was not collected in a traceframe. */ NOT_AVAILABLE_ERROR, diff --git a/gdb/python/python.c b/gdb/python/python.c index 1873936..55bb6cf 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -1390,8 +1390,9 @@ eval_python_from_control_command (struct command_line *cmd) void source_python_script (FILE *file, const char *filename) { - throw_error (UNSUPPORTED_ERROR, - _("Python scripting is not supported in this copy of GDB.")); + internal_error (__FILE__, __LINE__, + _("source_python_script called when Python scripting is " + "not supported.")); } int diff --git a/gdb/python/python.h b/gdb/python/python.h index c07b2aa..abbb581 100644 --- a/gdb/python/python.h +++ b/gdb/python/python.h @@ -89,6 +89,19 @@ typedef enum py_frame_args CLI_ALL_VALUES } py_frame_args; +/* Returns true if Python support is built into GDB, false + otherwise. */ + +static inline int +have_python (void) +{ +#ifdef HAVE_PYTHON + return 1; +#else + return 0; +#endif +} + extern void finish_python_initialization (void); void eval_python_from_control_command (struct command_line *); -- 1.7.11.7 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Eliminate UNSUPPORTED_ERROR. 2013-12-10 18:14 ` [PATCH] Eliminate UNSUPPORTED_ERROR Pedro Alves @ 2013-12-11 16:33 ` Doug Evans 2013-12-11 19:17 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Doug Evans @ 2013-12-11 16:33 UTC (permalink / raw) To: Pedro Alves; +Cc: Hui Zhu, gdb-patches ml On Tue, Dec 10, 2013 at 10:14 AM, Pedro Alves <palves@redhat.com> wrote: > On 12/10/2013 05:34 PM, Pedro Alves wrote: >> On 12/09/2013 09:07 PM, Doug Evans wrote: > >>>>> --- a/gdb/cli/cli-cmds.c >>>>> +++ b/gdb/cli/cli-cmds.c >>>>> @@ -527,24 +527,16 @@ source_script_from_stream (FILE *stream, const char *file) >>>>> { >>>>> volatile struct gdb_exception e; >>>>> >>>>> - TRY_CATCH (e, RETURN_MASK_ERROR) >>>>> - { >>>>> - source_python_script (stream, file); >>>>> - } >>>>> - if (e.reason < 0) >>>>> + if (!source_python_script (stream, file)) >>> If we must change things, I would prefer having a predicate >>> and call that first. >> >> I can try that. > > OK? A few nits, but yep. > > gdb/ > 2013-12-10 Pedro Alves <palves@redhat.com> > > * cli/cli-cmds.c (source_script_from_stream) Use have_python > instead of catching UNSUPPORTED_ERROR. > * exceptions.h (UNSUPPORTED_ERROR): Delete. > * python/python.c (source_python_script) [!HAVE_PYTHON]: Internal > error if called. > * python/python.h (have_python): New static inline function. > --- > gdb/cli/cli-cmds.c | 27 ++++++++------------------- > gdb/exceptions.h | 3 --- > gdb/python/python.c | 5 +++-- > gdb/python/python.h | 13 +++++++++++++ > 4 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index 52a6bc9..a0586ff 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -525,27 +525,16 @@ source_script_from_stream (FILE *stream, const char *file) > if (script_ext_mode != script_ext_off > && strlen (file) > 3 && !strcmp (&file[strlen (file) - 3], ".py")) > { > - volatile struct gdb_exception e; > - > - TRY_CATCH (e, RETURN_MASK_ERROR) > + if (have_python ()) > + source_python_script (stream, file); > + else if (script_ext_mode == script_ext_soft) > { > - source_python_script (stream, file); > - } > - if (e.reason < 0) > - { > - /* Should we fallback to ye olde GDB script mode? */ > - if (script_ext_mode == script_ext_soft > - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) > - { > - fseek (stream, 0, SEEK_SET); > - script_from_file (stream, (char*) file); > - } > - else > - { > - /* Nope, just punt. */ > - throw_exception (e); > - } > + /* Fallback to GDB script mode. */ > + fseek (stream, 0, SEEK_SET); > + script_from_file (stream, (char*) file); Remove the fseek and cast. [that'll probably require removing the { } too, sigh] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Eliminate UNSUPPORTED_ERROR. 2013-12-11 16:33 ` Doug Evans @ 2013-12-11 19:17 ` Pedro Alves 2013-12-12 4:23 ` Doug Evans 0 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-12-11 19:17 UTC (permalink / raw) To: Doug Evans; +Cc: Hui Zhu, gdb-patches ml On 12/11/2013 04:33 PM, Doug Evans wrote: >> > - /* Should we fallback to ye olde GDB script mode? */ >> > - if (script_ext_mode == script_ext_soft >> > - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) >> > - { >> > - fseek (stream, 0, SEEK_SET); >> > - script_from_file (stream, (char*) file); >> > - } >> > - else >> > - { >> > - /* Nope, just punt. */ >> > - throw_exception (e); >> > - } >> > + /* Fallback to GDB script mode. */ >> > + fseek (stream, 0, SEEK_SET); >> > + script_from_file (stream, (char*) file); > Remove the fseek and cast. Hmm, indeed the fseek doesn't look necessary. Will do. However, that makes me wonder whether I'm missing something, as it doesn't look necessary before my patch either. Was it ever really needed? I see you added it (and the unnecessary cast too ;-) ) in: https://www.sourceware.org/ml/gdb-patches/2010-04/msg00110.html but I can't tell why. -- Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Eliminate UNSUPPORTED_ERROR. 2013-12-11 19:17 ` Pedro Alves @ 2013-12-12 4:23 ` Doug Evans 2013-12-12 10:23 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Doug Evans @ 2013-12-12 4:23 UTC (permalink / raw) To: Pedro Alves; +Cc: Hui Zhu, gdb-patches ml On Wed, Dec 11, 2013 at 11:17 AM, Pedro Alves <palves@redhat.com> wrote: > On 12/11/2013 04:33 PM, Doug Evans wrote: >>> > - /* Should we fallback to ye olde GDB script mode? */ >>> > - if (script_ext_mode == script_ext_soft >>> > - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) >>> > - { >>> > - fseek (stream, 0, SEEK_SET); >>> > - script_from_file (stream, (char*) file); >>> > - } >>> > - else >>> > - { >>> > - /* Nope, just punt. */ >>> > - throw_exception (e); >>> > - } >>> > + /* Fallback to GDB script mode. */ >>> > + fseek (stream, 0, SEEK_SET); >>> > + script_from_file (stream, (char*) file); >> Remove the fseek and cast. > > Hmm, indeed the fseek doesn't look necessary. Will do. However, > that makes me wonder whether I'm missing something, as it doesn't > look necessary before my patch either. Was it ever really > needed? I think I added it to be extra conservative in case source_python_script happened to read from the file. [I know it normally wouldn't read and then throw an UNSUPPORTED_ERROR, but I'm guessing I didn't want this code to assume that.] Now that we're not even calling source_python_script the need is gone. I see you added it (and the unnecessary cast too ;-) ) in: > https://www.sourceware.org/ml/gdb-patches/2010-04/msg00110.html > but I can't tell why. script_from_file was changed to take a const char *arg the prior week. Presumably I just didn't update that part of the patch. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Eliminate UNSUPPORTED_ERROR. 2013-12-12 4:23 ` Doug Evans @ 2013-12-12 10:23 ` Pedro Alves 0 siblings, 0 replies; 27+ messages in thread From: Pedro Alves @ 2013-12-12 10:23 UTC (permalink / raw) To: Doug Evans; +Cc: Hui Zhu, gdb-patches ml On 12/12/2013 04:22 AM, Doug Evans wrote: > On Wed, Dec 11, 2013 at 11:17 AM, Pedro Alves <palves@redhat.com> wrote: >> On 12/11/2013 04:33 PM, Doug Evans wrote: >>>>> - /* Should we fallback to ye olde GDB script mode? */ >>>>> - if (script_ext_mode == script_ext_soft >>>>> - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) >>>>> - { >>>>> - fseek (stream, 0, SEEK_SET); >>>>> - script_from_file (stream, (char*) file); >>>>> - } >>>>> - else >>>>> - { >>>>> - /* Nope, just punt. */ >>>>> - throw_exception (e); >>>>> - } >>>>> + /* Fallback to GDB script mode. */ >>>>> + fseek (stream, 0, SEEK_SET); >>>>> + script_from_file (stream, (char*) file); >>> Remove the fseek and cast. >> >> Hmm, indeed the fseek doesn't look necessary. Will do. However, >> that makes me wonder whether I'm missing something, as it doesn't >> look necessary before my patch either. Was it ever really >> needed? > > I think I added it to be extra conservative in case > source_python_script happened to read from the file. > [I know it normally wouldn't read and then throw an UNSUPPORTED_ERROR, > but I'm guessing I didn't want this code to assume that.] > > Now that we're not even calling source_python_script the need is gone. > > I see you added it (and the unnecessary cast too ;-) ) in: >> https://www.sourceware.org/ml/gdb-patches/2010-04/msg00110.html >> but I can't tell why. > > script_from_file was changed to take a const char *arg the prior week. > Presumably I just didn't update that part of the patch. > Alright, pushed, as below. -------- Eliminate UNSUPPORTED_ERROR. I have a case that could use an exception for "unsupported feature". I found UNSUPPORTED_ERROR, but looking deeper, I think as is, reusing it for other things would be fragile. E.g., if the Python script sourced by source_script_from_stream triggers any other missing functionality that would result in UNSUPPORTED_ERROR being propagated out to source_script_from_stream, that would confuse the error for Python not being built into GDB. This patch thus redoes things a little. Instead of using an exception for the "No Python" scenario, check whether Python is configured in before actually trying to source the file. It adds a new function instead of using #ifdef HAVE_PYTHON directly, as that is better at avoiding bitrot, as both Python and !Python paths are visible to the compiler this way. Tested on Fedora 17, with and without Python. gdb/ 2013-12-12 Pedro Alves <palves@redhat.com> * cli/cli-cmds.c (source_script_from_stream) Use have_python instead of catching UNSUPPORTED_ERROR. * exceptions.h (UNSUPPORTED_ERROR): Delete. * python/python.c (source_python_script) [!HAVE_PYTHON]: Internal error if called. * python/python.h (have_python): New static inline function. --- gdb/ChangeLog | 9 +++++++++ gdb/cli/cli-cmds.c | 26 +++++++------------------- gdb/exceptions.h | 3 --- gdb/python/python.c | 5 +++-- gdb/python/python.h | 13 +++++++++++++ 5 files changed, 32 insertions(+), 24 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 40368b5..637a462 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2013-12-12 Pedro Alves <palves@redhat.com> + + * cli/cli-cmds.c (source_script_from_stream) Use have_python + instead of catching UNSUPPORTED_ERROR. + * exceptions.h (UNSUPPORTED_ERROR): Delete. + * python/python.c (source_python_script) [!HAVE_PYTHON]: Internal + error if called. + * python/python.h (have_python): New static inline function. + 2013-12-11 Doug Evans <dje@google.com> * dwarf2read.c (lookup_dwo_cutu): Include name of dwp file in diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c index 52a6bc9..73e03cf 100644 --- a/gdb/cli/cli-cmds.c +++ b/gdb/cli/cli-cmds.c @@ -525,27 +525,15 @@ source_script_from_stream (FILE *stream, const char *file) if (script_ext_mode != script_ext_off && strlen (file) > 3 && !strcmp (&file[strlen (file) - 3], ".py")) { - volatile struct gdb_exception e; - - TRY_CATCH (e, RETURN_MASK_ERROR) + if (have_python ()) + source_python_script (stream, file); + else if (script_ext_mode == script_ext_soft) { - source_python_script (stream, file); - } - if (e.reason < 0) - { - /* Should we fallback to ye olde GDB script mode? */ - if (script_ext_mode == script_ext_soft - && e.reason == RETURN_ERROR && e.error == UNSUPPORTED_ERROR) - { - fseek (stream, 0, SEEK_SET); - script_from_file (stream, (char*) file); - } - else - { - /* Nope, just punt. */ - throw_exception (e); - } + /* Fallback to GDB script mode. */ + script_from_file (stream, file); } + else + error (_("Python scripting is not supported in this copy of GDB.")); } else script_from_file (stream, file); diff --git a/gdb/exceptions.h b/gdb/exceptions.h index 705f1a1..2cb8242 100644 --- a/gdb/exceptions.h +++ b/gdb/exceptions.h @@ -79,9 +79,6 @@ enum errors { /* Error accessing memory. */ MEMORY_ERROR, - /* Feature is not supported in this copy of GDB. */ - UNSUPPORTED_ERROR, - /* Value not available. E.g., a register was not collected in a traceframe. */ NOT_AVAILABLE_ERROR, diff --git a/gdb/python/python.c b/gdb/python/python.c index 1873936..55bb6cf 100644 --- a/gdb/python/python.c +++ b/gdb/python/python.c @@ -1390,8 +1390,9 @@ eval_python_from_control_command (struct command_line *cmd) void source_python_script (FILE *file, const char *filename) { - throw_error (UNSUPPORTED_ERROR, - _("Python scripting is not supported in this copy of GDB.")); + internal_error (__FILE__, __LINE__, + _("source_python_script called when Python scripting is " + "not supported.")); } int diff --git a/gdb/python/python.h b/gdb/python/python.h index c07b2aa..abbb581 100644 --- a/gdb/python/python.h +++ b/gdb/python/python.h @@ -89,6 +89,19 @@ typedef enum py_frame_args CLI_ALL_VALUES } py_frame_args; +/* Returns true if Python support is built into GDB, false + otherwise. */ + +static inline int +have_python (void) +{ +#ifdef HAVE_PYTHON + return 1; +#else + return 0; +#endif +} + extern void finish_python_initialization (void); void eval_python_from_control_command (struct command_line *); -- 1.7.11.7 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-10 17:34 ` Pedro Alves 2013-12-10 18:14 ` [PATCH] Eliminate UNSUPPORTED_ERROR Pedro Alves @ 2013-12-11 16:40 ` Doug Evans 1 sibling, 0 replies; 27+ messages in thread From: Doug Evans @ 2013-12-11 16:40 UTC (permalink / raw) To: Pedro Alves; +Cc: Hui Zhu, gdb-patches ml On Tue, Dec 10, 2013 at 9:34 AM, Pedro Alves <palves@redhat.com> wrote: >> Doug wrote: >> If we must change things, I would prefer having a predicate >> and call that first. > > I can try that. Does your script API series already have > something like that? I'd guess it's probably touching this > code too. Hi. I read this one after the actual patch. No worries. > Yeah, the previous post was just an RFC, I didn't mean to apply > all of it as a single commit. In this case, there are two > implementations of that function (the real one, and then > the dummy one for when Python isn't configured in), but > only of them is documented, and I needed to document the > return code, which affects the dummy version too. Moving > to the header sorted that out. BTW, I realize this is > probably conflicting with your scripts API series. ISTR > that removes the dummy functions anyway, right? In any > case, I'll try the predicate way. Fortunately I anticipated the patch and included a change in my updated series. It'll need some tweaks if UNSUPPORTED_ERROR is removed but easy enough. ^ permalink raw reply [flat|nested] 27+ messages in thread
* breakpoint.c:insert_bp_location: Constify local. (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) 2013-12-09 21:07 ` Doug Evans 2013-12-10 17:34 ` Pedro Alves @ 2013-12-12 10:55 ` Pedro Alves 2013-12-12 12:55 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves 2 siblings, 0 replies; 27+ messages in thread From: Pedro Alves @ 2013-12-12 10:55 UTC (permalink / raw) To: Doug Evans; +Cc: Hui Zhu, gdb-patches ml On 12/09/2013 09:07 PM, Doug Evans wrote: >> > @@ -2496,12 +2496,16 @@ insert_bp_location (struct bp_location *bl, >> > /* No overlay handling: just set the breakpoint. */ >> > TRY_CATCH (e, RETURN_MASK_ALL) >> > { >> > + int val; >> > + >> > val = bl->owner->ops->insert_location (bl); >> > + if (val) >> > + bp_err = GENERIC_ERROR; >> > } >> > if (e.reason < 0) >> > { >> > - val = 1; >> > - hw_bp_err_string = (char *) e.message; >> > + bp_err = e.error; >> > + bp_err_message = (char *) e.message; > Presumably there's a sufficient reason to keep them, > but the question must be asked. :-) > Are the casts necessary? > [does bp_err_message have to be a char *] A bit of an unrelated change, but OK, I'll bite. ;-) Pushed. ---------- breakpoint.c:insert_bp_location: Constify local. gdb/ 2013-12-12 Pedro Alves <palves@redhat.com> * breakpoint.c (insert_bp_location): Make 'hw_bp_err_string' local const, and remove casts. --- gdb/ChangeLog | 5 +++++ gdb/breakpoint.c | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 637a462..c49895b 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,10 @@ 2013-12-12 Pedro Alves <palves@redhat.com> + * breakpoint.c (insert_bp_location): Make 'hw_bp_err_string' local + const, and remove casts. + +2013-12-12 Pedro Alves <palves@redhat.com> + * cli/cli-cmds.c (source_script_from_stream) Use have_python instead of catching UNSUPPORTED_ERROR. * exceptions.h (UNSUPPORTED_ERROR): Delete. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 111660f..589aa19 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2396,7 +2396,7 @@ insert_bp_location (struct bp_location *bl, int *hw_bp_error_explained_already) { int val = 0; - char *hw_bp_err_string = NULL; + const char *hw_bp_err_string = NULL; struct gdb_exception e; if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update)) @@ -2501,7 +2501,7 @@ insert_bp_location (struct bp_location *bl, if (e.reason < 0) { val = 1; - hw_bp_err_string = (char *) e.message; + hw_bp_err_string = e.message; } } else @@ -2543,7 +2543,7 @@ insert_bp_location (struct bp_location *bl, if (e.reason < 0) { val = 1; - hw_bp_err_string = (char *) e.message; + hw_bp_err_string = e.message; } } else -- 1.7.11.7 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-09 21:07 ` Doug Evans 2013-12-10 17:34 ` Pedro Alves 2013-12-12 10:55 ` breakpoint.c:insert_bp_location: Constify local. (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) Pedro Alves @ 2013-12-12 12:55 ` Pedro Alves 2014-01-09 18:36 ` Pedro Alves 2 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2013-12-12 12:55 UTC (permalink / raw) To: Doug Evans; +Cc: Hui Zhu, gdb-patches ml On 12/09/2013 09:07 PM, Doug Evans wrote: >> @@ -2554,13 +2577,18 @@ insert_bp_location (struct bp_location *bl, >> } >> } >> >> - if (val) >> + if (bp_err != GDB_NO_ERROR) >> { >> /* Can't set the breakpoint. */ >> - if (solib_name_from_address (bl->pspace, bl->address)) >> + >> + /* In some cases, we might not be able to insert a >> + breakpoint in a shared library that has already been >> + removed, but we have not yet processed the shlib unload >> + event. */ >> + if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) > > It's not readily clear that the code will DTRT if a GENERIC_ERROR > is thrown (instead of being assigned to bp_err manually). > [are we introducing a fragility akin to > source_python_script/UNSUPPORTED_ERROR - presumably not] > A comment affirming this is ok would be welcome. I've extended the comment. Updated patch below. ------------ Subject: [PATCH] Handle the case of a remote target supporting target side commands, but not on software breakpoints. Although we can tell upfront whether a remote target supports target side commands, we can only tell whether the target supports that in combination with a given breakpoint kind (software, hardware, watchpoints, etc.) when we go and try to insert such a breakpoint kind the first time. It's not desirable to make remote_insert_breakpoint simply return -1 in this case, because if the breakpoint was set in a shared library, insert_bp_location will assume that the breakpoint insertion failed because the library wasn't mapped in. insert_bp_location already handles errors/exceptions thrown from the target_insert_xxx methods, exactly so the backend can tell the user the detailed reason the insertion of hw breakpoints failed. But, in the case of software breakpoints, it discards the detailed error message. So the patch makes insert_bp_location use the error's message for SW breakpoints too, and, introduces a NOT_SUPPORTED_ERROR error code so that insert_bp_location doesn't confuse the error for failure due to a shared library disappearing. The result is: (gdb) c Warning: Cannot insert breakpoint 2: Target doesn't support breakpoints that have target side commands. 2013-12-12 Pedro Alves <palves@redhat.com> Hui Zhu <hui@codesourcery.com> PR gdb/16101 * breakpoint.c (insert_bp_location): Rename hw_bp_err_string to bp_err_string. Don't mark the location shlib_disabled if the error thrown wasn't a generic or memory error. Catch errors thrown while inserting breakpoints in overlayed code. Output error message of software breakpoints. * remote.c (remote_insert_breakpoint): If this breakpoint has target-side commands but this stub doesn't support Z0 packets, throw NOT_SUPPORTED_ERROR error. * exceptions.h (enum errors) <NOT_SUPPORTED_ERROR>: New error. * target.h (target_insert_breakpoint): Extend comment. (target_insert_hw_breakpoint): Add comment. --- gdb/breakpoint.c | 105 ++++++++++++++++++++++++++++++++++++++++--------------- gdb/exceptions.h | 3 ++ gdb/remote.c | 6 ++++ gdb/target.h | 11 ++++-- 4 files changed, 95 insertions(+), 30 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 589aa19..169fafa 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2395,8 +2395,8 @@ insert_bp_location (struct bp_location *bl, int *hw_breakpoint_error, int *hw_bp_error_explained_already) { - int val = 0; - const char *hw_bp_err_string = NULL; + enum errors bp_err = GDB_NO_ERROR; + const char *bp_err_message = NULL; struct gdb_exception e; if (!should_be_inserted (bl) || (bl->inserted && !bl->needs_update)) @@ -2496,12 +2496,16 @@ insert_bp_location (struct bp_location *bl, /* No overlay handling: just set the breakpoint. */ TRY_CATCH (e, RETURN_MASK_ALL) { + int val; + val = bl->owner->ops->insert_location (bl); + if (val) + bp_err = GENERIC_ERROR; } if (e.reason < 0) { - val = 1; - hw_bp_err_string = e.message; + bp_err = e.error; + bp_err_message = e.message; } } else @@ -2523,9 +2527,24 @@ insert_bp_location (struct bp_location *bl, /* Set a software (trap) breakpoint at the LMA. */ bl->overlay_target_info = bl->target_info; bl->overlay_target_info.placed_address = addr; - val = target_insert_breakpoint (bl->gdbarch, - &bl->overlay_target_info); - if (val != 0) + + /* No overlay handling: just set the breakpoint. */ + TRY_CATCH (e, RETURN_MASK_ALL) + { + int val; + + val = target_insert_breakpoint (bl->gdbarch, + &bl->overlay_target_info); + if (val) + bp_err = GENERIC_ERROR; + } + if (e.reason < 0) + { + bp_err = e.error; + bp_err_message = e.message; + } + + if (bp_err != GDB_NO_ERROR) fprintf_unfiltered (tmp_error_stream, "Overlay breakpoint %d " "failed: in ROM?\n", @@ -2538,12 +2557,16 @@ insert_bp_location (struct bp_location *bl, /* Yes. This overlay section is mapped into memory. */ TRY_CATCH (e, RETURN_MASK_ALL) { + int val; + val = bl->owner->ops->insert_location (bl); + if (val) + bp_err = GENERIC_ERROR; } if (e.reason < 0) { - val = 1; - hw_bp_err_string = e.message; + bp_err = e.error; + bp_err_message = e.message; } } else @@ -2554,13 +2577,23 @@ insert_bp_location (struct bp_location *bl, } } - if (val) + if (bp_err != GDB_NO_ERROR) { /* Can't set the breakpoint. */ - if (solib_name_from_address (bl->pspace, bl->address)) + + /* In some cases, we might not be able to insert a + breakpoint in a shared library that has already been + removed, but we have not yet processed the shlib unload + event. Unfortunately, some targets that implement + breakpoint insertion themselves (necessary if this is a + HW breakpoint, but SW breakpoints likewise) can't tell + why the breakpoint insertion failed (e.g., the remote + target doesn't define error codes), so we must treat + generic errors as memory errors. */ + if ((bp_err == GENERIC_ERROR || bp_err == MEMORY_ERROR) + && solib_name_from_address (bl->pspace, bl->address)) { /* See also: disable_breakpoints_in_shlibs. */ - val = 0; bl->shlib_disabled = 1; observer_notify_breakpoint_modified (bl->owner); if (!*disabled_breaks) @@ -2575,39 +2608,51 @@ insert_bp_location (struct bp_location *bl, *disabled_breaks = 1; fprintf_unfiltered (tmp_error_stream, "breakpoint #%d\n", bl->owner->number); + return 0; } else { if (bl->loc_type == bp_loc_hardware_breakpoint) { - *hw_breakpoint_error = 1; - *hw_bp_error_explained_already = hw_bp_err_string != NULL; + *hw_breakpoint_error = 1; + *hw_bp_error_explained_already = bp_err_message != 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); + bl->owner->number, bp_err_message ? ":" : ".\n"); + if (bp_err_message != NULL) + fprintf_unfiltered (tmp_error_stream, "%s.\n", bp_err_message); } else { - char *message = memory_error_message (TARGET_XFER_E_IO, - bl->gdbarch, bl->address); - struct cleanup *old_chain = make_cleanup (xfree, message); - - fprintf_unfiltered (tmp_error_stream, - "Cannot insert breakpoint %d.\n" - "%s\n", - bl->owner->number, message); - - do_cleanups (old_chain); + if (bp_err_message == NULL) + { + char *message + = memory_error_message (TARGET_XFER_E_IO, + bl->gdbarch, bl->address); + struct cleanup *old_chain = make_cleanup (xfree, message); + + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d.\n" + "%s\n", + bl->owner->number, message); + do_cleanups (old_chain); + } + else + { + fprintf_unfiltered (tmp_error_stream, + "Cannot insert breakpoint %d: %s\n", + bl->owner->number, + bp_err_message); + } } + return 1; } } else bl->inserted = 1; - return val; + return 0; } else if (bl->loc_type == bp_loc_hardware_watchpoint @@ -2615,6 +2660,8 @@ insert_bp_location (struct bp_location *bl, watchpoints. It's not clear that it's necessary... */ && bl->owner->disposition != disp_del_at_next_stop) { + int val; + gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert_location != NULL); @@ -2658,6 +2705,8 @@ insert_bp_location (struct bp_location *bl, else if (bl->owner->type == bp_catchpoint) { + int val; + gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert_location != NULL); diff --git a/gdb/exceptions.h b/gdb/exceptions.h index 2cb8242..bd1cce4 100644 --- a/gdb/exceptions.h +++ b/gdb/exceptions.h @@ -97,6 +97,9 @@ enum errors { /* An undefined command was executed. */ UNDEFINED_COMMAND_ERROR, + /* Requested feature, method, mechanism, etc. is not supported. */ + NOT_SUPPORTED_ERROR, + /* Add more errors here. */ NR_ERRORS }; diff --git a/gdb/remote.c b/gdb/remote.c index 2ac8c36..dce5e05 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8260,6 +8260,12 @@ remote_insert_breakpoint (struct gdbarch *gdbarch, } } + /* If this breakpoint has target-side commands but this stub doesn't + support Z0 packets, throw error. */ + if (!VEC_empty (agent_expr_p, bp_tgt->tcommands)) + throw_error (NOT_SUPPORTED_ERROR, _("\ +Target doesn't support breakpoints that have target side commands.")); + return memory_insert_breakpoint (gdbarch, bp_tgt); } diff --git a/gdb/target.h b/gdb/target.h index f22e5c6..d76f519 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1123,8 +1123,10 @@ int target_write_memory_blocks (VEC(memory_write_request_s) *requests, #define target_files_info() \ (*current_target.to_files_info) (¤t_target) -/* Insert a breakpoint at address BP_TGT->placed_address in the target - machine. Result is 0 for success, non-zero for error. */ +/* Insert a hardware breakpoint at address BP_TGT->placed_address in + the target machine. Returns 0 for success, and returns non-zero or + throws an error (with a detailed failure reason error code and + message) otherwise. */ extern int target_insert_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt); @@ -1553,6 +1555,11 @@ extern int target_insert_mask_watchpoint (CORE_ADDR, CORE_ADDR, int); extern int target_remove_mask_watchpoint (CORE_ADDR, CORE_ADDR, int); +/* Insert a hardware breakpoint at address BP_TGT->placed_address in + the target machine. Returns 0 for success, and returns non-zero or + throws an error (with a detailed failure reason error code and + message) otherwise. */ + #define target_insert_hw_breakpoint(gdbarch, bp_tgt) \ (*current_target.to_insert_hw_breakpoint) (gdbarch, bp_tgt) -- 1.7.11.7 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet 2013-12-12 12:55 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves @ 2014-01-09 18:36 ` Pedro Alves 0 siblings, 0 replies; 27+ messages in thread From: Pedro Alves @ 2014-01-09 18:36 UTC (permalink / raw) To: gdb-patches ml; +Cc: Doug Evans, Hui Zhu On 12/12/2013 12:55 PM, Pedro Alves wrote: > 2013-12-12 Pedro Alves <palves@redhat.com> > Hui Zhu <hui@codesourcery.com> > > PR gdb/16101 > * breakpoint.c (insert_bp_location): Rename hw_bp_err_string to > bp_err_string. Don't mark the location shlib_disabled if the > error thrown wasn't a generic or memory error. Catch errors > thrown while inserting breakpoints in overlayed code. Output > error message of software breakpoints. > * remote.c (remote_insert_breakpoint): If this breakpoint has > target-side commands but this stub doesn't support Z0 packets, > throw NOT_SUPPORTED_ERROR error. > * exceptions.h (enum errors) <NOT_SUPPORTED_ERROR>: New error. > * target.h (target_insert_breakpoint): Extend comment. > (target_insert_hw_breakpoint): Add comment. I've pushed this one in. -- Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2014-01-09 18:36 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-10-21 10:30 [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Hui Zhu 2013-10-21 15:37 ` Pedro Alves 2013-11-28 10:56 ` Hui Zhu 2013-11-28 17:38 ` Maciej W. Rozycki 2013-11-29 9:41 ` Hui Zhu 2013-11-29 15:27 ` [pushed] Plug target side conditions and commands leaks (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) Pedro Alves 2013-11-29 16:05 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves 2013-12-02 12:45 ` Hui Zhu 2013-12-02 14:38 ` Pedro Alves 2013-12-03 4:50 ` Hui Zhu 2013-12-03 4:54 ` Hui Zhu 2013-12-06 19:29 ` Pedro Alves 2013-12-08 5:19 ` Hui Zhu 2013-12-08 8:34 ` Doug Evans 2013-12-08 14:18 ` Hui Zhu 2013-12-09 19:48 ` Pedro Alves 2013-12-09 21:07 ` Doug Evans 2013-12-10 17:34 ` Pedro Alves 2013-12-10 18:14 ` [PATCH] Eliminate UNSUPPORTED_ERROR Pedro Alves 2013-12-11 16:33 ` Doug Evans 2013-12-11 19:17 ` Pedro Alves 2013-12-12 4:23 ` Doug Evans 2013-12-12 10:23 ` Pedro Alves 2013-12-11 16:40 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Doug Evans 2013-12-12 10:55 ` breakpoint.c:insert_bp_location: Constify local. (was: Re: [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet) Pedro Alves 2013-12-12 12:55 ` [PATCH] Let gdbserver doesn't tell GDB it support target-side breakpoint conditions and commands if it doesn't support 'Z' packet Pedro Alves 2014-01-09 18:36 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).