[PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[Sergio Durigan Junior]

Hi,

This patch is a fix for PR 15413.  The failure happens when one tries to
complete the argument for the "condition" command if there is only a
pending breakpoint inserted.  In this case, GDB segfaults.

The submitter pinged me on IRC this Sunday with a suggestion to fix the
issue, which correctly eliminated the segfault but still wasn't complete
because the "condition" completer did not complete the breakpoint number
yet.  I modified the code so that the completion now works OK and no
segfault happens.

The fix is trivial IMO, so it shouldn't take long to approve the patch.
Regtested on Fedora 17 x86_64, no regressions.

OK to apply?

-- 
Sergio

gdb/
2013-05-05  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/15413:
	* breakpoint.c (condition_completer): Rewrite parts of the code to
	handle completion of the "condition" command for pending
	breakpoints.

gdb/testsuite/
2013-05-05  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/15413:
	* gdb.base/pending.exp: Add test for completion of the "condition"
	command for pending breakpoints.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 35ada7a..81bf5ed 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1013,25 +1013,36 @@ condition_completer (struct cmd_list_element *cmd,
 
       ALL_BREAKPOINTS (b)
       {
-	int single = b->loc->next == NULL;
-	struct bp_location *loc;
+	struct bp_location *loc = NULL;
 	int count = 1;
 
-	for (loc = b->loc; loc; loc = loc->next)
+	do
 	  {
 	    char location[50];
 
-	    if (single)
-	      xsnprintf (location, sizeof (location), "%d", b->number);
+	    if (b->loc == NULL)
+	      {
+		/* We're probably dealing with a pending breakpoint.  Just
+		   inform its number.  */
+		xsnprintf (location, sizeof (location), "%d", b->number);
+	      }
 	    else
-	      xsnprintf (location, sizeof (location),  "%d.%d", b->number,
-			 count);
+	      {
+		if (b->loc->next == NULL)
+		  xsnprintf (location, sizeof (location), "%d", b->number);
+		else
+		  xsnprintf (location, sizeof (location),  "%d.%d", b->number,
+			     count);
+
+		loc = b->loc->next;
+	      }
 
 	    if (strncmp (location, text, len) == 0)
 	      VEC_safe_push (char_ptr, result, xstrdup (location));
 
 	    ++count;
 	  }
+	while (loc != NULL);
       }
 
       return result;
diff --git a/gdb/testsuite/gdb.base/pending.exp b/gdb/testsuite/gdb.base/pending.exp
index 68322f5..a8dbef5 100644
--- a/gdb/testsuite/gdb.base/pending.exp
+++ b/gdb/testsuite/gdb.base/pending.exp
@@ -55,6 +55,13 @@ gdb_test_multiple "break pendfunc1" "set pending breakpoint" {
      }
 }
 
+# Complete the condition (PR 15413).
+# This test must come right after we set the first pending breakpoint, and
+# before we set any other breakpoint, since we are testing if the "condition"
+# command can properly complete its argument.  The PR only fails if there
+# is only one pending breakpoint set (without anything else).
+gdb_test "complete condition " "condition 1"
+
 gdb_test "info break" \
     "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendfunc1.*" \

Re: [PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[Pedro Alves]

On 05/06/2013 03:58 AM, Sergio Durigan Junior wrote:
> gdb/
> 2013-05-05  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR breakpoints/15413:
> 	* breakpoint.c (condition_completer): Rewrite parts of the code to
> 	handle completion of the "condition" command for pending
> 	breakpoints.
> 
> gdb/testsuite/
> 2013-05-05  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR breakpoints/15413:
> 	* gdb.base/pending.exp: Add test for completion of the "condition"
> 	command for pending breakpoints.
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 35ada7a..81bf5ed 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1013,25 +1013,36 @@ condition_completer (struct cmd_list_element *cmd,
>  
>        ALL_BREAKPOINTS (b)
>        {
> -	int single = b->loc->next == NULL;
> -	struct bp_location *loc;
> +	struct bp_location *loc = NULL;
>  	int count = 1;
>  
> -	for (loc = b->loc; loc; loc = loc->next)
> +	do
>  	  {
>  	    char location[50];
>  
> -	    if (single)
> -	      xsnprintf (location, sizeof (location), "%d", b->number);
> +	    if (b->loc == NULL)
> +	      {
> +		/* We're probably dealing with a pending breakpoint.  Just
> +		   inform its number.  */

s/probably//

s/inform/complete/ ?

> +		xsnprintf (location, sizeof (location), "%d", b->number);
> +	      }
>  	    else
> -	      xsnprintf (location, sizeof (location),  "%d.%d", b->number,
> -			 count);
> +	      {
> +		if (b->loc->next == NULL)
> +		  xsnprintf (location, sizeof (location), "%d", b->number);
> +		else
> +		  xsnprintf (location, sizeof (location),  "%d.%d", b->number,
> +			     count);
> +
> +		loc = b->loc->next;

This is always picking the same loc over and over?  I guess this means
the test should be extended.  :-)

> +	      }
>  
>  	    if (strncmp (location, text, len) == 0)
>  	      VEC_safe_push (char_ptr, result, xstrdup (location));
>  
>  	    ++count;
>  	  }
> +	while (loc != NULL);
>        }
>  

I notice the condition completer is more broken than this, btw:

$ ./gdb ./testsuite/gdb.cp/mb-ctor
GNU gdb (GDB) 7.6.50.20130430-cvs
(gdb) b Derived::Derived
Breakpoint 1 at 0x400811: Derived::Derived. (2 locations)
(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   <MULTIPLE>
1.1                         y     0x0000000000400811 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
1.2                         y     0x0000000000400867 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
(gdb) complete condition
condition 1.1
condition 1.2
(gdb) complete condition 1
condition 1.1
condition 1.2
(gdb) complete condition 1.
condition 1.1.1
condition 1.1.2
(gdb) complete condition 1.1
condition 1.1.1
(gdb) complete condition 1.1
condition 1.1.1
(gdb) complete condition 1.1.1
(gdb)

Or:

(gdb) condition<tab>
(gdb) condition <tab>1.<tab>1.
(gdb) condition 1.
(gdb) condition 1.<tab>
(gdb) condition 1.1.<tab>
(gdb) condition 1.1.<enter>
Bad breakpoint argument: '1.1.'


BTW2, I'm thinking it'd make sense to always include the
breakpoint-number-only ("%d", b->number) completion option, even if there
are multiple locations?  That is, with breakpoint 1 having two locations,
this would happen:

(gdb) condition 1<tab>
1 1.1 1.2

instead of:
(gdb) condition 1<tab>
(gdb) condition 1.
(gdb) condition 1.<tab>
1.1 1.2

Oh, wait, wait, wait...  The condition is a breakpoint property,
not a location property, so what's with the completer suggesting
location numbers at all?

(gdb) info breakpoints
Num     Type           Disp Enb Address            What
1       breakpoint     keep y   <MULTIPLE>
1.1                         y     0x0000000000400811 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
1.2                         y     0x0000000000400867 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
(gdb) condition 1 0
(gdb)
(gdb) condition 1.2 0
Bad breakpoint argument: '1.2 0'

-- 
Pedro Alves

Re: [PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[Sergio Durigan Junior]

Thanks for the review.

On Monday, May 06 2013, Pedro Alves wrote:

> On 05/06/2013 03:58 AM, Sergio Durigan Junior wrote:

>> +		xsnprintf (location, sizeof (location), "%d", b->number);
>> +	      }
>>  	    else
>> -	      xsnprintf (location, sizeof (location),  "%d.%d", b->number,
>> -			 count);
>> +	      {
>> +		if (b->loc->next == NULL)
>> +		  xsnprintf (location, sizeof (location), "%d", b->number);
>> +		else
>> +		  xsnprintf (location, sizeof (location),  "%d.%d", b->number,
>> +			     count);
>> +
>> +		loc = b->loc->next;
>
> This is always picking the same loc over and over?  I guess this means
> the test should be extended.  :-)

Ops, you're right, I will fix it right away, thanks for the catch.

>> +	      }
>>  
>>  	    if (strncmp (location, text, len) == 0)
>>  	      VEC_safe_push (char_ptr, result, xstrdup (location));
>>  
>>  	    ++count;
>>  	  }
>> +	while (loc != NULL);
>>        }
>>  
>
> I notice the condition completer is more broken than this, btw:
>
> $ ./gdb ./testsuite/gdb.cp/mb-ctor
> GNU gdb (GDB) 7.6.50.20130430-cvs
> (gdb) b Derived::Derived
> Breakpoint 1 at 0x400811: Derived::Derived. (2 locations)
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   <MULTIPLE>
> 1.1                         y     0x0000000000400811 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
> 1.2                         y     0x0000000000400867 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
> (gdb) complete condition
> condition 1.1
> condition 1.2
> (gdb) complete condition 1
> condition 1.1
> condition 1.2
> (gdb) complete condition 1.
> condition 1.1.1
> condition 1.1.2
> (gdb) complete condition 1.1
> condition 1.1.1
> (gdb) complete condition 1.1
> condition 1.1.1
> (gdb) complete condition 1.1.1
> (gdb)
>
> Or:
>
> (gdb) condition<tab>
> (gdb) condition <tab>1.<tab>1.
> (gdb) condition 1.
> (gdb) condition 1.<tab>
> (gdb) condition 1.1.<tab>
> (gdb) condition 1.1.<enter>
> Bad breakpoint argument: '1.1.'
>
>
> BTW2, I'm thinking it'd make sense to always include the
> breakpoint-number-only ("%d", b->number) completion option, even if there
> are multiple locations?  That is, with breakpoint 1 having two locations,
> this would happen:
>
> (gdb) condition 1<tab>
> 1 1.1 1.2
>
> instead of:
> (gdb) condition 1<tab>
> (gdb) condition 1.
> (gdb) condition 1.<tab>
> 1.1 1.2
>
> Oh, wait, wait, wait...  The condition is a breakpoint property,
> not a location property, so what's with the completer suggesting
> location numbers at all?
>
> (gdb) info breakpoints
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y   <MULTIPLE>
> 1.1                         y     0x0000000000400811 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
> 1.2                         y     0x0000000000400867 in Derived::Derived(int) at ../../../src/gdb/testsuite/gdb.cp/mb-ctor.cc:34
> (gdb) condition 1 0
> (gdb)
> (gdb) condition 1.2 0
> Bad breakpoint argument: '1.2 0'

