[patch] MI ignores conditions for pending breakpoints

[patch] MI ignores conditions for pending breakpoints

[Marc Khouzam]

Hi,

I got a bug report that conditions were ignored for pending breakpoints.
It turns out to only happen for MI and I added a example session at the end
of this mail.

The below patch fixes this by storing the condition string for pending bp.
The extra_string should receive the same treatment, but I'll send that in
a separate patch.

No regressions on Ubuntu 32bit (actually, I got better results, but I think
it has to do with intermittent failures).

Is this ok for HEAD and 7_5?

Thanks

Marc

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

	* breakpoint.c (create_breakpoint): Store condition for pending
	breakpoints.

### 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    21 Jul 2012 02:30:14 -0000
@@ -9557,7 +9557,18 @@
       init_raw_breakpoint_without_location (b, gdbarch, type_wanted, ops);
 
       b->addr_string = copy_arg;
-      b->cond_string = NULL;
+      if (parse_condition_and_thread)
+       b->cond_string = NULL;
+      else
+       {
+         /* Create a private copy of condition string.  */
+         if (cond_string)
+           {
+             cond_string = xstrdup (cond_string);
+             make_cleanup (xfree, cond_string);
+           }
+         b->cond_string = cond_string;
+       }
       b->extra_string = NULL;
       b->ignore_count = ignore_count;
       b->disposition = tempflag ? disp_del : disp_donttouch;


Broken session:
--------------
> gdb myapp.exe
GNU gdb (GDB) 7.5.50.20120720-cvs
(gdb) interpreter-exec mi "-break-insert -f -c 1==2 /home/lmckhou/runtime-TestDSF/myLinuxDll/src/mydll.c:3"
&"No source file named /home/lmckhou/runtime-TestDSF/myLinuxDll/src/mydll.c.\n"
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="<PENDING>",pending="/home/lmckhou/runtime-TestDSF/myLinuxDll/src/mydll.c:3",times="0",original-location="/home/lmckhou/runtime-TestDSF/myLinuxDll/src/mydll.c:3"}
(gdb) info b
Num     Type           Disp Enb Address    What
1       breakpoint     keep y   <PENDING>  /home/lmckhou/runtime-TestDSF/myLinuxDll/src/mydll.c:3

=> Note that there is no condition shown with the breakpoint above

(gdb) r
Starting program: /home/lmckhou/runtime-TestDSF/myapp/Debug/myapp.exe 
warning: Cannot call inferior functions, you have broken Linux kernel i386 NX (non-executable pages) support!
Breakpoint 1, foo () at ../src/mydll.c:3
3               int j = 0;

=> Breakpoint hits although condition was 1==2

Re: [patch] MI ignores conditions for pending breakpoints

[Tom Tromey]

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

Marc> 2012-07-20  Marc Khouzam  <marc.khouzam@ericsson.com>
Marc> 	* breakpoint.c (create_breakpoint): Store condition for pending
Marc> 	breakpoints.

This is ok for trunk and the 7.5 branch.
Thanks.

Marc> Broken session:
Marc> --------------

A patch to the test suite would be nice.

Tom

RE: [patch] MI ignores conditions for pending breakpoints

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Monday, July 23, 2012 3:08 PM
> To: Marc Khouzam
> Cc: gdb-patches@sourceware.org
> Subject: Re: [patch] MI ignores conditions for pending breakpoints
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> 2012-07-20  Marc Khouzam  <marc.khouzam@ericsson.com>
> Marc> 	* breakpoint.c (create_breakpoint): Store 
> condition for pending
> Marc> 	breakpoints.
> 
> This is ok for trunk and the 7.5 branch.
> Thanks.

Committed to both.

Thanks!

> 
> Marc> Broken session:
> Marc> --------------
> 
> A patch to the test suite would be nice.

I working on one now.

Marc

