[Patch] Cannot set pending bp if condition set explicitly

[Patch] Cannot set pending bp if condition set explicitly

[Marc Khouzam]

Hi,

If I set a condition explicitly on a pending breakpoint (using
the 'condition' command), the breakpoint fails to install.

I believe it is because the condition is marked as parsed,
although, for a pending bp, this is not the case.
Note that using the form
  b mydll:c:5 if j==3
does work because the condition is not examined until the
pending breakpoint is installed.  This is not the case
when using the 'condition' command.
Broken session below.

No regressions on Ubuntu 32bit.

Is this ok for HEAD and 7_5?

Thanks

2012-07-20  Marc Khouzam  <marc.khouzam@ericsson.com>

	* breakpoint.c (set_breakpoint_condition): For pending
	breakpoints, mark condition as not parsed.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.694
diff -u -r1.694 breakpoint.c
--- gdb/breakpoint.c    19 Jul 2012 15:38:16 -0000      1.694
+++ gdb/breakpoint.c    23 Jul 2012 15:46:24 -0000
@@ -951,7 +951,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 breakoint, the condition is not parsed yet */
+      if (b->loc == NULL || b->loc->shlib_disabled)
+       b->condition_not_parsed = 1;
+      else  
+       b->condition_not_parsed = 0;
 
       if (is_watchpoint (b))
        {


> gdb myapp.exe
GNU gdb (GDB) 7.4.1
(gdb) b mydll.c:5
No source file named mydll.c.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (mydll.c:5) pending.
(gdb) cond 1 j==3
(gdb) info b
Num     Type           Disp Enb Address    What
1       breakpoint     keep y   <PENDING>  mydll.c:5
        stop only if j==3
(gdb) r
Starting program: /home/lmckhou/runtime-TestDSF/myapp/Debug/myapp.exe 
Error in re-setting breakpoint 1: No source file named /home/lmckhou/runtime-TestDSF/myLinuxDll/src/mydll.c.
5
warning: Temporarily disabling breakpoints for unloaded shared library "/home/lmckhou/runtime-TestDSF/myLinuxDll/Debug/libmyLinuxDll"
[Inferior 1 (process 3438) exited normally]

=> breakpoint was not installed and didn't hit

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

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> If I set a condition explicitly on a pending breakpoint (using
Marc> the 'condition' command), the breakpoint fails to install.

Marc> +      /* For a pending breakoint, the condition is not parsed yet */

Comment should end with a period and two spaces.

Marc> +      if (b->loc == NULL || b->loc->shlib_disabled)

I think this ought to use all_locations_are_pending.
But, I am not 100% sure of this.

A test case for this would be quite nice to have.

Tom

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

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Wednesday, July 25, 2012 11:28 AM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [Patch] Cannot set pending bp if condition set explicitly
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> If I set a condition explicitly on a pending breakpoint (using
> Marc> the 'condition' command), the breakpoint fails to install.
> 
> Marc> +      /* For a pending breakoint, the condition is not 
> parsed yet */
> 
> Comment should end with a period and two spaces.

Fixed, along with "breakoint" typo.

> Marc> +      if (b->loc == NULL || b->loc->shlib_disabled)
> 
> I think this ought to use all_locations_are_pending.
> But, I am not 100% sure of this.

I took the original line from print_one_exception_catchpoint()
when it figures out to print <PENDING>.

But I changed it to use all_locations_are_pending() in the below 
patch, and it also solves the problem I was seeing.

> A test case for this would be quite nice to have.

I modified gdb.base/pending.exp to test this case. The test fails
before the patch and passes after.  You'll note that I removed the 
two tests tha were disabling bp 7 and 5.  There were no bp 7 or 5 in
the original test, and my changes added bp 5 which needed to stay 
enabled.

How does it look for HEAD and 7_5 (not the tests
for 7_5 I gather)?

Thanks

Marc

2012-07-25  Marc Khouzam  <marc.khouzam@ericsson.com>

	* breakpoint.c: Add declaration of all_locations_are_pending.
	(set_breakpoint_condition): For pending breakpoints, mark 
	condition as not parsed.

2012-07-25  Marc Khouzam  <marc.khouzam@ericsson.com>

	* gdb.base/pendshr.c: Extra line to set a new breakpoint.
	* gdb.base/pending.exp: Set an enabled pending breakpoint
	with an explicit condition.


### Eclipse Workspace Patch 1.0
#P src
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))
        {
Index: gdb/testsuite/gdb.base/pending.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/pending.exp,v
retrieving revision 1.26
diff -u -r1.26 pending.exp
--- gdb/testsuite/gdb.base/pending.exp  21 Jun 2012 20:46:22 -0000      1.26
+++ gdb/testsuite/gdb.base/pending.exp  25 Jul 2012 20:15:20 -0000
@@ -209,6 +209,32 @@
 "multiple pending breakpoints 2"
 
 #
+# Try a pending break with an explicit condition which is enabled at startup
+#
+
+set bp5_loc [gdb_get_line_number "y++" ${libfile}.c]
+gdb_test_multiple "break pendshr.c:$bp5_loc" "Set pending breakpoint 4" {
+     -re ".*Make breakpoint pending.*y or \\\[n\\\]. $" {
+           gdb_test "y" "Breakpoint.*pendshr.c:$bp5_loc.*pending." \
+               "Set pending breakpoint 5"
+     }
+}
+
+gdb_test_no_output "condition 5 k == 1"
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.*PENDING.*pendfunc1.*
+\[\t \]+stop only if k == 1.*
+\[\t \]+print k.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp2_loc if x > 3.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp3_loc.*ignore next 2 hits.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp5_loc.*
+\[\t \]+stop only if k == 1.*" \
+"multiple pending breakpoints 3"
+
+#
 # Run to main which should resolve a pending breakpoint
 #
 
@@ -218,6 +244,24 @@
 "running to main"
 
 #
+# Verify that all pending breakpoints have resolved.
+#
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.* in pendfunc1 at .*
+\[\t \]+stop only if k == 1.*
+\[\t \]+print k.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline.*
+\[\t \]+breakpoint already hit 1 time.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in pendfunc1 at .*pendshr.c:$bp2_loc.*
+\[\t \]+stop only if x > 3.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in pendfunc1 at .*pendshr.c:$bp3_loc.*
+\[\t \]+ignore next 2 hits.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in pendfunc1 at .*pendshr.c:$bp5_loc.*
+\[\t \]+stop only if k == 1.*" \
+"multiple pending breakpoints 4"
+
+#
 # Re-enable the first pending breakpoint which should resolve
 #
 
@@ -237,19 +281,14 @@
 \[$\]1 = 1." \
 "continue to resolved breakpoint 1"
 
-#
-# Disable the other two breakpoints, and continue to the one with
-# the ignore count.  Make sure you hit it the third time, x should
-# be 3 then.
-#
-
-gdb_test "disable 7" "" "Disable other breakpoints"
-gdb_test "disable 5" "" "Disable other breakpoints"
-
 gdb_test "continue" \
         ".*Breakpoint.*pendfunc1.*\\\(x=3\\\) at.*pendshr.c:$bp3_loc.*printf.*;" \
 "continue to resolved breakpoint 3"
 
+gdb_test "continue" \
+        ".*Breakpoint.*pendfunc1.*\\\(x=3\\\) at.*pendshr.c:$bp5_loc.*y\\+\\+;" \
+"continue to resolved breakpoint 5"
+
 delete_breakpoints
 
 gdb_breakpoint "main"
Index: gdb/testsuite/gdb.base/pendshr.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/pendshr.c,v
retrieving revision 1.10
diff -u -r1.10 pendshr.c
--- gdb/testsuite/gdb.base/pendshr.c    4 Jan 2012 08:17:46 -0000       1.10
+++ gdb/testsuite/gdb.base/pendshr.c    25 Jul 2012 20:15:20 -0000
@@ -21,6 +21,7 @@
 {
   int y = x + 4;
   printf ("in pendfunc1, x is %d\n", x);
+  y++;
 }
 
 void pendfunc (int x)

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

[Tom Tromey]

Marc> I took the original line from print_one_exception_catchpoint()
Marc> when it figures out to print <PENDING>.

Yeah, I'm just not sure that this test is definitive in all cases.

Marc> But I changed it to use all_locations_are_pending() in the below 
Marc> patch, and it also solves the problem I was seeing.

I'd like Pedro's input on this patch.  I CC'd him.

Marc> How does it look for HEAD and 7_5 (not the tests
Marc> for 7_5 I gather)?

I think it is fine to put a new test onto the release branch.
The worst that can happen is a new FAIL -- but it isn't like the test
case is 0-FAIL in normal circumstances anyway.

Tom

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

[Pedro Alves]

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

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

[Marc Khouzam]

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

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

[Pedro Alves]

On 07/27/2012 03:44 AM, Marc Khouzam wrote:

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

Yes, we parse the condition in the context of the breakpoint location,
for locals, etc., so we need the location's address.  We assume the condition
would parse more or less the same at any location, as in, gross validation,
and skipping past the condition, so we just pick the first location).

