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