RE: [patch] MI ignores conditions for pending breakpoints

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Marc Khouzam
> Sent: Tuesday, July 24, 2012 1:42 PM
> To: 'Tom Tromey'
> Cc: 'gdb-patches@sourceware.org'
> Subject: RE: [patch] MI ignores conditions for pending breakpoints
> 
> > -----Original Message-----
> > From: Tom Tromey [mailto:tromey@redhat.com] 
> > Sent: Monday, July 23, 2012 3:08 PM
> > To: Marc Khouzam
> > Cc: gdb-patches@sourceware.org
> > Subject: Re: [patch] MI ignores conditions for pending breakpoints
> > 
> > >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> > 
> > Marc> 2012-07-20  Marc Khouzam  <marc.khouzam@ericsson.com>
> > Marc> 	* breakpoint.c (create_breakpoint): Store 
> > condition for pending
> > Marc> 	breakpoints.
> > 
> > This is ok for trunk and the 7.5 branch.
> > Thanks.
> 
> Committed to both.
> 
> Thanks!
> 
> > 
> > Marc> Broken session:
> > Marc> --------------
> > 
> > A patch to the test suite would be nice.

Here is a test case for the problem.  I'm not thrilled
with it but it does fail before the fix and pass
after, so it is probably sufficient.
What I don't like about it is that it verifies
that the condition prevents the breakpoint from
hitting instead of making sure a breakpoint hits at
the right condition.  I had to do that because the alternative
is to use a variable in the sharedlib for the condition,
but then I would need to explicitly specify the sharedlib
file name and line number when setting the breakpoint which
didn't seem very future-proof.

What do you think?



### Eclipse Workspace Patch 1.0
#P src
Index: gdb/testsuite/gdb.mi/mi-pending.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-pending.exp,v
retrieving revision 1.16
diff -u -r1.16 mi-pending.exp
--- gdb/testsuite/gdb.mi/mi-pending.exp 10 Jul 2012 15:32:51 -0000      1.16
+++ gdb/testsuite/gdb.mi/mi-pending.exp 25 Jul 2012 13:03:42 -0000
@@ -53,7 +53,14 @@
     ".*\\^done,bkpt=\{number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\",original-location=\"pendfunc1\"\}"\
     "MI pending breakpoint on pendfunc1"
 
+# Set pending breakpoint with a condition via MI
+# We use a condition that will prevent the bp from hitting
+mi_gdb_test "-break-insert -f -c 1==4 pendfunc0" \
+    ".*\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc0\",cond=\"1==4\",times=\"0\",original-location=\"pendfunc0\"\}"\
+    "MI pending breakpoint on pendfunc0 if 1==4"
+
 mi_run_cmd
+# We should not stop on the breakpiont at pendfunc0
 mi_expect_stop "breakpoint-hit" "pendfunc1" ".*" ".*" ".*" \
     { "" "disp=\"keep\"" } \
     "Run till MI pending breakpoint on pendfunc1"
Index: gdb/testsuite/gdb.mi/mi-pendshr.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-pendshr.c,v
retrieving revision 1.7
diff -u -r1.7 mi-pendshr.c
--- gdb/testsuite/gdb.mi/mi-pendshr.c   4 Jan 2012 08:17:54 -0000       1.7
+++ gdb/testsuite/gdb.mi/mi-pendshr.c   25 Jul 2012 13:03:42 -0000
@@ -17,6 +17,12 @@
 
 #include <stdio.h>
 
