This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [Patch] Cannot set pending bp if condition set explicitly
> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Thursday, July 26, 2012 3:05 PM
> To: Marc Khouzam
> Cc: 'Tom Tromey'; 'gdb-patches@sourceware.org'
> Subject: Re: [Patch] Cannot set pending bp if condition set explicitly
>
> Hello,
Thanks Pedro for having taken the time to dig into this.
> On 07/25/2012 09:30 PM, Marc Khouzam wrote:
> > Index: gdb/breakpoint.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/breakpoint.c,v
> > retrieving revision 1.695
> > diff -u -r1.695 breakpoint.c
> > --- gdb/breakpoint.c 24 Jul 2012 17:37:56 -0000 1.695
> > +++ gdb/breakpoint.c 25 Jul 2012 20:15:20 -0000
> > @@ -230,6 +230,8 @@
> >
> > static void tcatch_command (char *arg, int from_tty);
> >
> > +static int all_locations_are_pending (struct bp_location *loc);
> > +
> > static void detach_single_step_breakpoints (void);
> >
> > static int single_step_breakpoint_inserted_here_p (struct
> address_space *,
> > @@ -951,7 +953,12 @@
> > /* I don't know if it matters whether this is the
> string the user
> > typed in or the decompiled expression. */
> > b->cond_string = xstrdup (arg);
> > - b->condition_not_parsed = 0;
> > +
> > + /* For a pending breakpoint, the condition is not
> parsed yet. */
> > + if (all_locations_are_pending (b->loc))
> > + b->condition_not_parsed = 1;
> > + else
> > + b->condition_not_parsed = 0;
> >
> > if (is_watchpoint (b))
> > {
>
> It looks to me the sole reason of b->condition_not_parsed's
> existence, is that
> when the user does
>
> break foo 1 == 1 thread 3
>
> we need to split the "1 == 1" part from the "thread 3" part.
> The expression
> that results from this parse is just discarded.
>
> E.g., see:
>
> /* Here we only parse 'arg' to separate condition
> from thread number, so parsing in context of first
> sal is OK. When setting the breakpoint we'll
> re-parse it in context of each sal. */
>
> find_condition_and_thread (arg,
> lsal->sals.sals[0].pc, &cond_string,
> &thread, &task, &rest);
> if (cond_string)
> make_cleanup (xfree, cond_string);
> if (rest)
> make_cleanup (xfree, rest);
> if (rest)
> extra_string = rest;
>
>
> So "condition_not_parsed" is just a flag that means
> "find_condition_and_thread
> not called yet, so I don't know yet where my condition string ends".
Ok, I see that now.
> Unfortunately, the "1 == 1" part is language dependent, so it
> needs to be parsed
> with some context.
>
> Note that the result of such parsing is always discarded -
> the only thing
> find_condition_and_thread is interested in, is in advancing
> the command arg pointer
> past the condition. Later, each location's condition
> expression is always reparsed
> using each location as context, and that is what is stored in
> bloc->cond
> or in ((struct watchpoint *)b)->cond_exp, for watchpoints.
That makes me wonder why we need to jump
through hoops with having "condition_not_parsed"? Why not
always call find_condition_and_thread() inside of create_breakpoint()
even for pending breakpoints? That way, b->cond_string,
b->thread, and b->extra_string would always be set after
create_breakpoint() and we wouldn't need to treat pending breakpoints
differently in this case. It seems find_condition_and_thread() requires
an address, which would explain why it is not called for pending bp.
However, after reading your explanation, since find_condition_and_thread()
is only interested in advancing the command arg pointer past the condition,
why do we need the address at all? Should work fine for pending
breakpoints, no? Maybe it is just a requirement for parse_exp_1()
which is the one that actually parse the command argument?
> Therefore, it seems to be the patch has unexpected consequences.
>
> This currently works:
>
> (gdb) b main if 1 == 1 thread 1
> Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29.
>
> But this does not:
>
> (gdb) condition 2 1 == 1 thread 1
> Junk at end of expression
On a side-note, I see that although there is an error printed and
set_breakpoint_condition() is aborted, the bad condition is not
cleaned-up, as shown here:
(gdb) info b
Num Type Disp Enb Address What
1 breakpoint keep y 0x0804851d in main at ../../../src/gdb/testsuite/gdb.base/pending.c:26
stop only if k==1 thread 1
Shouldn't there be some kind of cleanup in set_breakpoint_condition()?
> However, for pending breakpoints, this will not produce an error:
>
> (gdb) condition 2 1 == 1 thread 1
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 2 breakpoint keep y <PENDING> foo
> stop only if 1 == 1 thread 1
>
> But later on, when the breakpoint's condition is finally
> parsed, the "thread 1" part will not really be used for anything.
> More on that below.
>
> > @@ -951,7 +953,12 @@
> > /* I don't know if it matters whether this is the
> string the user
> > typed in or the decompiled expression. */
> > b->cond_string = xstrdup (arg);
> > - b->condition_not_parsed = 0;
> > +
> > + /* For a pending breakpoint, the condition is not
> parsed yet. */
> > + if (all_locations_are_pending (b->loc))
> > + b->condition_not_parsed = 1;
> > + else
> > + b->condition_not_parsed = 0;
> >
>
> So in light of the above, this reads a bit odd. This function is only
> called from condition_command. The "condition" command only supports
> a real condition, not the "thread N" part. So the passed in EXP to
> set_condition_command already _is_ the condition, and we don't need to
> call find_condition_and_thread. Therefore, setting
> condition_not_parsed
> seems wrong. (if it were right, it would seem better to
> return immediately,
> rather than continue and try to parse the condition expression for
> each location, given we had just asserted we didn't know what
> the condition
> string was!).
Got it.
> The reason setting "condition_not_parsed" fixes the problem, is that
> when you "run", breakpoints end up being re_set, and that ends up
> with:
>
> Error in re-setting breakpoint 5: No source file named pendshr.c.
>
> seen here:
>
> #0 addr_string_to_sals (b=0xf9ecc0, addr_string=0xe7dbe0
> "pendshr.c:24", found=0x7fffffffd2ec) at
> ../../src/gdb/breakpoint.c:14002
> #1 0x0000000000555483 in breakpoint_re_set_default
> (b=0xf9ecc0) at ../../src/gdb/breakpoint.c:14076
> #2 0x000000000055306e in bkpt_re_set (b=0xf9ecc0) at
> ../../src/gdb/breakpoint.c:12812
> #3 0x000000000055577f in breakpoint_re_set_one
> (bint=0xf9ecc0) at ../../src/gdb/breakpoint.c:14193
> #4 0x00000000005ce097 in catch_errors (func=0x555747
> <breakpoint_re_set_one>, func_args=0xf9ecc0,
> errstring=0x111a190 "Error in re-setting breakpoint 7: ", mask=6)
> at ../../src/gdb/exceptions.c:546
> #5 0x0000000000555811 in breakpoint_re_set () at
> ../../src/gdb/breakpoint.c:14217
> #6 0x00000000006df7c1 in solib_add (pattern=0x0, from_tty=0,
> target=0xc42100, readsyms=1) at ../../src/gdb/solib.c:930
>
> static struct symtabs_and_lines
> addr_string_to_sals (struct breakpoint *b, char *addr_string,
> int *found)
> {
> char *s;
> struct symtabs_and_lines sals = {0};
> volatile struct gdb_exception e;
>
> gdb_assert (b->ops != NULL);
> s = addr_string;
>
> TRY_CATCH (e, RETURN_MASK_ERROR)
> {
> b->ops->decode_linespec (b, &s, &sals);
> }
> if (e.reason < 0)
> {
> int not_found_and_ok = 0;
> /* For pending breakpoints, it's expected that parsing will
> fail until the right shared library is loaded. User has
> already told to create pending breakpoints and don't need
> extra messages. If breakpoint is in bp_shlib_disabled
> state, then user already saw the message about that
> breakpoint being disabled, and don't want to see more
> errors. */
> if (e.error == NOT_FOUND_ERROR
> && (b->condition_not_parsed
> ^^^^^^^^^^^^^^^^^^^^^^^
> || (b->loc && b->loc->shlib_disabled)
> || (b->loc && b->loc->pspace->executing_startup)
> || b->enable_state == bp_disabled))
> not_found_and_ok = 1;
>
> And "b->condition_not_parsed" being set results in not_found_and_ok
> being set too.
>
> When finally decode_linespec does not throw an error, because the
> library is loaded in the inferior, and the breakpoint manages to
> become resolvable, we reach:
>
> if (e.reason == 0 || e.error != NOT_FOUND_ERROR)
> {
> int i;
>
> for (i = 0; i < sals.nelts; ++i)
> resolve_sal_pc (&sals.sals[i]);
> if (b->condition_not_parsed && s && s[0])
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> {
> char *cond_string, *extra_string;
> int thread, task;
>
> find_condition_and_thread (s, sals.sals[0].pc,
> &cond_string, &thread, &task,
> &extra_string);
>
>
> But s[0] is \0, because s decode_linespace consumes the whole
> addr_string,
> which was "pendshr.c:24".
Nice! Thanks for clearing that up.
> So find_condition_and_thread is
> never called,
> meaning the "thread N" never actually works, and,
> condition_not_parsed remains
> set forever. I am of the opinion that "condition BP thread
> N" should _not_
> work (reserve that for a separate command, we have enough
> trouble due to
> "thread/task" already), so "condition" for non-pending
> breakpoints should
> remain as it is, and for pending breakpoints, thread "1 == 1
> thread N" should
> be treated as a bogus condition, just like "ninja fu" is an invalid
> C condition.
Yes, that makes sense. In that case, set_breakpoint_condition()
would need to parse the condition even if the bp was pending.
This would fit well with needed cleanup of set_breakpoint_condition()
mentioned above.
Should I write a patch for this? (I'm leaving for vacation on Friday,
so this may not happen very soon.)
> I think create_breakpoint has a related latent bug. The contract is:
>
> /* (...) This function has two major
> modes of operations, selected by the PARSE_CONDITION_AND_THREAD
> parameter. If non-zero, the function will parse arg, extracting
> breakpoint location, address and thread. Otherwise, ARG is just
> the location of breakpoint, with condition and thread specified by
> the COND_STRING and THREAD parameters. (...) */
>
> But when a pending breakpoint is created, condition_not_parsed
> is left set, even when PARSE_CONDITION_AND_THREAD was false, thus
> ignoring the THREAD argument.
That is in part my fault. The handling of PARSE_CONDITION_AND_THREAD
was not done right for pending breakpoints. I tried to fix it here:
http://sourceware.org/ml/gdb-patches/2012-07/msg00418.html
but I didn't deal with condition_not_parsed because if I set
it to 0 in that case, MI started showing the bug that we're trying to
fix in this patch. But since you showed that setting
condition_not_parsed to 1 only solves the problem by a side-effect,
MI was only working by chance in this case.
Another side-note is that when a pending breakpoint is created,
b->extra_string doesn't seem set properly, just like b->cond_string
was not. I believe extra_string is for dprintf which cannot be set
using MI yet (I think), but once it will, this will become a problem.
> So given all that, I tried the alternate patch below. IOW,
> change how addr_string_to_sals knows a breakpoint is a
> pending breakpoint.
> It also fixes the new test, and causes no regressions (x86_64
> Fedora 17).
That makes total sense.
> Not sure yet what is the best predicate to put there.
> The whole condition looks a bit in need of TLC.
> all_locations_are_pending would
> return true if all locations had been shlib_disabled. The
> condition does
> also check for shlib_disabled, but only on the first
> location. Especially now
> that breakpoints can have locations anywhere, I think we
> might need to consider
> what happens and what should happen to when only a few of the
> conditions
> are shlib_disabled..
>
> WDYT? I'll try to give this predicate a bit more thought
> (but I'm running
> out of day for today), but these are my thoughts so far.
I don't have much of an opinion. I'm not yet grasping
all the details of breakpoints with multiple locations.
Maybe the breakpoint structure needs a pending flag?
> gdb/breakpoint.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 03719d4..ae21631 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -9558,7 +9558,10 @@ create_breakpoint (struct gdbarch *gdbarch,
>
> b->addr_string = copy_arg;
> if (parse_condition_and_thread)
> - b->cond_string = NULL;
> + {
> + b->cond_string = NULL;
> + b->condition_not_parsed = 1;
> + }
> else
> {
> /* Create a private copy of condition string. */
> @@ -9572,7 +9575,6 @@ create_breakpoint (struct gdbarch *gdbarch,
> b->extra_string = NULL;
> b->ignore_count = ignore_count;
> b->disposition = tempflag ? disp_del : disp_donttouch;
> - b->condition_not_parsed = 1;
> b->enable_state = enabled ? bp_enabled : bp_disabled;
> if ((type_wanted != bp_breakpoint
> && type_wanted != bp_hardware_breakpoint) || thread != -1)
> @@ -14008,7 +14010,7 @@ addr_string_to_sals (struct
> breakpoint *b, char *addr_string, int *found)
> breakpoint being disabled, and don't want to see more
> errors. */
> if (e.error == NOT_FOUND_ERROR
> - && (b->condition_not_parsed
> + && (b->loc == NULL
> || (b->loc && b->loc->shlib_disabled)
> || (b->loc && b->loc->pspace->executing_startup)
> || b->enable_state == bp_disabled))
This patch works well from the basic testing I did.
Please let me know how you suggest I can go forward with
the different issues discussed above about bp conditions.
Thanks again!
Marc