Re: [PATCH] Fix for PR tdep/16397: SystemTap SDT probe support for x86 doesn't work with "triplet operands"

Sergio Durigan Junior
2014-01-17T21:31:38Z

On Sunday, January 12 2014, I wrote:

> Hi,
>
> This is the continuation of what Joel proposed on:
>
> <https://sourceware.org/ml/gdb-patches/2013-12/msg00977.html>

Ping.

> Now that I have already submitted and pushed the patch to split
> i386_stap_parse_special_token into two smaller functions, it is indeed
> simpler to understand this patch.
>
> It occurs because, on x86, triplet displacement operands are allowed
> (like "-4+8-20(%rbp)"), and the current parser for this expression is
> buggy. It does not correctly extract the register name from the
> expression, which leads to incorrect evaluation. The parser was also
> being very "generous" with the expression, so I included a few more
> checks to ensure that we're indeed dealing with a triplet displacement
> operand.
>
> This patch also includes testcases for the two different kind of
> expressions that can be encountered on x86: the triplet displacement
> (explained above) and the three-argument displacement (as in
> "(%rbx,%ebx,-8)"). The tests are obviously arch-dependent and are
> placed under gdb.arch/.
>
> OK to apply?
>
> --
> Sergio
>
> gdb/
> 2014-01-12 Sergio Durigan Junior <sergiodj@redhat.com>
>
> PR tdep/16397
> * i386-tdep.c (i386_stap_parse_special_token_triplet): Check if a
> number comes after the + or - signs. Adjust length of register
> name to be extracted.
>
> gdb/testsuite
> 2014-01-12 Sergio Durigan Junior <sergiodj@redhat.com>
>
> PR tdep/16397
> * gdb.arch/amd64-stap-special-operands.exp: New file.
> * gdb.arch/amd64-stap-three-arg-disp.S: Likewise.
> * gdb.arch/amd64-stap-three-arg-disp.c: Likewise.
> * gdb.arch/amd64-stap-triplet.S: Likewise.
> * gdb.arch/amd64-stap-triplet.c: Likewise.
>
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 9d1d9e0..26d5d8f 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -3639,6 +3639,9 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch,
> got_minus[0] = 1;
> }
>
> + if (!isdigit (*s))
> + return 0;
> +
> displacements[0] = strtol (s, &endp, 10);
> s = endp;
>
> @@ -3657,6 +3660,9 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch,
> got_minus[1] = 1;
> }
>
> + if (!isdigit (*s))
> + return 0;
> +
> displacements[1] = strtol (s, &endp, 10);
> s = endp;
>
> @@ -3675,6 +3681,9 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch,
> got_minus[2] = 1;
> }
>
> + if (!isdigit (*s))
> + return 0;
> +
> displacements[2] = strtol (s, &endp, 10);
> s = endp;
>
> @@ -3690,7 +3699,7 @@ i386_stap_parse_special_token_triplet (struct gdbarch *gdbarch,
> if (*s++ != ')')
> return 0;
>
> - len = s - start;
> + len = s - start - 1;
> regname = alloca (len + 1);
>
> strncpy (regname, start, len);
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-special-operands.exp b/gdb/testsuite/gdb.arch/amd64-stap-special-operands.exp
> new file mode 100644
> index 0000000..f80dfdf
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-special-operands.exp
> @@ -0,0 +1,47 @@
> +# Copyright 2013-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-*-*"] && ![istarget "i?86-*-*"] } {
> + verbose "Skipping amd64-stap-special-operands.exp"
> + return
> +}
> +
> +proc test_probe { probe_name } {
> + if { ![runto "-pstap $probe_name"] } {
> + fail "run to probe $probe_name"
> + return
> + }
> +
> + gdb_test "print \$_probe_argc" " = 1"
> + gdb_test "print \$_probe_arg0" " = 10"
> +}
> +
> +standard_testfile amd64-stap-triplet.S
> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
> + untested amd64-stap-special-operands.exp
> + return -1
> +}
> +
> +test_probe "triplet"
> +
> +standard_testfile amd64-stap-three-arg-disp.S
> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
> + untested amd64-stap-special-operands.exp
> + return -1
> +}
> +
> +test_probe "three_arg"
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.S b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.S
> new file mode 100644
> index 0000000..1e2905c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.S
> @@ -0,0 +1,91 @@
> +/* Copyright (C) 2013-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/>.
> +
> + This file was generated from the equivalent .c file using the
> + following command:
> +
> + #> gcc -S amd64-stap-three-arg-disp.c -o amd64-stap-three-arg-disp.S
> +
> + Then, the SystemTap SDT probe definition below was tweaked. See below
> + for more details. */
> +
> + .file "amd64-stap-three-arg-disp.c"
> + .text
> + .globl main
> + .type main, @function
> +main:
> +.LFB0:
> + .cfi_startproc
> +# BLOCK 2 seq:0
> +# PRED: ENTRY (fallthru)
> + pushq %rbp
> + .cfi_def_cfa_offset 16
> + .cfi_offset 6, -16
> + movq %rsp, %rbp
> + .cfi_def_cfa_register 6
> + movl %edi, -20(%rbp)
> + movq %rsi, -32(%rbp)
> + movl $10, -4(%rbp)
> +#APP
> +# 8 "amd64-stap-three-arg-disp.c" 1
> + 990: nop
> +.pushsection .note.stapsdt,"?","note"
> +.balign 4
> +.4byte 992f-991f,994f-993f,3
> +991: .asciz "stapsdt"
> +992: .balign 4
> +993: .8byte 990b
> +.8byte _.stapsdt.base
> +.8byte 0
> +.asciz "test"
> +.asciz "three_arg"
> +/* The probe's argument definition below was tweaked in order to mimic a
> + three arg displacement in x86 asm. The original probe argument was:
> +
> + -4@-4(%rbp)
> +
> + The argument below is equivalent to that. */
> +.asciz "-4@-4(%rbp,%ebx,0)"
> +994: .balign 4
> +.popsection
> +
> +# 0 "" 2
> +# 8 "amd64-stap-three-arg-disp.c" 1
> + .ifndef _.stapsdt.base
> +.pushsection .stapsdt.base,"aG","progbits",.stapsdt.base,comdat
> +.weak _.stapsdt.base
> +.hidden _.stapsdt.base
> +_.stapsdt.base: .space 1
> +.size _.stapsdt.base,1
> +.popsection
> +.endif
> +
> +# 0 "" 2
> +#NO_APP
> + movl $0, %eax
> +/* Here, we zero-out %ebx in order to use it safely when evaluating
> + the probe argument. */
> + movl $0, %ebx
> + popq %rbp
> + .cfi_def_cfa 7, 8
> +# SUCC: EXIT [100.0%]
> + ret
> + .cfi_endproc
> +.LFE0:
> + .size main, .-main
> + .ident "GCC: (GNU) 4.7.2 20120921 (Red Hat 4.7.2-2)"
> + .section .note.GNU-stack,"",@progbits
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.c b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.c
> new file mode 100644
> index 0000000..666e5ec
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-three-arg-disp.c
> @@ -0,0 +1,31 @@
> +/* Copyright (C) 2013-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/>.
> +
> + This file is not used directly. Please, see the equivalent .S file
> + for more details. */
> +
> +#include <sys/sdt.h>
> +
> +int
> +main (int argc, char *argv[])
> +{
> + int a = 10;
> +
> + STAP_PROBE1 (test, three_arg, a);
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-triplet.S b/gdb/testsuite/gdb.arch/amd64-stap-triplet.S
> new file mode 100644
> index 0000000..be22250
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-triplet.S
> @@ -0,0 +1,88 @@
> +/* Copyright (C) 2013-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/>.
> +
> + This file was generated from the equivalent .c file using the
> + following command:
> +
> + #> gcc -S amd64-stap-triplet.c -o amd64-stap-triplet.S
> +
> + Then, the SystemTap SDT probe definition below was tweaked. See below
> + for more details. */
> +
> + .file "amd64-stap-triplet.c"
> + .text
> + .globl main
> + .type main, @function
> +main:
> +.LFB0:
> + .cfi_startproc
> +# BLOCK 2 seq:0
> +# PRED: ENTRY (fallthru)
> + pushq %rbp
> + .cfi_def_cfa_offset 16
> + .cfi_offset 6, -16
> + movq %rsp, %rbp
> + .cfi_def_cfa_register 6
> + movl %edi, -20(%rbp)
> + movq %rsi, -32(%rbp)
> + movl $10, -4(%rbp)
> +#APP
> +# 8 "amd64-stap-triplet.c" 1
> + 990: nop
> +.pushsection .note.stapsdt,"?","note"
> +.balign 4
> +.4byte 992f-991f,994f-993f,3
> +991: .asciz "stapsdt"
> +992: .balign 4
> +993: .8byte 990b
> +.8byte _.stapsdt.base
> +.8byte 0
> +.asciz "test"
> +.asciz "triplet"
> +/* The probe's argument definition below was tweaked in order to mimic a
> + triplet displacement in x86 asm. The original probe argument was:
> +
> + -4@-4(%rbp)
> +
> + The argument below is equivalent to that. */
> +.asciz "-4@-4+16-16(%rbp)"
> +994: .balign 4
> +.popsection
> +
> +# 0 "" 2
> +# 8 "amd64-stap-triplet.c" 1
> + .ifndef _.stapsdt.base
> +.pushsection .stapsdt.base,"aG","progbits",.stapsdt.base,comdat
> +.weak _.stapsdt.base
> +.hidden _.stapsdt.base
> +_.stapsdt.base: .space 1
> +.size _.stapsdt.base,1
> +.popsection
> +.endif
> +
> +# 0 "" 2
> +#NO_APP
> + movl $0, %eax
> + popq %rbp
> + .cfi_def_cfa 7, 8
> +# SUCC: EXIT [100.0%]
> + ret
> + .cfi_endproc
> +.LFE0:
> + .size main, .-main
> + .ident "GCC: (GNU) 4.7.2 20120921 (Red Hat 4.7.2-2)"
> + .section .note.GNU-stack,"",@progbits
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-triplet.c b/gdb/testsuite/gdb.arch/amd64-stap-triplet.c
> new file mode 100644
> index 0000000..0ae2730
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-triplet.c
> @@ -0,0 +1,31 @@
> +/* Copyright (C) 2013-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/>.
> +
> + This file is not used directly. Please, see the equivalent .S file
> + for more details. */
> +
> +#include <sys/sdt.h>
> +
> +int
> +main (int argc, char *argv[])
> +{
> + int a = 10;
> +
> + STAP_PROBE1 (test, triplet, a);
> +
> + return 0;
> +}

--
Sergio