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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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