+void pendfunc0 (int x)
+{
+  int y = x + 4;
+  printf ("in pendfunc0, x is %d\n", x);
+}
+
 void pendfunc1 (int x)
 {
   int y = x + 4;
@@ -25,5 +31,6 @@
 
 void pendfunc (int x)
 {
+  pendfunc0 (x);
   pendfunc1 (x);
 }

Re: [patch] MI ignores conditions for pending breakpoints

[Tom Tromey]

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

Marc> Here is a test case for the problem.  I'm not thrilled
Marc> with it but it does fail before the fix and pass
Marc> after, so it is probably sufficient.
Marc> What I don't like about it is that it verifies
Marc> that the condition prevents the breakpoint from
Marc> hitting instead of making sure a breakpoint hits at
Marc> the right condition.  I had to do that because the alternative
Marc> is to use a variable in the sharedlib for the condition,
Marc> but then I would need to explicitly specify the sharedlib
Marc> file name and line number when setting the breakpoint which
Marc> didn't seem very future-proof.

Marc> What do you think?

I think as long as it exercises the bug -- fails before the patch and
passes after -- and is reasonably clean, then it is fine.

Coding in the name of the library, etc, is also fine; there are some
tools in the test suite to make this a bit less fragile, namely using
variables in place of file names and using gdb_get_line_number.

I don't have any problem with the approach in the patch though.

Marc> +# Set pending breakpoint with a condition via MI
Marc> +# We use a condition that will prevent the bp from hitting

Periods after sentences.

Marc> +# We should not stop on the breakpiont at pendfunc0

Typo, "breakpoint".

Ok with those changes plus a ChangeLog entry.

Tom

RE: [patch] MI ignores conditions for pending breakpoints

[Marc Khouzam]

 
> I think as long as it exercises the bug -- fails before the patch and
> passes after -- and is reasonably clean, then it is fine.
> 
> Coding in the name of the library, etc, is also fine; there are some
> tools in the test suite to make this a bit less fragile, namely using
> variables in place of file names and using gdb_get_line_number.

I updated to a better test based on what you said.  The test now
makes sure the breakpoint is skipped when it should and hits
when it should.

Ok?

Thanks!


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

	* gdb.mi/mi-pending.c: New method to set a second pending
	breakpoint.
	* gdb.mi/mi-pending.exp: Set a pending breakpoint with a
	condition.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/testsuite/gdb.mi/mi-pending.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-pending.exp,v
retrieving revision 1.16
diff -u -r1.16 mi-pending.exp
--- gdb/testsuite/gdb.mi/mi-pending.exp 10 Jul 2012 15:32:51 -0000      1.16
+++ gdb/testsuite/gdb.mi/mi-pending.exp 25 Jul 2012 17:39:48 -0000
@@ -24,8 +24,9 @@
     return 0
 }
 
-standard_testfile mi-pending.c mi-pendshr.c
-set lib_sl [standard_output_file mi-pendshr.sl]
+set libfile "mi-pendshr"
+standard_testfile mi-pending.c $libfile.c
+set lib_sl [standard_output_file $libfile.sl]
 
 set lib_opts  debug
 set exec_opts [list debug shlib=$lib_sl]
@@ -48,12 +49,29 @@
 mi_gdb_load ${binfile}
 mi_load_shlibs $lib_sl
 
-# Set pending breakpoint via MI
+# Set pending breakpoint via MI.
 mi_gdb_test "-break-insert -f pendfunc1" \
     ".*\\^done,bkpt=\{number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\",original-location=\"pendfunc1\"\}"\
     "MI pending breakpoint on pendfunc1"
 
+# Set pending breakpoint with a condition via MI.
+mi_gdb_test "-break-insert -f -c x==4 ${libfile}.c:pendfunc2" \
+    ".*\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"${libfile}.c:pendfunc2\",cond=\"x==4\",times=\"0\",original-location=\"${libfile}.c:pendfunc2\"\}"\
+    "MI pending breakpoint on ${libfile}.c:pendfunc2 if x==4"
+
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "pendfunc1" ".*" ".*" ".*" \
     { "" "disp=\"keep\"" } \
     "Run till MI pending breakpoint on pendfunc1"