An alternative would be to restrict pending breakpoints conditions
to globals only.  Not sure whether that would fly.

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

Indeed.  Hmm, that, or disable the location, with a warning.  We parse the condition in the
context of each location.  A condition might well make sense and parse okay
for a location, but not for another (e.g., different locals/parameters).  Disabling
is also what we do if we fail to parse a location later on with "break foo if bar".

> 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've already got something in the works.  Let me see if I can finish it.
And I'm leaving for vacation at the end of the week too.  :-)

-- 
Pedro Alves

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

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> An alternative would be to restrict pending breakpoints conditions
Pedro> to globals only.  Not sure whether that would fly.

Yeah, I'd rather not.

For example, it would mean that a breakpoint that examines function
arguments would work fine if the function was in the main program; but
if you refactored the code to move it to a shared library, it would not
work.

Also it seems to me that you'd have to add extra semantics to decide
what to do in the re-run case, where a conditional breakpoint is set in
a .so that is loaded at some point during the run.  Here, won't the
breakpoint convert itself to pending, and then back when the .so is
loaded?  Would this then mean that the expression is interpreted
differently?

Tom

disable breakpoints with invalid condition (Re: [Patch] Cannot set pending bp if condition set explicitly)

[Pedro Alves]

On 07/30/2012 04:18 PM, Pedro Alves wrote:
>>> >> 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()?
> Indeed.  Hmm, that, or disable the location, with a warning.  We parse the condition in the
> context of each location.  A condition might well make sense and parse okay
> for a location, but not for another (e.g., different locals/parameters).  Disabling
> is also what we do if we fail to parse a location later on with "break foo if bar".
> 

