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

Mark Kettenis
2014-01-30T15:36:03Z

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu, 30 Jan 2014 13:16:15 -0200
>
> On Friday, January 17 2014, I wrote:
>
> > 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.
>
> Ping^2.

No objection to this going in (other than the unsafe use of
isdigit(3)) but I don't claim to know wnything of the SDT stuff.

> >> 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
>
> --
> Sergio
>