So, if I understood your brain dump correctly, you're suggesting that
the "condition" command shouldn't complete multiple locations at all,
since the condition is inherent to the breakpoint, not to the
location(s).  Is that right?  I will submit a patch soon.

[OTOH, I guess it would make more sense if the condition were a location
property.]

Thanks,

-- 
Sergio

Re: [PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[Pedro Alves]

On 05/06/2013 09:57 PM, Sergio Durigan Junior wrote:

> So, if I understood your brain dump correctly, you're suggesting that
> the "condition" command shouldn't complete multiple locations at all,
> since the condition is inherent to the breakpoint, not to the
> location(s).  Is that right?  I will submit a patch soon.

Yes.  The patch to do that should be quite trivial,
mostly just removing/simplifying code, and it should fix the
segfault too as consequence, so I'd prefer focusing on a patch
that did that first (over trying to fix the existing bogus
multi-location code),

> [OTOH, I guess it would make more sense if the condition were a location
> property.]

and leave this for a separate discussion.  The patch should then
be quite safe for 7.6 backporting too.

-- 
Pedro Alves

Re: [PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[Sergio Durigan Junior]

On Monday, May 06 2013, Pedro Alves wrote:

> On 05/06/2013 09:57 PM, Sergio Durigan Junior wrote:
>
>> So, if I understood your brain dump correctly, you're suggesting that
>> the "condition" command shouldn't complete multiple locations at all,
>> since the condition is inherent to the breakpoint, not to the
>> location(s).  Is that right?  I will submit a patch soon.
>
> Yes.  The patch to do that should be quite trivial,
> mostly just removing/simplifying code, and it should fix the
> segfault too as consequence, so I'd prefer focusing on a patch
> that did that first (over trying to fix the existing bogus
> multi-location code),
>
>> [OTOH, I guess it would make more sense if the condition were a location
>> property.]
>
> and leave this for a separate discussion.  The patch should then
> be quite safe for 7.6 backporting too.

Sure enough, I wasn't planning to address this whole issue now.

Here is the updated patch.  I created a new test on
gdb.linespec/linespec.exp because it seemed like the best place to test
the "condition" for multiple locations.

OK?

-- 
Sergio

gdb/
2013-05-07  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/15413:
	* breakpoint.c (condition_completer): Simplify the code to
	disconsider multiple locations of breakpoints when completing the
	"condition" command.

gdb/testsuite
2013-05-07  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/15413:
	* gdb.base/pending.exp: Add test for completion of the "condition"
	command for pending breakpoints.
	* gdb.linespec/linespec.ex: Add test for completion of the
	"condition" command when dealing with multiple locations.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ef9c23c..b1488dc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1012,27 +1012,14 @@ condition_completer (struct cmd_list_element *cmd,
       len = strlen (text);
 
       ALL_BREAKPOINTS (b)
-      {
-	int single = b->loc->next == NULL;
-	struct bp_location *loc;
-	int count = 1;
-
-	for (loc = b->loc; loc; loc = loc->next)
-	  {
-	    char location[50];
-
-	    if (single)
-	      xsnprintf (location, sizeof (location), "%d", b->number);
-	    else
-	      xsnprintf (location, sizeof (location),  "%d.%d", b->number,
-			 count);
+	{
+	  char location[50];
 
-	    if (strncmp (location, text, len) == 0)
-	      VEC_safe_push (char_ptr, result, xstrdup (location));
+	  xsnprintf (location, sizeof (location), "%d", b->number);
 
-	    ++count;
-	  }
-      }
+	  if (strncmp (location, text, len) == 0)
+	    VEC_safe_push (char_ptr, result, xstrdup (location));
+	}
 
       return result;
     }
diff --git a/gdb/testsuite/gdb.base/pending.exp b/gdb/testsuite/gdb.base/pending.exp
index 68322f5..a8dbef5 100644
--- a/gdb/testsuite/gdb.base/pending.exp
+++ b/gdb/testsuite/gdb.base/pending.exp
@@ -55,6 +55,13 @@ gdb_test_multiple "break pendfunc1" "set pending breakpoint" {
      }
 }
 
+# Complete the condition (PR 15413).
+# This test must come right after we set the first pending breakpoint, and
+# before we set any other breakpoint, since we are testing if the "condition"
+# command can properly complete its argument.  The PR only fails if there
+# is only one pending breakpoint set (without anything else).
+gdb_test "complete condition " "condition 1"
+
 gdb_test "info break" \
     "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendfunc1.*" \
diff --git a/gdb/testsuite/gdb.linespec/linespec.exp b/gdb/testsuite/gdb.linespec/linespec.exp
index 741ada0..fe02365 100644
--- a/gdb/testsuite/gdb.linespec/linespec.exp
+++ b/gdb/testsuite/gdb.linespec/linespec.exp
@@ -69,6 +69,10 @@ gdb_test "break dupname:label" \
     "Breakpoint $decimal at $hex: dupname:label. \[(\]2 locations\[)\]" \
     "multi-location break using duplicate function name and label"
 
+# Testing if the "condition" command completes only the breakpoints,
+# not the locations.
+gdb_test "complete condition " "condition $decimal\r\ncondition $decimal\r\ncondition $decimal"
+
 gdb_test_no_output "set breakpoint pending off" \
     "disable pending breakpoints for linespec tests"
 

Re: [PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[Pedro Alves]

On 05/07/2013 04:39 AM, Sergio Durigan Junior wrote:

> -	for (loc = b->loc; loc; loc = loc->next)
> -	  {
> -	    char location[50];
> -
> -	    if (single)
> -	      xsnprintf (location, sizeof (location), "%d", b->number);
> -	    else
> -	      xsnprintf (location, sizeof (location),  "%d.%d", b->number,
> -			 count);
> +	{
> +	  char location[50];

This is no longer a location.  So s/location/number/g.

>  
> +# Complete the condition (PR 15413).
> +# This test must come right after we set the first pending breakpoint, and
> +# before we set any other breakpoint, since we are testing if the "condition"
> +# command can properly complete its argument.  The PR only fails if there
> +# is only one pending breakpoint set (without anything else).

This last sentence isn't right.  You only need a pending breakpoint to
trigger the crash, it doesn't matter how many breakpoints you have.

> +gdb_test "complete condition " "condition 1"

OK with these fixed, mainline and 7.6.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Fix for PR 15413 (segfault when completing "condition" for pending bp)

[Sergio Durigan Junior]

On Tuesday, May 07 2013, Pedro Alves wrote:

>> +# Complete the condition (PR 15413).
>> +# This test must come right after we set the first pending breakpoint, and
>> +# before we set any other breakpoint, since we are testing if the "condition"
>> +# command can properly complete its argument.  The PR only fails if there
>> +# is only one pending breakpoint set (without anything else).
>
> This last sentence isn't right.  You only need a pending breakpoint to
> trigger the crash, it doesn't matter how many breakpoints you have.

Fixed, thanks.

>> +gdb_test "complete condition " "condition 1"
>
> OK with these fixed, mainline and 7.6.

This is what I committed.

     http://sourceware.org/ml/gdb-cvs/2013-05/msg00062.html
     http://sourceware.org/ml/gdb-cvs/2013-05/msg00063.html

Thanks,

-- 
Sergio

gdb/
2013-05-07  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/15413:
	* breakpoint.c (condition_completer): Simplify the code to
	disconsider multiple locations of breakpoints when completing the
	"condition" command.

gdb/testsuite
2013-05-07  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/15413:
	* gdb.base/pending.exp: Add test for completion of the "condition"
	command for pending breakpoints.
	* gdb.linespec/linespec.ex: Add test for completion of the
	"condition" command when dealing with multiple locations.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ef9c23c..f4f9325 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1012,27 +1012,14 @@ condition_completer (struct cmd_list_element *cmd,
       len = strlen (text);
 
       ALL_BREAKPOINTS (b)
-      {
-	int single = b->loc->next == NULL;
-	struct bp_location *loc;
-	int count = 1;
-
-	for (loc = b->loc; loc; loc = loc->next)
-	  {
-	    char location[50];
-
-	    if (single)
-	      xsnprintf (location, sizeof (location), "%d", b->number);
-	    else
-	      xsnprintf (location, sizeof (location),  "%d.%d", b->number,
-			 count);
+	{
+	  char number[50];
 
-	    if (strncmp (location, text, len) == 0)
-	      VEC_safe_push (char_ptr, result, xstrdup (location));
+	  xsnprintf (number, sizeof (number), "%d", b->number);
 
-	    ++count;
-	  }
-      }
+	  if (strncmp (number, text, len) == 0)
+	    VEC_safe_push (char_ptr, result, xstrdup (number));
+	}
 
       return result;
     }
diff --git a/gdb/testsuite/gdb.base/pending.exp b/gdb/testsuite/gdb.base/pending.exp
index 68322f5..1ab896a 100644
--- a/gdb/testsuite/gdb.base/pending.exp
+++ b/gdb/testsuite/gdb.base/pending.exp
@@ -55,6 +55,9 @@ gdb_test_multiple "break pendfunc1" "set pending breakpoint" {
      }
 }
 
+# Complete the condition (PR 15413).
+gdb_test "complete condition " "condition 1"
+
 gdb_test "info break" \
     "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
 \[0-9\]+\[\t \]+breakpoint     keep y.*PENDING.*pendfunc1.*" \
diff --git a/gdb/testsuite/gdb.linespec/linespec.exp b/gdb/testsuite/gdb.linespec/linespec.exp
index 741ada0..fe02365 100644
--- a/gdb/testsuite/gdb.linespec/linespec.exp
+++ b/gdb/testsuite/gdb.linespec/linespec.exp
@@ -69,6 +69,10 @@ gdb_test "break dupname:label" \
     "Breakpoint $decimal at $hex: dupname:label. \[(\]2 locations\[)\]" \
     "multi-location break using duplicate function name and label"
 
+# Testing if the "condition" command completes only the breakpoints,
+# not the locations.
+gdb_test "complete condition " "condition $decimal\r\ncondition $decimal\r\ncondition $decimal"
+
 gdb_test_no_output "set breakpoint pending off" \
     "disable pending breakpoints for linespec tests"