[PATCH] Fix for PR gdb/17235: possible bug extracting systemtap probe operand

[PATCH] Fix for PR gdb/17235: possible bug extracting systemtap probe operand

[Sergio Durigan Junior]

Hi there,

This patch is a fix to PR gdb/17235.  The bug is about an unused
variable that got declared and set during one of the parsing phases of
an SDT probe's argument.  I took the opportunity to rewrite some of the
code to improve the parsing.  The bug was actually a thinko, because
what I wanted to do in the code was to discard the number on the string
being parsed.

During this portion, the code identifies that it is dealing with an
expression that begins with a sign ('+', '-' or '~').  This means that
the expression could be:

- a numeric literal (e.g., '+5')
- a register displacement (e.g., '-4(%rsp)')
- a subexpression (e.g., '-(2*3)')

So, after saving the sign and moving forward 1 char, now the code needs
to know if there is a digit followed by a register displacement prefix
operand (e.g., '(' on x86_64).  If yes, then it is a register
operation.  If not, then it will be handled recursively, and the code
will later apply the requested operation on the result (either a '+', a
'-' or a '~').

With the bug, the code was correctly discarding the digit (though using
strtol unnecessarily), but it wasn't properly dealing with
subexpressions when the register indirection prefix was '(', like on
x86_64.  This patch also fixes this bug, and includes a testcase.  It
passes on x86_64 Fedora 20.

OK to check it in?

Thanks,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

gdb/ChangeLog:
2014-08-26  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR gdb/17235
	* stap-probe.c (stap_parse_single_operand): Delete unused variable
	'number'.  New variable 'has_digit'.  Rewrite code to deal with
	subexpressions on SDT probes.

gdb/testsuite/ChangeLog:
2014-08-26  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR gdb/17235
	* gdb.arch/amd64-stap-wrong-subexp.exp: New file.
	* gdb.arch/amd64-stap-wrong-subexp.S: Likewise.

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index 84714b5..23202d7 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -753,9 +753,9 @@ stap_parse_single_operand (struct stap_parse_info *p)
   if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+')
     {
       char c = *p->arg;
-      int number;
       /* We use this variable to do a lookahead.  */
       const char *tmp = p->arg;
+      int has_digit = 0;
 
       /* Skipping signal.  */
       ++tmp;
@@ -772,26 +772,19 @@ stap_parse_single_operand (struct stap_parse_info *p)
       if (p->inside_paren_p)
 	tmp = skip_spaces_const (tmp);
 
-      if (isdigit (*tmp))
+      while (isdigit (*tmp))
 	{
-	  char *endp;
-
-	  number = strtol (tmp, &endp, 10);
-	  tmp = endp;
+	  /* We skip the digit here because we are only interested in
+	     knowing what kind of unary operation this is.  The digit
+	     will be handled by one of the functions that will be
+	     called below ('stap_parse_argument_conditionally' or
+	     'stap_parse_register_operand').  */
+	  ++tmp;
+	  has_digit = 1;
 	}
 
-      if (!stap_is_register_indirection_prefix (gdbarch, tmp, NULL))
-	{
-	  /* This is not a displacement.  We skip the operator, and deal
-	     with it later.  */
-	  ++p->arg;
-	  stap_parse_argument_conditionally (p);
-	  if (c == '-')
-	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
-	  else if (c == '~')
-	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
-	}
-      else
+      if (has_digit && stap_is_register_indirection_prefix (gdbarch, tmp,
+							    NULL))
 	{
 	  /* If we are here, it means it is a displacement.  The only
 	     operations allowed here are `-' and `+'.  */
@@ -801,6 +794,17 @@ stap_parse_single_operand (struct stap_parse_info *p)
 
 	  stap_parse_register_operand (p);
 	}
+      else
+	{
+	  /* This is not a displacement.  We skip the operator, and
+	     deal with it when the recursion returns.  */
+	  ++p->arg;
+	  stap_parse_argument_conditionally (p);
+	  if (c == '-')
+	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
+	  else if (c == '~')
+	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
+	}
     }
   else if (isdigit (*p->arg))
     {
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S
new file mode 100644
index 0000000..2848462
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S
@@ -0,0 +1,27 @@
+/* Copyright (C) 2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <sys/sdt.h>
+
+	.file	"amd64-stap-wrong-subexp.S"
+	.text
+	.globl	main
+main:
+	STAP_PROBE1(probe, foo, -4@$-3($4+$3))
+	STAP_PROBE2(probe, bar, -4@-($4), -4@$-3+($22/$2)-$16)
+	xor	%rax,%rax
+	ret
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp
new file mode 100644
index 0000000..5a1ad53
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp
@@ -0,0 +1,41 @@
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if { ![istarget "x86_64-*-*"] || ![is_lp64_target] } {
+    verbose "Skipping amd64-stap-wrong-subexp.exp"
+    return
+}
+
+standard_testfile amd64-stap-wrong-subexp.S
+
+if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
+    untested amd64-stap-wrong-subexp.exp
+    return -1
+}
+
+proc goto_probe { probe_name } {
+    if { ![runto "-pstap $probe_name"] } {
+	fail "run to probe $probe_name"
+	return
+    }
+}
+
+goto_probe foo
+gdb_test "print \$_probe_arg0" "Invalid operator `\\\(' on expression .*" \
+    "print probe foo arg0"
+
+goto_probe bar
+gdb_test "print \$_probe_arg0" " = -4" "print probe bar arg0"
+gdb_test "print \$_probe_arg1" " = -8" "print probe bar arg1"

Re: [PATCH] Fix for PR gdb/17235: possible bug extracting systemtap probe operand

[Sergio Durigan Junior]

On Tuesday, August 26 2014, I wrote:

> Hi there,

Ping.

> This patch is a fix to PR gdb/17235.  The bug is about an unused
> variable that got declared and set during one of the parsing phases of
> an SDT probe's argument.  I took the opportunity to rewrite some of the
> code to improve the parsing.  The bug was actually a thinko, because
> what I wanted to do in the code was to discard the number on the string
> being parsed.
>
> During this portion, the code identifies that it is dealing with an
> expression that begins with a sign ('+', '-' or '~').  This means that
> the expression could be:
>
> - a numeric literal (e.g., '+5')
> - a register displacement (e.g., '-4(%rsp)')
> - a subexpression (e.g., '-(2*3)')
>
> So, after saving the sign and moving forward 1 char, now the code needs
> to know if there is a digit followed by a register displacement prefix
> operand (e.g., '(' on x86_64).  If yes, then it is a register
> operation.  If not, then it will be handled recursively, and the code
> will later apply the requested operation on the result (either a '+', a
> '-' or a '~').
>
> With the bug, the code was correctly discarding the digit (though using
> strtol unnecessarily), but it wasn't properly dealing with
> subexpressions when the register indirection prefix was '(', like on
> x86_64.  This patch also fixes this bug, and includes a testcase.  It
> passes on x86_64 Fedora 20.
>
> OK to check it in?
>
> Thanks,
>
> -- 
> Sergio
> GPG key ID: 0x65FC5E36
> Please send encrypted e-mail if possible
> http://sergiodj.net/
>
> gdb/ChangeLog:
> 2014-08-26  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	PR gdb/17235
> 	* stap-probe.c (stap_parse_single_operand): Delete unused variable
> 	'number'.  New variable 'has_digit'.  Rewrite code to deal with
> 	subexpressions on SDT probes.
>
> gdb/testsuite/ChangeLog:
> 2014-08-26  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	PR gdb/17235
> 	* gdb.arch/amd64-stap-wrong-subexp.exp: New file.
> 	* gdb.arch/amd64-stap-wrong-subexp.S: Likewise.
>
> diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
> index 84714b5..23202d7 100644
> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -753,9 +753,9 @@ stap_parse_single_operand (struct stap_parse_info *p)
>    if (*p->arg == '-' || *p->arg == '~' || *p->arg == '+')
>      {
>        char c = *p->arg;
> -      int number;
>        /* We use this variable to do a lookahead.  */
>        const char *tmp = p->arg;
> +      int has_digit = 0;
>  
>        /* Skipping signal.  */
>        ++tmp;
> @@ -772,26 +772,19 @@ stap_parse_single_operand (struct stap_parse_info *p)
>        if (p->inside_paren_p)
>  	tmp = skip_spaces_const (tmp);
>  
> -      if (isdigit (*tmp))
> +      while (isdigit (*tmp))
>  	{
> -	  char *endp;
> -
> -	  number = strtol (tmp, &endp, 10);
> -	  tmp = endp;
> +	  /* We skip the digit here because we are only interested in
> +	     knowing what kind of unary operation this is.  The digit
> +	     will be handled by one of the functions that will be
> +	     called below ('stap_parse_argument_conditionally' or
> +	     'stap_parse_register_operand').  */
> +	  ++tmp;
> +	  has_digit = 1;
>  	}
>  
> -      if (!stap_is_register_indirection_prefix (gdbarch, tmp, NULL))
> -	{
> -	  /* This is not a displacement.  We skip the operator, and deal
> -	     with it later.  */
> -	  ++p->arg;
> -	  stap_parse_argument_conditionally (p);
> -	  if (c == '-')
> -	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
> -	  else if (c == '~')
> -	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
> -	}
> -      else
> +      if (has_digit && stap_is_register_indirection_prefix (gdbarch, tmp,
> +							    NULL))
>  	{
>  	  /* If we are here, it means it is a displacement.  The only
>  	     operations allowed here are `-' and `+'.  */
> @@ -801,6 +794,17 @@ stap_parse_single_operand (struct stap_parse_info *p)
>  
>  	  stap_parse_register_operand (p);
>  	}
> +      else
> +	{
> +	  /* This is not a displacement.  We skip the operator, and
> +	     deal with it when the recursion returns.  */
> +	  ++p->arg;
> +	  stap_parse_argument_conditionally (p);
> +	  if (c == '-')
> +	    write_exp_elt_opcode (&p->pstate, UNOP_NEG);
> +	  else if (c == '~')
> +	    write_exp_elt_opcode (&p->pstate, UNOP_COMPLEMENT);
> +	}
>      }
>    else if (isdigit (*p->arg))
>      {
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S
> new file mode 100644
> index 0000000..2848462
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.S
> @@ -0,0 +1,27 @@
> +/* Copyright (C) 2014 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <sys/sdt.h>
> +
> +	.file	"amd64-stap-wrong-subexp.S"
> +	.text
> +	.globl	main
> +main:
> +	STAP_PROBE1(probe, foo, -4@$-3($4+$3))
> +	STAP_PROBE2(probe, bar, -4@-($4), -4@$-3+($22/$2)-$16)
> +	xor	%rax,%rax
> +	ret
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp
> new file mode 100644
> index 0000000..5a1ad53
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-wrong-subexp.exp
> @@ -0,0 +1,41 @@
> +# Copyright 2014 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if { ![istarget "x86_64-*-*"] || ![is_lp64_target] } {
> +    verbose "Skipping amd64-stap-wrong-subexp.exp"
> +    return
> +}
> +
> +standard_testfile amd64-stap-wrong-subexp.S
> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
> +    untested amd64-stap-wrong-subexp.exp
> +    return -1
> +}
> +
> +proc goto_probe { probe_name } {
> +    if { ![runto "-pstap $probe_name"] } {
> +	fail "run to probe $probe_name"
> +	return
> +    }
> +}
> +
> +goto_probe foo
> +gdb_test "print \$_probe_arg0" "Invalid operator `\\\(' on expression .*" \
> +    "print probe foo arg0"
> +
> +goto_probe bar
> +gdb_test "print \$_probe_arg0" " = -4" "print probe bar arg0"
> +gdb_test "print \$_probe_arg1" " = -8" "print probe bar arg1"

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Fix for PR gdb/17235: possible bug extracting systemtap probe operand

[Joel Brobecker]

Hi Sergio,

> > gdb/ChangeLog:
> > 2014-08-26  Sergio Durigan Junior  <sergiodj@redhat.com>
> >
> > 	PR gdb/17235
> > 	* stap-probe.c (stap_parse_single_operand): Delete unused variable
> > 	'number'.  New variable 'has_digit'.  Rewrite code to deal with
> > 	subexpressions on SDT probes.
> >
> > gdb/testsuite/ChangeLog:
> > 2014-08-26  Sergio Durigan Junior  <sergiodj@redhat.com>
> >
> > 	PR gdb/17235
> > 	* gdb.arch/amd64-stap-wrong-subexp.exp: New file.
> > 	* gdb.arch/amd64-stap-wrong-subexp.S: Likewise.

Sorry for the delay. This is OK.

Thank you,
-- 
Joel

Re: [PATCH] Fix for PR gdb/17235: possible bug extracting systemtap probe operand

[Sergio Durigan Junior]

On Friday, September 05 2014, Joel Brobecker wrote:

> Sorry for the delay. This is OK.

No problem at all.  Thank you, Joel.

Fix pushed.

  <https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=474ca4f6871d4addb7ce6a177245bce79c89550e>
  <https://sourceware.org/ml/gdb-cvs/2014-09/msg00015.html>

Cheers,

-- 
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/