Here's a patch implementing this idea.  Examples (currently, the condition text
is left stale, and the breakpoint becomes uncondition):

(gdb) b main
Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29.
(gdb) condition 2 foo bar
warning: Breakpoint 2.1 disabled: No symbol "foo" in current context.
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   <MULTIPLE>
        stop only if foo bar
2.1                         n     0x00000000004572bb in main at ../../src/gdb/gdb.c:29
(gdb) condition 2
Breakpoint 2 now unconditional.
(gdb) enable 2.1
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29

(gdb) watch args
Hardware watchpoint 3: args
(gdb) condition 3
Breakpoint 3 now unconditional.
(gdb) condition 3 foo bar
warning: Watchpoint 3 disabled: No symbol "foo" in current context.
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29
3       hw watchpoint  keep n                      args
        stop only if foo bar
(gdb) condition 3 1 == 1 thread 1
warning: Watchpoint 3 disabled - junk at end of expression: thread 1
(gdb)

There are several little details we get wrong that I haven't addressed.
For instance, in this case, the re-enabling of a watchpoint is prevented:

(gdb) condition 3 asdf
warning: Watchpoint 3 disabled: No symbol "asdf" in current context.
(gdb) info breakpoints 3
Num     Type           Disp Enb Address            What
3       hw watchpoint  keep n                      args
        stop only if asdf
        breakpoint already hit 1 time
