This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]