diff mbox

[RFA] Fix dangling cleanup in linespec_parse_basic

Message ID 535A9F8F.1080809@redhat.com
State Committed
Headers

Commit Message

Keith Seitz April 25, 2014, 5:46 p.m. UTC 
  Hi,

For a linespec such as "filename:$convenience_variable", the local 
variable holding the name of the convenience variable ('name' in the 
code) is not freed if no parsing error occurred, i.e., the convenience 
variable is defined. This leaves a dangling cleanup.

The error case was already properly handling the cleanup.  This patch 
adds the appropriate cleanup handling when there is no error.

I've also added a test for this to ls-dollar.exp.

Ok?

Keith

ChangeLog
2014-04-25  Keith Seitz  <keiths@redhat.com>

	* linespec.c (linespec_parse_basic): Run cleanups if a convenience
	variable or history value is successfully parsed.

testsuite/ChangeLog
2014-04-25  Keith Seitz  <keiths@redhat.com>

	* gdb.linespec/ls-dollar.exp: Add test for linespec
	file:convenience_variable.

Comments

Tom Tromey April 25, 2014, 7:05 p.m. UTC | #1 
>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> For a linespec such as "filename:$convenience_variable", the local
Keith> variable holding the name of the convenience variable ('name' in the
Keith> code) is not freed if no parsing error occurred, i.e., the convenience
Keith> variable is defined. This leaves a dangling cleanup.

Keith> The error case was already properly handling the cleanup.  This patch
Keith> adds the appropriate cleanup handling when there is no error.

Keith> I've also added a test for this to ls-dollar.exp.

Thanks.

This patch is ok.

I found the cleanup code in linespec_parse_basic a bit hard to follow
but I don't have any concrete suggestions.

Tom
Keith Seitz May 5, 2014, 8:46 p.m. UTC | #2 
On 04/25/2014 12:05 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>
> Keith> For a linespec such as "filename:$convenience_variable", the local
> Keith> variable holding the name of the convenience variable ('name' in the
> Keith> code) is not freed if no parsing error occurred, i.e., the convenience
> Keith> variable is defined. This leaves a dangling cleanup.
>
> Keith> The error case was already properly handling the cleanup.  This patch
> Keith> adds the appropriate cleanup handling when there is no error.
>
> Keith> I've also added a test for this to ls-dollar.exp.
>
> Thanks.
>
> This patch is ok.

I've pushed this patch. Thank you for taking a look.

Keith
diff mbox

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 610809d..cb76b9c 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -1665,6 +1665,10 @@  linespec_parse_basic (linespec_parser *parser)
 	      discard_cleanups (cleanup);
 	      return;
 	    }
+
+	  /* The convenience variable/history value parsed correctly.
+	     NAME is no longer needed.  */
+	  do_cleanups (cleanup);
 	}
       else
 	{
diff --git a/gdb/testsuite/gdb.linespec/ls-dollar.exp b/gdb/testsuite/gdb.linespec/ls-dollar.exp
index 2e35804..bccc40a 100644
--- a/gdb/testsuite/gdb.linespec/ls-dollar.exp
+++ b/gdb/testsuite/gdb.linespec/ls-dollar.exp
@@ -30,7 +30,16 @@  if {[prepare_for_testing $testfile $exefile $srcfile \
 
 gdb_test_no_output "set listsize 1"
 
+set line [gdb_get_line_number {dollar_func}]
+
 gdb_test "list \$dollar_var" \
     ".*static int [string_to_regexp {$dollar_var}] = 0;"
 gdb_test "break \$dollar_func" \
-    "Breakpoint $decimal at $hex: file .*$srcfile, line [gdb_get_line_number {dollar_func}]\\\."
+    "Breakpoint $decimal at $hex: file .*$srcfile, line $line\\\."
+
+gdb_test_no_output "set var \$theline = $line"
+gdb_test "list $srcfile:\$theline" \
+    ".*[string_to_regexp {/* dollar_func */}]"
+
+gdb_test "break $srcfile:\$theline" \
+    "Breakpoint $decimal at $hex: file .*$srcfile, line $line\\\."