(gdb) enable 3
Cannot enable watchpoint 3: No symbol "asdf" in current context.

While this doesn't:

(gdb) condition 3 1 == 1 thread 1
warning: Watchpoint 3 disabled - junk at end of expression: thread 1
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29
3       hw watchpoint  keep n                      args
        stop only if 1 == 1 thread 1
        breakpoint already hit 1 time
(gdb) enable 3
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
2       breakpoint     keep y   0x00000000004572bb in main at ../../src/gdb/gdb.c:29
3       hw watchpoint  keep y                      args
        stop only if 1 == 1 thread 1
        breakpoint already hit 1 time
(gdb)

For regular breakpoints, you can always "enable" them, but when you let
the program run, you'll find they are unconditional, and if something causes
a re-set, they get disabled again:

warning: failed to reevaluate condition for breakpoint 4: blah blah.


I'd appreciate hearing opinions on whether this change is a good direction or not.

macscp.exp had a case of an invalid condition being set on a breakpoint, and then
the test assuming the breakpoint would be hit.

2012-08-02  Pedro Alves  <palves@redhat.com>

	gdb/
	* breakpoint.c (set_breakpoint_condition): If condition fails to
	parse, disable the watchpoint or breakpoint location, and warn.

	gdb/doc/
	* gdb.texinfo (condition command): Mention that locations are
	disabled when the condition has a problem.

	gdb/testsuite/
	* gdb.base/condbreak.exp: New tests for invalid conditions with
	"condition" command.
	* gdb.base/macscp.exp: After setting an invalid condition to a
	breakpoint, make the breakpoint unconditional and re-enable it.

---

 gdb/testsuite/gdb.base/condbreak.exp |    9 +++++++++
 gdb/testsuite/gdb.base/macscp.exp    |    7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 03719d4..b8484a7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -956,26 +956,59 @@ set_breakpoint_condition (struct breakpoint *b, char *exp,
       if (is_watchpoint (b))
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
+	  volatile struct gdb_exception e;

 	  innermost_block = NULL;
 	  arg = exp;
-	  w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
-	  if (*arg)
-	    error (_("Junk at end of expression"));
-	  w->cond_exp_valid_block = innermost_block;
+	  TRY_CATCH (e, RETURN_MASK_ERROR)
+	    {
+	      w->cond_exp = parse_exp_1 (&arg, 0, 0, 0);
+	    }
+	  if (e.reason < 0)
+	    {
+	      b->enable_state = bp_disabled;
+	      warning (_("Watchpoint %d disabled: %s"),
+		     b->number, e.message);
+	    }
+	  else if (*arg)
+	    {
+	      b->enable_state = bp_disabled;
+	      warning (_("Watchpoint %d disabled - "
+		       "junk at end of expression: %s"),
+		     b->number, arg);
+	    }
+	  else
+	    w->cond_exp_valid_block = innermost_block;
 	}
       else
 	{
 	  struct bp_location *loc;
+	  int loc_n;

-	  for (loc = b->loc; loc; loc = loc->next)
+	  for (loc = b->loc, loc_n = 1; loc; loc = loc->next, loc_n++)
 	    {
+	      volatile struct gdb_exception e;
+
 	      arg = exp;
-	      loc->cond =
-		parse_exp_1 (&arg, loc->address,
-			     block_for_pc (loc->address), 0);
-	      if (*arg)
-		error (_("Junk at end of expression"));
+
+	      TRY_CATCH (e, RETURN_MASK_ERROR)
+		{
+		  loc->cond = parse_exp_1 (&arg, loc->address,
+					   block_for_pc (loc->address), 0);
+		}
+	      if (e.reason < 0)
+		{
+		  loc->enabled = 0;
+		  warning (_("Breakpoint %d.%d disabled: %s"),
+			   b->number, loc_n, e.message);
+		}
+	      else if (*arg)
+		{
+		  loc->enabled = 0;
+		  warning (_("Breakpoint %d.%d disabled - "
+			     "junk at end of expression: %s"),
+			   b->number, loc_n, arg);
+		}
 	    }
 	}
     }
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index facca8f..8518d2f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -4487,11 +4487,12 @@ breakpoint @var{bnum} stops your program only if the value of
 @code{condition}, @value{GDBN} checks @var{expression} immediately for
 syntactic correctness, and to determine whether symbols in it have
 referents in the context of your breakpoint.  If @var{expression} uses
