[RFA]: pending breakpoint support [1/3]
J. Johnston
jjohnstn@redhat.com
Tue Jan 27 20:41:00 GMT 2004
Daniel Jacobowitz wrote:
> On Wed, Jan 21, 2004 at 03:52:42PM -0500, Jeff Johnston wrote:
>
>>This is the first of the last 3 patches for pending breakpoint support.
>>This patch adds the actual support. The other two patches are the
>>documentation (previously approved, but before code was accepted) and
>>changes to the testsuite.
>>
>>This patch accounts for previous comments on the breakpoint code.
>>
>>Ok to commit?
>
>
> Sorry, but a number of my comments from last time still apply, and I
> have some others. The concept is looking great now, though!
>
Actually, I'm the one who's sorry. I worked on a number of these issues but
they were strewn about multiple copies. I never finished the multiple parsing
issue. Anyway, I have a new and improved patch attached that addresses the
issues below. I have included answers to questions below as well.
2004-01-26 Jeff Johnston <jjohnstn@redhat.com>
* breakpoint.h (struct breakpoint): Add new flag, from_tty,
and pending fields for pending breakpoint support.
* breakpoint.c (breakpoint_enabled): Add check for not pending.
(condition_command): Only parse condition if not a pending
breakpoint.
(print_one_breakpoint): Add support for pending breakpoints.
(describe_other_breakpoints): Add checks to verify we are not
dealing with pending breakpoints.
(check_duplicates): Don't check pending breakpoints.
(set_raw_breakpoint): Initialize pending flag.
(do_restore_lang_radix_cleanup): New cleanup routine.
(resolve_pending_breakpoint): New function.
(re_enable_breakpoints_in_shlibs): Try and resolve any
pending breakpoints via resolve_pending_breakpoint.
(mention): Add pending breakpoint support.
(parse_breakpoint_sals): Add new parameter to pass to
decode_line_1 to indicate silent errors when files or functions
are not found. Change all callers.
(do_captured_parse_breakpoint): New function.
(break_command_1): Change prototype to return an rc value and to
take an optional pending breakpoint pointer. Support creating
a pending breakpoint if a "not found" form of error occurs when
parsing the breakpoint. Also support resolving an existing pending
breakpoint and be silent if the resolution fails.
(create_breakpoints): Change prototype to take pending breakpoint
pointer. When resolving a pending breakpoint, use the new pointer
to provide a conditional or commands added by the end-user.
(delete_breakpoint): Add appropriate check for pending.
(breakpoint_re_set_one): Ditto.
(do_enable_breakpoint): Ditto.
>
>>Index: breakpoint.h
>>===================================================================
>>RCS file: /cvs/src/src/gdb/breakpoint.h,v
>>retrieving revision 1.26
>>diff -u -p -r1.26 breakpoint.h
>>--- breakpoint.h 6 Nov 2003 18:24:55 -0000 1.26
>>+++ breakpoint.h 21 Jan 2004 02:34:10 -0000
>>@@ -385,6 +385,15 @@ struct breakpoint
>>
>> /* Methods associated with this breakpoint. */
>> struct breakpoint_ops *ops;
>>+
>>+ /* Initial from_tty value. */
>>+ int from_tty;
>>+
>>+ /* Initial flag value. */
>>+ int flag;
>>+
>>+ /* Is breakpoint pending on shlib loads? */
>>+ int pending;
>
>
> Can you read this comment and figure out from it what flag is, or even
> where to look for more information? I can't. Please indicate where
> they come from, and that they are used for pending breakpoints.
> Something more descriptive than "flag" would be good.
>
Ok, done.
>
>>@@ -3458,7 +3463,15 @@ print_one_breakpoint (struct breakpoint
>> if (addressprint)
>> {
>> annotate_field (4);
>>- ui_out_field_core_addr (uiout, "addr", b->loc->address);
>>+ if (b->pending)
>>+ {
>>+ if (TARGET_ADDR_BIT <= 32)
>>+ ui_out_field_string (uiout, "addr", "<PENDING> ");
>>+ else
>>+ ui_out_field_string (uiout, "addr", "<PENDING> ");
>>+ }
>>+ else
>>+ ui_out_field_core_addr (uiout, "addr", b->loc->address);
>> }
>> annotate_field (5);
>> *last_addr = b->loc->address;
>
>
> Last time I said:
>
> I'm curious what effect this will have on MI consumers. Also, the
> resulting MI output will be somewhat gruesome with the embedded spaces.
> Maybe:
> ui_out_field_string (uiout, "addr", "<PENDING>");
> ui_out_text (uiout, "addr", " ");
> but I'm not sure that works without testing it.
>
> The other possible alternative is not outputting addr at all for
> MI-like uiouts. May be better to do this (more flexible).
>
> I still don't know what to do about the latter, but please try the
> former.
>
The answer appears to be ui_out_spaces() which under mi does nothing and under
cli does exactly what I want it to do.
>
>>@@ -4288,23 +4316,139 @@ disable_breakpoints_in_shlibs (int silen
>> }
>> }
>>
>>+struct captured_parse_breakpoint_args
>>+ {
>>+ char **arg_p;
>>+ struct symtabs_and_lines *sals_p;
>>+ char ***addr_string_p;
>>+ int *not_found_ptr;
>>+ };
>>+
>>+struct lang_and_radix
>>+ {
>>+ enum language lang;
>>+ int radix;
>>+ };
>>+
>>+/* Cleanup helper routine to restore the current language and
>>+ input radix. */
>>+static void
>>+do_restore_lang_radix_cleanup (void *old)
>>+{
>>+ struct lang_and_radix *p = old;
>>+ set_language (p->lang);
>>+ input_radix = p->radix;
>>+}
>>+
>>+/* Try and resolve a pending breakpoint. */
>>+static struct breakpoint *
>>+resolve_pending_breakpoint (struct breakpoint *b)
>>+{
>>+ /* Try and reparse the breakpoint in case the shared library
>>+ is now loaded. */
>>+ struct symtabs_and_lines sals;
>>+ struct symtab_and_line pending_sal;
>>+ /* Pointers in arg to the start, and one past the end, of the
>>+ condition. */
>
>
> Last time I read this patch I read this comment and said "Huh?" That
> still applies :) What pointers in what arg to the end of what
> condition?
>
I agree. Comment removed.
>
>>+ char **cond_string = (char **) NULL;
>>+ char *copy_arg = b->addr_string;
>>+ char **addr_string;
>>+ char *errmsg;
>>+ struct captured_parse_breakpoint_args parse_args;
>>+ int rc;
>>+ int not_found = 0;
>>+ struct ui_file *old_gdb_stderr;
>>+
>>+ sals.sals = NULL;
>>+ sals.nelts = 0;
>>+ addr_string = NULL;
>>+
>>+ parse_args.arg_p = ©_arg;
>>+ parse_args.sals_p = &sals;
>>+ parse_args.addr_string_p = &addr_string;
>>+ parse_args.not_found_ptr = ¬_found;
>>+
>>+ rc = catch_exceptions (uiout, do_captured_parse_breakpoint,
>>+ &parse_args, NULL, RETURN_MASK_ALL);
>>+
>>+ if (rc == GDB_RC_OK)
>>+ {
>>+ struct lang_and_radix old_lr;
>>+ struct cleanup *old_chain;
>>+ char *arg;
>>+ struct breakpoint *b1;
>>+
>>+ printf_filtered ("Pending breakpoint \"%s\" resolved\n", b->addr_string);
>>+
>>+ /* Set language, input-radix, then reissue breakpoint command.
>>+ Ensure the language and input-radix are restored afterwards. */
>>+ old_lr.lang = current_language->la_language;
>>+ old_lr.radix = input_radix;
>>+ old_chain = make_cleanup (do_restore_lang_radix_cleanup, &old_lr);
>
>
> Thanks for adding the cleanup.
>
>
>>+
>>+ set_language (b->language);
>>+ input_radix = b->input_radix;
>>+ break_command_1 (b->addr_string, b->flag, b->from_tty);
>>+ b1 = breakpoint_chain;
>>+ while (b1->next)
>>+ b1 = b1->next;
>
>
> I would really prefer that you not walk the breakpoint chain looking
> for the last breakpoint.
>
Fixed. I know pass the pending breakpoint along so I don't have to walk the
chain anymore.
>
>>+ /* If there is condition specified, it should be copied over. */
>>+ if (b->cond_string)
>>+ {
>>+ arg = b->cond_string;
>>+ b1->cond_string = savestring (arg, strlen (arg));
>>+ b1->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
>>+ if (*arg)
>>+ error ("Junk at end of expression");
>>+ }
>>+ /* If there are commands associated with the breakpoint, they should
>>+ be copied too. */
>>+ if (b->commands)
>>+ {
>>+ b1->commands = copy_command_lines (b->commands);
>>+ }
>
>
> No, I've already asked you to do entire section differently:
> This, on the other hand, is not OK. First of all you're wasting work -
> you parse the breakpoint location twice. Then you go grubbing around
> in the breakpoint chain looking for it, which assumes it will be added
> at the end of the chain - it will, but that sort of detail shouldn't be
> exposed. And break_command_1 can currently create multiple
> breakpoints, so you'll get that wrong too.
>
> decode_line_1 can end up prompting the user; you should have a testcase
> for that and make sure it does something sane. Easiest way is probably
> to put it in gdb.cp using an overloaded function. All set breakpoints
> should get the condition and commands.
>
> Can you arrange for most of this function to happen inside
> break_command_1, possibly by giving it a pending breakpoint as an
> optional argument? Other ideas?
>
> Also, I'm 99% sure you mean b1->loc->address above. b->loc->address is
> the pending breakpoint's address, which is unset.
>
You are correct about the loc->address. I have implemented your suggestion.
The break_command_1 routine now has a return code and takes an optional argument
for a pending breakpoint to resolve. It pass the pending breakpoint pointer to
the create_breakpoints code so that if multiple breakpoints are created,
operations can be done when the breakpoint is created rather than finding them
on the list. Anyway, the result is that only one parsing is performed and the
breakpoint list does not need to be traversed. The caller can also find out if
the breakpoint was successful without having to use a catch.
One minor change is needed to the pending.exp test because of the redesign.
Rather than stating the breakpoint is resolved and then showing the new
breakpoints, the issuing of the message has been moved to after confirmation of
the resolved breakpoints created. It has been slightly reworded as well.
>
>>+
>>+ do_cleanups (old_chain);
>>+ return b1; /* Pending breakpoint resolved. */
>>+ }
>>+
>>+ /* Otherwise, we didn't successfully resolve pending breakpoint. */
>>+ return NULL;
>>+}
>>+
>> /* Try to reenable any breakpoints in shared libraries. */
>> void
>> re_enable_breakpoints_in_shlibs (void)
>> {
>> struct breakpoint *b;
>>+ struct breakpoint *del_b = NULL;
>>
>> ALL_BREAKPOINTS (b)
>>+ {
>>+ if (del_b)
>>+ {
>>+ delete_breakpoint (del_b);
>>+ del_b = NULL;
>>+ }
>
>
> I said:
> No, use ALL_BREAKPOINTS_SAFE and you can get rid of del_b.
>
Agreed, done.
>
>>@@ -4923,19 +5094,51 @@ break_command_1 (char *arg, int flag, in
>> sals.sals = NULL;
>> sals.nelts = 0;
>> addr_string = NULL;
>>- parse_breakpoint_sals (&arg, &sals, &addr_string);
>>
>>- if (!sals.nelts)
>>+ parse_args.arg_p = &arg;
>>+ parse_args.sals_p = &sals;
>>+ parse_args.addr_string_p = &addr_string;
>>+ parse_args.not_found_ptr = ¬_found;
>>+
>>+ rc = catch_exceptions_with_msg (uiout, do_captured_parse_breakpoint,
>>+ &parse_args, NULL, &err_msg,
>>+ RETURN_MASK_ALL);
>
>
> Awesome, the need to play with the text of the error message is gone.
>
>
>>+
>>+ if (rc != GDB_RC_OK)
>>+ {
>>+ /* Check for file or function not found. */
>>+ if (not_found)
>>+ {
>>+ error_output_message (NULL, err_msg);
>>+ xfree (err_msg);
>>+ if (!query ("Make breakpoint pending on future shared library load? "))
>>+ return;
>
>
> I said:
> I'm still not really happy with this wording. What does that mean?
> Plus, it'll look really out of place if we don't have shared library
> support.
>
> When reading the testsuite diffs I asked:
> Might deferred be a better name for this concept anyway?
>
> Comments?
>
This is a case of "you can't please everybody all the time". I am certainly
happy with it. I wanted to distinguish it from "future-break". The two people
who are using it are happy with the terminology. One of them doesn't like the
word "deferred" - like I said, you can't please everybody. Anyway, we are
dealing with a whopping sampling of 4 people here. The breakpoint is "pending"
on a future event - that is certainly true. Yes, it could also be considered
"deferred" until a future event. There are multiple choices, I have made my
choice "pending". Like all unfamiliar terminology, people get used to it once
they know what it is. I am sure there are lots of such instances in gdb - for
example, "inferior".
Regarding shared libraries: I chose to spell it out at present because that is
the only way such a breakpoint gets resolved. I would imagine that on a
platform without shared libraries, a user wouldn't say "y" and if they did, it
certainly wouldn't affect them much as it would never occur. You have to
remember that this would only occur when the user enters a breakpoint location
that doesn't exist. I also mentioned perviously that a future option would be
added to make it so the query wouldn't be necessary. The people who use this
all the time always want it defaulted and I imagine those that don't have shared
libraries never want it on. The first step is to get the base functionality in.
>
>>+ copy_arg = (char *)xmalloc (strlen (addr_start));
>>+ strcpy (copy_arg, addr_start);
>
>
> I said:
> The cast is not necessary nowadays. There are also a number of
> functions for doing this, which don't suffer from the off-by-one error
> above :) Try xstrdup instead.
>
Ok.
>
>>+ addr_string = ©_arg;
>
>
> I said:
> Is addr_string guaranteed to still be NULL at this point, or are we
> leaking?
>
> The answer to this question is somewhere in the depths of
> decode_line_1, so I'm not sure what the answer is.
>
Well, it only gets there on a "not-found" error from decode_line_1 so the onus
is on decode_line_1 and it's children to do proper cleanups which is outside of
this patch.
>
>>+ sals.nelts = 1;
>>+ sals.sals = &pending_sal;
>>+ pending_sal.pc = 0;
>>+ pending = 1;
>>+ }
>>+ else
>>+ return;
>>+ }
>>+ else if (!sals.nelts)
>> return;
>>
>>+
>> /* Create a chain of things that always need to be cleaned up. */
>> old_chain = make_cleanup (null_cleanup, 0);
>>
>>- /* Make sure that all storage allocated to SALS gets freed. */
>>- make_cleanup (xfree, sals.sals);
>>-
>>- /* Cleanup the addr_string array but not its contents. */
>>- make_cleanup (xfree, addr_string);
>>+ if (!pending)
>>+ {
>>+ /* Make sure that all storage allocated to SALS gets freed. */
>>+ make_cleanup (xfree, sals.sals);
>>+
>>+ /* Cleanup the addr_string array but not its contents. */
>>+ make_cleanup (xfree, addr_string);
>>+ }
>>
>> /* Allocate space for all the cond expressions. */
>> cond = xcalloc (sals.nelts, sizeof (struct expression *));
>
>
> Yes, you don't want to free addr_string, but a few lines further down
> you probably want to add a cleanup to free copy_arg on the
> breakpoint_chain.
>
Added, thanks.
>
>>@@ -7363,70 +7600,92 @@ do_enable_breakpoint (struct breakpoint
>> error ("Hardware breakpoints used exceeds limit.");
>> }
>>
>>- if (bpt->enable_state != bp_permanent)
>>- bpt->enable_state = bp_enabled;
>>- bpt->disposition = disposition;
>>- check_duplicates (bpt);
>>- breakpoints_changed ();
>>-
>>- if (bpt->type == bp_watchpoint ||
>>- bpt->type == bp_hardware_watchpoint ||
>>- bpt->type == bp_read_watchpoint ||
>>- bpt->type == bp_access_watchpoint)
>>- {
>>- if (bpt->exp_valid_block != NULL)
>>- {
>>- struct frame_info *fr =
>>- fr = frame_find_by_id (bpt->watchpoint_frame);
>>- if (fr == NULL)
>>+ if (bpt->pending)
>>+ {
>>+ if (bpt->enable_state != bp_enabled)
>>+ {
>>+ /* When enabling a pending breakpoint, we need to check if the breakpoint
>>+ is resolvable since shared libraries could have been loaded
>>+ after the breakpoint was disabled. */
>>+ struct breakpoint *new_bp;
>>+ breakpoints_changed ();
>
>
> I said:
> Sure this is necessary? If we resolve the breakpoint successfully,
> this will get called from set_raw_breakpoint.
>
It is necessary because when we aren't successful, the breakpoint status will
still change from disabled to enabled but we won't have created a new breakpoint.
>
>>+ if ((new_bp = resolve_pending_breakpoint (bpt)) != NULL)
>
>
> I said:
> Might as well not use assignment-in-if-statement in new code.
>
> I think this is in the ARI.
>
Fixed.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pbreak.patch1b
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20040127/b5db3008/attachment.ksh>
More information about the Gdb-patches
mailing list