+
+mi_send_resuming_command "exec-continue" "continuing execution to skip conditional bp"
+# We should not stop on the conditional breakpoint yet, but we stop on the original bp. 
+mi_expect_stop "breakpoint-hit" "pendfunc1" ".*" ".*" ".*" \
+    { "" "disp=\"keep\"" } \
+    "Run till MI pending breakpoint on pendfunc1 a second time"
+
+mi_send_resuming_command "exec-continue" "continuing execution to conditional bp"
+# Now we should stop on the conditional breakpoint.
+mi_expect_stop "breakpoint-hit" "pendfunc2" ".*" ".*" ".*" \
+    { "" "disp=\"keep\"" } \
+    "Run till MI pending breakpoint on pendfunc2 with x==4"
Index: gdb/testsuite/gdb.mi/mi-pendshr.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-pendshr.c,v
retrieving revision 1.7
diff -u -r1.7 mi-pendshr.c
--- gdb/testsuite/gdb.mi/mi-pendshr.c   4 Jan 2012 08:17:54 -0000       1.7
+++ gdb/testsuite/gdb.mi/mi-pendshr.c   25 Jul 2012 17:39:48 -0000
@@ -23,7 +23,13 @@
   printf ("in pendfunc1, x is %d\n", x);
 }
 
+void pendfunc2 (int x)
+{
+  printf ("in pendfunc2, x is %d\n", x);
+}
+
 void pendfunc (int x)
 {
   pendfunc1 (x);
+  pendfunc2 (x);
 }

Re: [patch] MI ignores conditions for pending breakpoints

[Tom Tromey]

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

Marc> -standard_testfile mi-pending.c mi-pendshr.c
Marc> -set lib_sl [standard_output_file mi-pendshr.sl]
Marc> +set libfile "mi-pendshr"
Marc> +standard_testfile mi-pending.c $libfile.c
Marc> +set lib_sl [standard_output_file $libfile.sl]

I think it is better to leave the standard_testfile call as-is:

    standard_testfile mi-pending.c mi-pendshr.c
    set lib_sl [standard_output_file mi-pendshr.sl]

and then

Marc> +mi_gdb_test "-break-insert -f -c x==4 ${libfile}.c:pendfunc2" \

... use ${srcfile2} here instead of ${libfile}.c

Setting srcfile2 is part of the contract of standard_testfile.

Tom

RE: [patch] MI ignores conditions for pending breakpoints

[Marc Khouzam]

 

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Wednesday, July 25, 2012 1:49 PM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [patch] MI ignores conditions for pending breakpoints
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> -standard_testfile mi-pending.c mi-pendshr.c
> Marc> -set lib_sl [standard_output_file mi-pendshr.sl]
> Marc> +set libfile "mi-pendshr"
> Marc> +standard_testfile mi-pending.c $libfile.c
> Marc> +set lib_sl [standard_output_file $libfile.sl]
> 
> I think it is better to leave the standard_testfile call as-is:
> 
>     standard_testfile mi-pending.c mi-pendshr.c
>     set lib_sl [standard_output_file mi-pendshr.sl]
> 
> and then
> 
> Marc> +mi_gdb_test "-break-insert -f -c x==4 ${libfile}.c:pendfunc2" \
> 
> ... use ${srcfile2} here instead of ${libfile}.c
> 
> Setting srcfile2 is part of the contract of standard_testfile.

Thanks for that.  I've also added a check to make sure x==4 when
the breakpoint hits.  How about this?


P.S. To save time, is there a good way to run a single test while
writing it?  I currently hack around to get it done, but I'm hoping
there is a better way.

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

	* gdb.mi/mi-pending.c: New method to set a second pending
	breakpoint.
	* gdb.mi/mi-pending.exp: Set a pending breakpoint with a
	condition.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/testsuite/gdb.mi/mi-pending.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-pending.exp,v
retrieving revision 1.16
diff -u -r1.16 mi-pending.exp
--- gdb/testsuite/gdb.mi/mi-pending.exp 10 Jul 2012 15:32:51 -0000      1.16
+++ gdb/testsuite/gdb.mi/mi-pending.exp 25 Jul 2012 18:24:54 -0000
@@ -48,12 +48,29 @@
 mi_gdb_load ${binfile}
 mi_load_shlibs $lib_sl
 