-symbols not referenced in the context of the breakpoint, @value{GDBN}
-prints an error message:
+symbols not referenced in the context any of the breakpoint's
+locations, @value{GDBN} prints a warning message and disables the
+corresponding location:

 @smallexample
-No symbol "foo" in current context.
+warning: Breakpoint 1.1 disabled: No symbol "foo" in current context.
 @end smallexample

 @noindent
diff --git a/gdb/testsuite/gdb.base/condbreak.exp b/gdb/testsuite/gdb.base/condbreak.exp
index e8e0d84..9d91fab 100644
--- a/gdb/testsuite/gdb.base/condbreak.exp
+++ b/gdb/testsuite/gdb.base/condbreak.exp
@@ -266,3 +266,12 @@ gdb_test "complete cond 1" "cond 1"
 gdb_test "set variable \$var = 1"
 gdb_test "complete cond \$v" "cond \\\$var"
 gdb_test "complete cond 1 values\[0\].a" "cond 1 values.0..a_field"
+
+gdb_test "cond 1 foo bar" ".*" "set invalid condition"
+
+gdb_test "info break 1" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+1\[\t \]+breakpoint     keep y.* <MULTIPLE>.*
+\[\t \]+stop only if foo bar.*
+1.1\[\t \]+n\[\t \]+.*in main at .*$srcfile:$bp_location6.*" \
+    "location with invalid condition is disabled"
diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
index 1f995d5..ad18245 100644
--- a/gdb/testsuite/gdb.base/macscp.exp
+++ b/gdb/testsuite/gdb.base/macscp.exp
@@ -444,6 +444,13 @@ gdb_test "cond \$bpnum foo == MACRO_TO_EXPAND" \
   "No symbol \"MACRO_TO_EXPAND\" in current context\." \
   "macro MACRO_TO_EXPAND not in scope at breakpoint"

+gdb_test "cond \$bpnum" \
+  "Breakpoint .* now unconditional\." \
+  "make breakpoint unconditional"
+
+gdb_test_no_output "enable \$bpnum.1" \
+  "re-enable breakpoint"
+
 # Note that we choose the condition so that this breakpoint never
 # stops.
 set l2 [gdb_get_line_number "set second breakpoint here"]

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

[Pedro Alves]

On 07/26/2012 08:05 PM, Pedro Alves wrote:

> 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 left the condition as is for now.  A pending breakpoint has no locations, and when
a shared library is unloaded, the locations remain (though disabled), so I think
this should be equivalent to the original intention.  So essentially, this is the
same patch as before, now with ChangeLog and with your test.

2012-08-02  Pedro Alves  <palves@redhat.com>
	    Marc Khouzam  <marc.khouzam@ericsson.com>

	* breakpoint.c (create_breakpoint): Only set condition_not_parsed
	if PARSE_CONDITION_AND_THREAD is true.
	(addr_string_to_sals): Consider NOT_FOUND_ERROR okay when there
	are no locations yet, instead of when CONDITION_NOT_PARSED is
	true.

2012-08-02  Marc Khouzam  <marc.khouzam@ericsson.com>

	* gdb.base/pendshr.c: Extra line to set a new breakpoint.
	* gdb.base/pending.exp: Set an enabled pending breakpoint
	with an explicit condition.
---

 gdb/testsuite/gdb.base/pending.exp |   57 ++++++++++++++++++++++++++++++------
 gdb/testsuite/gdb.base/pendshr.c   |    1 +
 2 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b8484a7..605dee2 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -9591,7 +9591,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.  */
@@ -9605,7 +9608,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)
@@ -14041,7 +14043,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))
diff --git a/gdb/testsuite/gdb.base/pending.exp b/gdb/testsuite/gdb.base/pending.exp
index 79c4576..910c917 100644
--- a/gdb/testsuite/gdb.base/pending.exp
+++ b/gdb/testsuite/gdb.base/pending.exp
@@ -209,6 +209,32 @@ gdb_test "info break" \
 "multiple pending breakpoints 2"

 #
+# Try a pending break with an explicit condition which is enabled at startup
+#
+
+set bp5_loc [gdb_get_line_number "y++" ${libfile}.c]
+gdb_test_multiple "break pendshr.c:$bp5_loc" "Set pending breakpoint 4" {
+     -re ".*Make breakpoint pending.*y or \\\[n\\\]. $" {
+           gdb_test "y" "Breakpoint.*pendshr.c:$bp5_loc.*pending." \
+               "Set pending breakpoint 5"
+     }
+}
+
+gdb_test_no_output "condition 5 k == 1"
+
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.*PENDING.*pendfunc1.*
+\[\t \]+stop only if k == 1.*
+\[\t \]+print k.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp2_loc if x > 3.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp3_loc.*ignore next 2 hits.*
+\[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendshr.c:$bp5_loc.*
+\[\t \]+stop only if k == 1.*" \
+"multiple pending breakpoints 3"
+
+#
 # Run to main which should resolve a pending breakpoint
 #

@@ -218,6 +244,24 @@ gdb_test "" \
 "running to main"

 #
+# Verify that all pending breakpoints have resolved.
+#
+gdb_test "info break" \
+    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
+\[0-9\]+\[\t \]+breakpoint     keep n.* in pendfunc1 at .*
+\[\t \]+stop only if k == 1.*
+\[\t \]+print k.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*$srcfile:$mainline.*
+\[\t \]+breakpoint already hit 1 time.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in pendfunc1 at .*pendshr.c:$bp2_loc.*
+\[\t \]+stop only if x > 3.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in pendfunc1 at .*pendshr.c:$bp3_loc.*
+\[\t \]+ignore next 2 hits.*
+\[0-9\]+\[\t \]+breakpoint     keep y.* in pendfunc1 at .*pendshr.c:$bp5_loc.*
+\[\t \]+stop only if k == 1.*" \
+"multiple pending breakpoints 4"
+
+#
 # Re-enable the first pending breakpoint which should resolve
 #

@@ -237,19 +281,14 @@ gdb_test "continue" \
 \[$\]1 = 1." \
 "continue to resolved breakpoint 1"

-#
-# Disable the other two breakpoints, and continue to the one with
-# the ignore count.  Make sure you hit it the third time, x should
-# be 3 then.
-#
-
-gdb_test "disable 7" "" "Disable other breakpoints"
-gdb_test "disable 5" "" "Disable other breakpoints"
-
 gdb_test "continue" \
 	 ".*Breakpoint.*pendfunc1.*\\\(x=3\\\) at.*pendshr.c:$bp3_loc.*printf.*;" \
 "continue to resolved breakpoint 3"

+gdb_test "continue" \
+        ".*Breakpoint.*pendfunc1.*\\\(x=3\\\) at.*pendshr.c:$bp5_loc.*y\\+\\+;" \
+"continue to resolved breakpoint 5"
+
 delete_breakpoints

 gdb_breakpoint "main"
diff --git a/gdb/testsuite/gdb.base/pendshr.c b/gdb/testsuite/gdb.base/pendshr.c
index bc3b9e3..c5b40fd 100644
--- a/gdb/testsuite/gdb.base/pendshr.c
+++ b/gdb/testsuite/gdb.base/pendshr.c
@@ -21,6 +21,7 @@ void pendfunc1 (int x)
 {
   int y = x + 4;
   printf ("in pendfunc1, x is %d\n", x);
+  y++;
 }

 void pendfunc (int x)