This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch] Cannot set pending bp if condition set explicitly


Hello,

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

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.

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

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

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

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.

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

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.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]