Re: [RFA]: pending breakpoint support [1/3]

Daniel Jacobowitz
2004-01-30T04:13:26Z

On Tue, Jan 27, 2004 at 03:41:04PM -0500, J. Johnston wrote:
> Anyway, I have a new and improved patch attached that
> addresses the issues below. I have included answers to questions below as
> well.

Thanks. I have just one question and a couple of cosmetic issues left.

I'm also still curious about pending breakpoints which resolve to
multiple functions, et cetera. We can revisit at a later date.

> >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.

OK. I have some ideas for better wording of the message, but I can
take care of it later.

> 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.

Gotcha.

About the new patch:

> + /* Flag value for pending breakpoint.
> + first bit : 0 non-temporary, 1 temporary.
> + second bit : 0 normal breakpoint, 1 hardware breakpoint. */
> + int flag;

OK, now I can see what flag is used for. This raises a new question.
Should temporary breakpoints be allowed to be pending?

Oh, I guess the answer is yes. tbreak my_dso_initialization; run. So
this is fine.

> +/* Try and resolve a pending breakpoint. */
> +static int
> +resolve_pending_breakpoint (struct breakpoint *b)

I love the new version of this function...

> + input_radix = b->input_radix;
> + rc = break_command_1 (b->addr_string, b->flag, b->from_tty, b);
> +
> + if (rc == GDB_RC_OK)
> + /* Pending breakpoint has been resolved. */
> + printf_filtered ("Resolves pending breakpoint \"%s\"\n", b->addr_string);

Spelling error, though.

> @@ -4779,6 +4888,26 @@ create_breakpoints (struct symtabs_and_l
> b->ignore_count = ignore_count;
> b->enable_state = bp_enabled;
> b->disposition = disposition;
> + /* If resolving a pending breakpoint, a check must be made to see if
> + the user has specified a new condition or commands for the
> + breakpoint. A new condition will override any condition that was
> + initially specified with the initial breakpoint command. */
> + if (pending_bp)
> + {
> + char *arg;
> + if (pending_bp->cond_string)
> + {
> + arg = pending_bp->cond_string;
> + b->cond_string = savestring (arg, strlen (arg));
> + b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
> + if (*arg)
> + error ("Junk at end of pending breakpoint condition expression");
> + }
> + /* If there are commands associated with the breakpoint, they should
> + be copied too. */
> + if (pending_bp->commands)
> + b->commands = copy_command_lines (pending_bp->commands);
> + }
> mention (b);
> }
> }

Here's the one question.

The only way to get here with a PENDING_BP is from break_command_1 from
resolve_pending_breakpoint. So I don't think it is possible for there
to be a condition already set on B, which makes the comment about
"overriding" such a condition a little strange. Am I right, or is
there some other way to get a condition to here?

> -static void
> -break_command_1 (char *arg, int flag, int from_tty)
> + PENDING_BP is non-NULL when this function is being called to resolve
> + a pending breakpoint. */
> +
> +static int
> +break_command_1 (char *arg, int flag, int from_tty, struct breakpoint *pending_bp)
> {
> int tempflag, hardwareflag;
> struct symtabs_and_lines sals;
> struct expression **cond = 0;
> + struct symtab_and_line pending_sal;
> /* Pointers in arg to the start, and one past the end, of the
> condition. */

Aha! That's where that baffling comment came from, it was already
present in break_command_1. Here we have a var named ARG, so it makes
more sense, but the variables cond_start and cond_end that it refers to
have walked down about a hundred lines.

> + 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 ();
> + if (resolve_pending_breakpoint (bpt) == GDB_RC_OK)

new_bp is no longer used, you can remove it.

--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer