This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]