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

Sergio Durigan Junior
2014-01-30T15:16:26Z

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.

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