-# Set pending breakpoint via MI
+# Set pending breakpoint via MI.
 mi_gdb_test "-break-insert -f pendfunc1" \
     ".*\\^done,bkpt=\{number=\"1\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"pendfunc1\",times=\"0\",original-location=\"pendfunc1\"\}"\
     "MI pending breakpoint on pendfunc1"
 
+# Set pending breakpoint with a condition via MI.
+mi_gdb_test "-break-insert -f -c x==4 ${srcfile2}:pendfunc2" \
+    ".*\\^done,bkpt=\{number=\"2\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"<PENDING>\",pending=\"${srcfile2}:pendfunc2\",cond=\"x==4\",times=\"0\",original-location=\"${srcfile2}:pendfunc2\"\}"\
+    "MI pending breakpoint on ${srcfile2}:pendfunc2 if x==4"
+
 mi_run_cmd
 mi_expect_stop "breakpoint-hit" "pendfunc1" ".*" ".*" ".*" \
     { "" "disp=\"keep\"" } \
     "Run till MI pending breakpoint on pendfunc1"
+
+mi_send_resuming_command "exec-continue" "continuing execution to skip conditional bp"
+# We should not stop on the conditional breakpoint yet, but we stop on the original bp. 
+mi_expect_stop "breakpoint-hit" "pendfunc1" ".*" ".*" ".*" \
+    { "" "disp=\"keep\"" } \
+    "Run till MI pending breakpoint on pendfunc1 a second time"
+
+mi_send_resuming_command "exec-continue" "continuing execution to conditional bp"
+# Now we should stop on the conditional breakpoint.
+mi_expect_stop "breakpoint-hit" "pendfunc2" "\{name=\"x\",value=\"4\"\}" ".*" ".*" \
+    { "" "disp=\"keep\"" } \
+    "Run till MI pending breakpoint on pendfunc2 with x==4"
Index: gdb/testsuite/gdb.mi/mi-pendshr.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-pendshr.c,v
retrieving revision 1.7
diff -u -r1.7 mi-pendshr.c
--- gdb/testsuite/gdb.mi/mi-pendshr.c   4 Jan 2012 08:17:54 -0000       1.7
+++ gdb/testsuite/gdb.mi/mi-pendshr.c   25 Jul 2012 18:24:54 -0000
@@ -23,7 +23,13 @@
   printf ("in pendfunc1, x is %d\n", x);
 }
 
+void pendfunc2 (int x)
+{
+  printf ("in pendfunc2, x is %d\n", x);
+}
+
 void pendfunc (int x)
 {
   pendfunc1 (x);
+  pendfunc2 (x);
 }

RE: [patch] MI ignores conditions for pending breakpoints

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Wednesday, July 25, 2012 2:30 PM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [patch] MI ignores conditions for pending breakpoints
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> Thanks for that.  I've also added a check to make sure x==4 when
> Marc> the breakpoint hits.  How about this?
> 
> Ok.

Committed to HEAD.

> 
> Marc> P.S. To save time, is there a good way to run a single 
> test while
> Marc> writing it?
> 
> What I do is
> 
>    cd build/gdb/testsuite
>    runtest whatever.exp

Awesome!  That's what I needed.

> "make check RUNTESTFLAGS=whatever.exp" also works but is wordier.
> 
> You can also run just a single directory with
> 
>    runtest --directory=gdb.python

I didn't know that, but I found this one (slightly faster to type):
  make check-gdb.python

Thanks!

Marc

Re: [patch] MI ignores conditions for pending breakpoints

[Tom Tromey]

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

Marc> Thanks for that.  I've also added a check to make sure x==4 when
Marc> the breakpoint hits.  How about this?

Ok.

Marc> P.S. To save time, is there a good way to run a single test while
Marc> writing it?

What I do is

   cd build/gdb/testsuite
   runtest whatever.exp

"make check RUNTESTFLAGS=whatever.exp" also works but is wordier.

You can also run just a single directory with

   runtest --directory=gdb.python

Tom