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

Sergio Durigan Junior
2014-09-02T16:33:26Z

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"