[PATCH 0/2] Fix a bug and extend SDT probe parser

[PATCH 0/2] Fix a bug and extend SDT probe parser

[Sergio Durigan Junior]

Hi,

This series was actually inspired by PR breakpoints/16889.  The first
commit fixes a bug that is happening when a probe argument does not
provide a prefix (i.e., the "N@" part).  There is a description in the
e-mail.  The second commit actually extends the SDT parser in order to
recognize other possible values for the argument bitness.

OK to apply?

[PATCH 1/2] Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments

[Sergio Durigan Junior]

This commit fixes PR breakpoints/16889, which is about a bug that
triggers when GDB tries to parse probes whose arguments do not contain
the initial (and optional) "N@" part.  For reference sake, the de
facto format is described here:

  <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>

Anyway, this PR actually uncovered two bugs (related) that were
happening while parsing the arguments.  The first one was that the
parser *was* catching *some* arguments that were missing the "N@"
part, but it wasn't correctly setting the argument's type.  This was
causing a NULL pointer being dereferenced, ouch...

The second bug uncovered was that the parser was not catching all of
the cases for a probe which did not provide the "N@" part.  The fix
for that was to simplify the check that the code was making to
identify non-prefixed probes.  The code is simpler and easier to read
now.

I am also providing a testcase for this bug, only for x86_64
architectures.

gdb/
2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/16889
	* stap-probe.c (stap_parse_probe_arguments): Simplify
	check for non-prefixed probes (i.e., probes whose
	arguments do not start with "N@").  Always set the
	argument type to a sane value.

gdb/testsuite/
2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR breakpoints/16889
	* gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S: New file.
	* gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp: Likewise.
---
 gdb/stap-probe.c                                   | 11 ++---
 .../gdb.arch/amd64-stap-optional-prefix.S          | 29 +++++++++++++
 .../gdb.arch/amd64-stap-optional-prefix.exp        | 50 ++++++++++++++++++++++
 3 files changed, 85 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
 create mode 100644 gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index dbe9f31..ef45495 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -1098,10 +1098,8 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	 Where `N' can be [+,-][4,8].  This is not mandatory, so
 	 we check it here.  If we don't find it, go to the next
 	 state.  */
-      if ((*cur == '-' && cur[1] != '\0' && cur[2] != '@')
-	  && cur[1] != '@')
-	arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
-      else
+      if ((cur[0] == '-' && isdigit (cur[1]) && cur[2] == '@')
+	  || (isdigit (cur[0]) && cur[1] == '@'))
 	{
 	  if (*cur == '-')
 	    {
@@ -1127,11 +1125,14 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	    }
 
 	  arg.bitness = b;
-	  arg.atype = stap_get_expected_argument_type (gdbarch, b);
 
 	  /* Discard the number and the `@' sign.  */
 	  cur += 2;
 	}
+      else
+	arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
+
+      arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness);
 
       expr = stap_parse_argument (&cur, arg.atype, gdbarch);
 
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
new file mode 100644
index 0000000..9c86f2d
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
@@ -0,0 +1,29 @@
+/* Copyright (C) 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/>.  */
+
+#include <sys/sdt.h>
+	.file	"amd64-stap-optional-prefix.S"
+	.text
+	.globl	main
+main:
+	mov	%rsp,%rbp
+	pushq	$42
+	STAP_PROBE1(probe, foo, (%rsp))
+	STAP_PROBE1(probe, bar, -8(%rbp))
+	mov	%rbp,%rsp
+	xor	%rax,%rax
+	ret
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
new file mode 100644
index 0000000..9747dc8
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
@@ -0,0 +1,50 @@
+# Copyright 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/>.
+
+# This testcase is for PR breakpoints/16889
+
+if { ![istarget "x86_64-*-*"] } {
+    verbose "Skipping amd64-stap-special-operands.exp"
+    return
+}
+
+standard_testfile ".S"
+
+if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
+    untested amd64-stap-optional-prefix.exp
+    return -1
+}
+
+# Run to the first probe (foo).
+
+if { ![runto "-pstap foo"] } {
+    fail "run to probe foo"
+    return
+}
+
+# Probe foo's first argument must me == $rsp
+
+gdb_test "print \$_probe_argc" "= 1"
+gdb_test "print \$_probe_arg0" "= 42"
+gdb_test "print \$_probe_arg0 == *((unsigned int *) \$rsp)" "= 1"
+
+# Continuing to the second probe (bar)
+
+gdb_test "break -pstap bar" "Breakpoint $decimal at $hex"
+gdb_test "continue" "Breakpoint $decimal, main \\(\\) at .*\r\n.*STAP_PROBE1.*bar,.*\\)"
+
+gdb_test "print \$_probe_argc" "= 1"
+gdb_test "print \$_probe_arg0" "= 42"
+gdb_test "print \$_probe_arg0 == *((unsigned int *) (\$rbp - 8))" "= 1"
-- 
1.7.11.7

Re: [PATCH 1/2] Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments

[Pedro Alves]

On 05/01/2014 10:52 PM, Sergio Durigan Junior wrote:

> gdb/testsuite/
> 2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR breakpoints/16889
> 	* gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S: New file.
> 	* gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp: Likewise.

No "gdb/testsuite/" in the entries.

> ---

l-prefix.S b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
> new file mode 100644
> index 0000000..9c86f2d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
> @@ -0,0 +1,29 @@
> +/* Copyright (C) 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/>.  */
> +
> +#include <sys/sdt.h>
> +	.file	"amd64-stap-optional-prefix.S"

I think a line break after the include would read better.

> +	.text
> +	.globl	main
> +main:



> +	mov	%rsp,%rbp
> +	pushq	$42
> +	STAP_PROBE1(probe, foo, (%rsp))
> +	STAP_PROBE1(probe, bar, -8(%rbp))

What controls whether the optional prefix is included?  Could
we perhaps add two extra probes that probe the same values,
but use the optional prefix?  Something to the effect of:

	STAP_PROBE1(probe, foo, (%rsp))
	STAP_PROBE1(probe, bar, -8(%rbp))
	STAP_PROBE1(probe, foo_prefix, 8@(%rsp))
	STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp))

(but I'm clueless on how that is actually written.)


> +	mov	%rbp,%rsp
> +	xor	%rax,%rax
> +	ret
> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
> new file mode 100644
> index 0000000..9747dc8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
> @@ -0,0 +1,50 @@
> +# Copyright 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/>.
> +
> +# This testcase is for PR breakpoints/16889
> +
> +if { ![istarget "x86_64-*-*"] } {
> +    verbose "Skipping amd64-stap-special-operands.exp"
> +    return
> +}
> +
> +standard_testfile ".S"

If you swap these, you can use $testfile:

standard_testfile ".S"

if { ![istarget "x86_64-*-*"] } {
    verbose "Skipping $testfile.exp"
    return
}

> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
> +    untested amd64-stap-optional-prefix.exp
> +    return -1
> +}

Here too.  But, prepare_for_testing already calls untested for
you, using the text passed as first argument.  Write:

if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
    return -1
}

> +
> +# Run to the first probe (foo).
> +
> +if { ![runto "-pstap foo"] } {
> +    fail "run to probe foo"
> +    return
> +}
> +
> +# Probe foo's first argument must me == $rsp

s/me/be ?  I think you meant:

 # Probe foo's first argument must be == ($rsp)

Might even make it easier to read to spell that out
in plain English.

Otherwise this looks good to me.

-- 
Pedro Alves

Re: [PATCH 1/2] Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments

[Sergio Durigan Junior]

[-- Attachment #1: Type: text/plain, Size: 3862 bytes --]

On Friday, May 02 2014, Pedro Alves wrote:

> On 05/01/2014 10:52 PM, Sergio Durigan Junior wrote:
>
>> gdb/testsuite/
>> 2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	PR breakpoints/16889
>> 	* gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S: New file.
>> 	* gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp: Likewise.
>
> No "gdb/testsuite/" in the entries.

Ops, typo, sorry.

>> +#include <sys/sdt.h>
>> +	.file	"amd64-stap-optional-prefix.S"
>
> I think a line break after the include would read better.

Done.

>> +	.text
>> +	.globl	main
>> +main:
>
>
>
>> +	mov	%rsp,%rbp
>> +	pushq	$42
>> +	STAP_PROBE1(probe, foo, (%rsp))
>> +	STAP_PROBE1(probe, bar, -8(%rbp))
>
> What controls whether the optional prefix is included?  Could
> we perhaps add two extra probes that probe the same values,
> but use the optional prefix?  Something to the effect of:
>
> 	STAP_PROBE1(probe, foo, (%rsp))
> 	STAP_PROBE1(probe, bar, -8(%rbp))
> 	STAP_PROBE1(probe, foo_prefix, 8@(%rsp))
> 	STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp))
>
> (but I'm clueless on how that is actually written.)

If the asm is generated by the compiler, than it is almost guaranteed
that the optional prefix will be included.  However, since it is
optional, if it is a hand-written asm then the user might choose to omit
it.

Extending the test as you mentioned is OK, but I chose not to do it
because the prefix-variant probes are already tested at
gdb.base/stap-probe.exp.

Either way, I am including them now (and extending the testcase).

>> diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
>> new file mode 100644
>> index 0000000..9747dc8
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
>> @@ -0,0 +1,50 @@
>> +# Copyright 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/>.
>> +
>> +# This testcase is for PR breakpoints/16889
>> +
>> +if { ![istarget "x86_64-*-*"] } {
>> +    verbose "Skipping amd64-stap-special-operands.exp"
>> +    return
>> +}
>> +
>> +standard_testfile ".S"
>
> If you swap these, you can use $testfile:
>
> standard_testfile ".S"
>
> if { ![istarget "x86_64-*-*"] } {
>     verbose "Skipping $testfile.exp"
>     return
> }

Done.

>> +
>> +if { [prepare_for_testing $testfile.exp $testfile $srcfile] } {
>> +    untested amd64-stap-optional-prefix.exp
>> +    return -1
>> +}
>
> Here too.  But, prepare_for_testing already calls untested for
> you, using the text passed as first argument.  Write:
>
> if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>     return -1
> }

Somehow I didn't know about it.  What a lame!  Anyway, done.

>> +
>> +# Run to the first probe (foo).
>> +
>> +if { ![runto "-pstap foo"] } {
>> +    fail "run to probe foo"
>> +    return
>> +}
>> +
>> +# Probe foo's first argument must me == $rsp
>
> s/me/be ?  I think you meant:
>
>  # Probe foo's first argument must be == ($rsp)
>
> Might even make it easier to read to spell that out
> in plain English.
>
> Otherwise this looks good to me.

Done, thanks.

Here's what I am going to push by the end of the day if there are no
other comments.

-- 
Sergio


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 11.patch --]
[-- Type: text/x-patch, Size: 6426 bytes --]

commit c7c77ebc3aad10cbf7be91e09de839bccdbf06ca
Author: Sergio Durigan Junior <sergiodj@redhat.com>
Date:   Thu May 1 18:20:05 2014 -0300

    Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments
    
    This commit fixes PR breakpoints/16889, which is about a bug that
    triggers when GDB tries to parse probes whose arguments do not contain
    the initial (and optional) "N@" part.  For reference sake, the de
    facto format is described here:
    
      <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>
    
    Anyway, this PR actually uncovered two bugs (related) that were
    happening while parsing the arguments.  The first one was that the
    parser *was* catching *some* arguments that were missing the "N@"
    part, but it wasn't correctly setting the argument's type.  This was
    causing a NULL pointer being dereferenced, ouch...
    
    The second bug uncovered was that the parser was not catching all of
    the cases for a probe which did not provide the "N@" part.  The fix
    for that was to simplify the check that the code was making to
    identify non-prefixed probes.  The code is simpler and easier to read
    now.
    
    I am also providing a testcase for this bug, only for x86_64
    architectures.
    
    gdb/
    2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
    
    	PR breakpoints/16889
    	* stap-probe.c (stap_parse_probe_arguments): Simplify
    	check for non-prefixed probes (i.e., probes whose
    	arguments do not start with "N@").  Always set the
    	argument type to a sane value.
    
    gdb/testsuite/
    2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
    
    	PR breakpoints/16889
    	* gdb.arch/amd64-stap-optional-prefix.S: New file.
    	* gdb.arch/amd64-stap-optional-prefix.exp: Likewise.

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index dbe9f31..ef45495 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -1098,10 +1098,8 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	 Where `N' can be [+,-][4,8].  This is not mandatory, so
 	 we check it here.  If we don't find it, go to the next
 	 state.  */
-      if ((*cur == '-' && cur[1] != '\0' && cur[2] != '@')
-	  && cur[1] != '@')
-	arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
-      else
+      if ((cur[0] == '-' && isdigit (cur[1]) && cur[2] == '@')
+	  || (isdigit (cur[0]) && cur[1] == '@'))
 	{
 	  if (*cur == '-')
 	    {
@@ -1127,11 +1125,14 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	    }
 
 	  arg.bitness = b;
-	  arg.atype = stap_get_expected_argument_type (gdbarch, b);
 
 	  /* Discard the number and the `@' sign.  */
 	  cur += 2;
 	}
+      else
+	arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
+
+      arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness);
 
       expr = stap_parse_argument (&cur, arg.atype, gdbarch);
 
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
new file mode 100644
index 0000000..66689cb
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
@@ -0,0 +1,32 @@
+/* Copyright (C) 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/>.  */
+
+#include <sys/sdt.h>
+
+	.file	"amd64-stap-optional-prefix.S"
+	.text
+	.globl	main
+main:
+	mov	%rsp,%rbp
+	pushq	$42
+	STAP_PROBE1(probe, foo, (%rsp))
+	STAP_PROBE1(probe, bar, -8(%rbp))
+	STAP_PROBE1(probe, foo_prefix, 8@(%rsp))
+	STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp))
+	mov	%rbp,%rsp
+	xor	%rax,%rax
+	ret
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
new file mode 100644
index 0000000..c69e54c
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
@@ -0,0 +1,68 @@
+# Copyright 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/>.
+
+# This testcase is for PR breakpoints/16889
+
+standard_testfile ".S"
+
+if { ![istarget "x86_64-*-*"] } {
+    verbose "Skipping $testfile.exp"
+    return
+}
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+# Helper procedure to go to probe NAME
+
+proc goto_probe { name } {
+    global decimal hex
+
+    gdb_test "break -pstap $name" "Breakpoint $decimal at $hex"
+    gdb_test "continue" "Breakpoint $decimal, main \\(\\) at .*\r\n.*STAP_PROBE1.*${name},.*\\)"
+}
+
+# Helper procedure to test the probe's argument
+
+proc test_probe_value { value reg_val } {
+    gdb_test "print \$_probe_argc" "= 1"
+    gdb_test "print \$_probe_arg0" "= $value"
+    gdb_test "print \$_probe_arg0 == *((unsigned int *) (${reg_val}))" "= 1"
+}
+
+# Run to the first probe (foo).
+
+if { ![runto "-pstap foo"] } {
+    fail "run to probe foo"
+    return
+}
+
+# Probe foo's first argument must be $rsp
+
+test_probe_value "42" "\$rsp"
+
+# Continuing to the second probe (bar)
+
+goto_probe "bar"
+test_probe_value "42" "\$rbp - 8"
+
+# Now, testing the prefixed probes
+
+goto_probe "foo_prefix"
+test_probe_value "42" "\$rsp"
+
+goto_probe "bar_prefix"
+test_probe_value "42" "\$rbp - 8"

Re: [PATCH 1/2] Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments

[Sergio Durigan Junior]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

On Friday, May 02 2014, I wrote:

> Here's what I am going to push by the end of the day if there are no
> other comments.

Pedro brought something else to my attention: the testcase messages were
not unique, and I could rewrite it in order to make it more
automatized.  So here is the third version that I will push if there are
no other comments.

Thanks,

-- 
Sergio


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 11.patch --]
[-- Type: text/x-patch, Size: 6321 bytes --]

commit 00412478d10a782e298d9be457a1680cbb6c07c8
Author: Sergio Durigan Junior <sergiodj@redhat.com>
Date:   Thu May 1 18:20:05 2014 -0300

    Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments
    
    This commit fixes PR breakpoints/16889, which is about a bug that
    triggers when GDB tries to parse probes whose arguments do not contain
    the initial (and optional) "N@" part.  For reference sake, the de
    facto format is described here:
    
      <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>
    
    Anyway, this PR actually uncovered two bugs (related) that were
    happening while parsing the arguments.  The first one was that the
    parser *was* catching *some* arguments that were missing the "N@"
    part, but it wasn't correctly setting the argument's type.  This was
    causing a NULL pointer being dereferenced, ouch...
    
    The second bug uncovered was that the parser was not catching all of
    the cases for a probe which did not provide the "N@" part.  The fix
    for that was to simplify the check that the code was making to
    identify non-prefixed probes.  The code is simpler and easier to read
    now.
    
    I am also providing a testcase for this bug, only for x86_64
    architectures.
    
    gdb/
    2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
    
    	PR breakpoints/16889
    	* stap-probe.c (stap_parse_probe_arguments): Simplify
    	check for non-prefixed probes (i.e., probes whose
    	arguments do not start with "N@").  Always set the
    	argument type to a sane value.
    
    gdb/testsuite/
    2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
    
    	PR breakpoints/16889
    	* gdb.arch/amd64-stap-optional-prefix.S: New file.
    	* gdb.arch/amd64-stap-optional-prefix.exp: Likewise.

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index dbe9f31..ef45495 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -1098,10 +1098,8 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	 Where `N' can be [+,-][4,8].  This is not mandatory, so
 	 we check it here.  If we don't find it, go to the next
 	 state.  */
-      if ((*cur == '-' && cur[1] != '\0' && cur[2] != '@')
-	  && cur[1] != '@')
-	arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
-      else
+      if ((cur[0] == '-' && isdigit (cur[1]) && cur[2] == '@')
+	  || (isdigit (cur[0]) && cur[1] == '@'))
 	{
 	  if (*cur == '-')
 	    {
@@ -1127,11 +1125,14 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	    }
 
 	  arg.bitness = b;
-	  arg.atype = stap_get_expected_argument_type (gdbarch, b);
 
 	  /* Discard the number and the `@' sign.  */
 	  cur += 2;
 	}
+      else
+	arg.bitness = STAP_ARG_BITNESS_UNDEFINED;
+
+      arg.atype = stap_get_expected_argument_type (gdbarch, arg.bitness);
 
       expr = stap_parse_argument (&cur, arg.atype, gdbarch);
 
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
new file mode 100644
index 0000000..66689cb
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
@@ -0,0 +1,32 @@
+/* Copyright (C) 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/>.  */
+
+#include <sys/sdt.h>
+
+	.file	"amd64-stap-optional-prefix.S"
+	.text
+	.globl	main
+main:
+	mov	%rsp,%rbp
+	pushq	$42
+	STAP_PROBE1(probe, foo, (%rsp))
+	STAP_PROBE1(probe, bar, -8(%rbp))
+	STAP_PROBE1(probe, foo_prefix, 8@(%rsp))
+	STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp))
+	mov	%rbp,%rsp
+	xor	%rax,%rax
+	ret
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
new file mode 100644
index 0000000..cc9d6c3
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
@@ -0,0 +1,57 @@
+# Copyright 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/>.
+
+# This testcase is for PR breakpoints/16889
+
+standard_testfile ".S"
+
+if { ![istarget "x86_64-*-*"] } {
+    verbose "Skipping $testfile.exp"
+    return
+}
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+# Helper procedure to go to probe NAME
+
+proc goto_probe { name } {
+    global decimal hex
+
+    gdb_test "break -pstap $name" "Breakpoint $decimal at $hex"
+    gdb_test "continue" "Breakpoint $decimal, main \\(\\) at .*\r\n.*STAP_PROBE1.*${name},.*\\)"
+}
+
+# Helper procedure to test the probe's argument
+
+proc test_probe_value { value reg_val } {
+    gdb_test "print \$_probe_argc" "= 1"
+    gdb_test "print \$_probe_arg0" "= $value"
+    gdb_test "print \$_probe_arg0 == *((unsigned int *) (${reg_val}))" "= 1"
+}
+
+if { ![runto_main] } {
+    return -1
+}
+
+foreach probe_name [list "foo" "bar" "foo_prefix" "bar_prefix"] \
+    probe_val [list "42" "42" "42" "42"] \
+    probe_reg_val [list "\$rsp" "\$rbp - 8" "\$rsp" "\$rbp - 8"] {
+    with_test_prefix $probe_name {
+	goto_probe $probe_name
+	test_probe_value $probe_val $probe_reg_val
+    }
+}

Re: [PATCH 1/2] Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments

[Pedro Alves]

On 05/02/2014 07:06 PM, Sergio Durigan Junior wrote:
> On Friday, May 02 2014, I wrote:
> 
>> Here's what I am going to push by the end of the day if there are no
>> other comments.
> 
> Pedro brought something else to my attention: the testcase messages were
> not unique, and I could rewrite it in order to make it more
> automatized.  So here is the third version that I will push if there are
> no other comments.

Thanks!  Looks great.

FYI, with {} instead of [list ...] one can avoid the backslashes.
That is, one can write:

 foreach probe_name {foo bar foo_prefix bar_prefix} \
     probe_val {42 42 42 42} \
     probe_reg_val {"$rsp" "$rbp - 8" "$rsp" "$rbp - 8"} {

instead of:

 foreach probe_name [list "foo" "bar" "foo_prefix" "bar_prefix"] \
     probe_val [list "42" "42" "42" "42"] \
     probe_reg_val [list "\$rsp" "\$rbp - 8" "\$rsp" "\$rbp - 8"] {
     with_test_prefix $probe_name {

Just a suggestion, it doesn't matter at all.

-- 
Pedro Alves

Re: [PATCH 1/2] Fix PR breakpoints/16889: gdb segfaults when printing ASM SDT arguments

[Sergio Durigan Junior]

On Friday, May 02 2014, Pedro Alves wrote:

> FYI, with {} instead of [list ...] one can avoid the backslashes.
> That is, one can write:
>
>  foreach probe_name {foo bar foo_prefix bar_prefix} \
>      probe_val {42 42 42 42} \
>      probe_reg_val {"$rsp" "$rbp - 8" "$rsp" "$rbp - 8"} {
>
> instead of:
>
>  foreach probe_name [list "foo" "bar" "foo_prefix" "bar_prefix"] \
>      probe_val [list "42" "42" "42" "42"] \
>      probe_reg_val [list "\$rsp" "\$rbp - 8" "\$rsp" "\$rbp - 8"] {
>      with_test_prefix $probe_name {
>
> Just a suggestion, it doesn't matter at all.

Thanks for the suggestion.  I actually knew about it, but believe it or
not I prefer to use [list ...] instead of { } in this case, because I
think it is more elegant.  Go figure...

Anyway, pushed:

  <https://sourceware.org/ml/gdb-cvs/2014-05/msg00004.html>

Thanks,

-- 
Sergio

[PATCH 2/2] Extend recognized types of SDT probe's arguments

[Sergio Durigan Junior]

This commit is actually an update to make the parser in
gdb/stap-probe.c be aware of all the possible prefixes that a probe
argument can have.  According to the section "Argument Format" in:

  <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>

The bitness of the arguments can be 8, 16, 32 or 64 bits, signed or
unsigned.  Currently GDB recognizes only 32 and 64-bit arguments.
This commit extends this.  Since this is a straightforward extension,
I am not submitting a testcase; I can do that if anybody wants.

gdb/
2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	stap-probe.c (enum stap_arg_bitness): New enums to represent 8
	and 16-bit signed and unsigned arguments.
	(stap_parse_probe_arguments): Extend code to handle such
	arguments.
---
 gdb/stap-probe.c | 62 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index ef45495..bc7f54a 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -68,6 +68,10 @@ static unsigned int stap_expression_debug = 0;
 enum stap_arg_bitness
 {
   STAP_ARG_BITNESS_UNDEFINED,
+  STAP_ARG_BITNESS_8BIT_UNSIGNED,
+  STAP_ARG_BITNESS_8BIT_SIGNED,
+  STAP_ARG_BITNESS_16BIT_UNSIGNED,
+  STAP_ARG_BITNESS_16BIT_SIGNED,
   STAP_ARG_BITNESS_32BIT_UNSIGNED,
   STAP_ARG_BITNESS_32BIT_SIGNED,
   STAP_ARG_BITNESS_64BIT_UNSIGNED,
@@ -329,6 +333,18 @@ stap_get_expected_argument_type (struct gdbarch *gdbarch,
       else
 	return builtin_type (gdbarch)->builtin_uint64;
 
+    case STAP_ARG_BITNESS_8BIT_UNSIGNED:
+      return builtin_type (gdbarch)->builtin_uint8;
+
+    case STAP_ARG_BITNESS_8BIT_SIGNED:
+      return builtin_type (gdbarch)->builtin_int8;
+
+    case STAP_ARG_BITNESS_16BIT_UNSIGNED:
+      return builtin_type (gdbarch)->builtin_uint16;
+
+    case STAP_ARG_BITNESS_16BIT_SIGNED:
+      return builtin_type (gdbarch)->builtin_int16;
+
     case STAP_ARG_BITNESS_32BIT_SIGNED:
       return builtin_type (gdbarch)->builtin_int32;
 
@@ -1095,7 +1111,7 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 
 	 N@OP
 
-	 Where `N' can be [+,-][4,8].  This is not mandatory, so
+	 Where `N' can be [+,-][1,2,4,8].  This is not mandatory, so
 	 we check it here.  If we don't find it, go to the next
 	 state.  */
       if ((cur[0] == '-' && isdigit (cur[1]) && cur[2] == '@')
@@ -1108,20 +1124,38 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	      got_minus = 1;
 	    }
 
-	  if (*cur == '4')
-	    b = (got_minus ? STAP_ARG_BITNESS_32BIT_SIGNED
-		 : STAP_ARG_BITNESS_32BIT_UNSIGNED);
-	  else if (*cur == '8')
-	    b = (got_minus ? STAP_ARG_BITNESS_64BIT_SIGNED
-		 : STAP_ARG_BITNESS_64BIT_UNSIGNED);
-	  else
+	  /* Defining the bitness.  */
+	  switch (*cur)
 	    {
-	      /* We have an error, because we don't expect anything
-		 except 4 and 8.  */
-	      complaint (&symfile_complaints,
-			 _("unrecognized bitness `%c' for probe `%s'"),
-			 *cur, probe->p.name);
-	      return;
+	    case '1':
+	      b = (got_minus ? STAP_ARG_BITNESS_8BIT_SIGNED
+		   : STAP_ARG_BITNESS_8BIT_UNSIGNED);
+	      break;
+
+	    case '2':
+	      b = (got_minus ? STAP_ARG_BITNESS_16BIT_SIGNED
+		   : STAP_ARG_BITNESS_16BIT_UNSIGNED);
+	      break;
+
+	    case '4':
+	      b = (got_minus ? STAP_ARG_BITNESS_32BIT_SIGNED
+		   : STAP_ARG_BITNESS_32BIT_UNSIGNED);
+	      break;
+
+	    case '8':
+	      b = (got_minus ? STAP_ARG_BITNESS_64BIT_SIGNED
+		   : STAP_ARG_BITNESS_64BIT_UNSIGNED);
+	      break;
+
+	    default:
+	      {
+		/* We have an error, because we don't expect anything
+		   except 4 and 8.  */
+		complaint (&symfile_complaints,
+			   _("unrecognized bitness `%c' for probe `%s'"),
+			   *cur, probe->p.name);
+		return;
+	      }
 	    }
 
 	  arg.bitness = b;
-- 
1.7.11.7

Re: [PATCH 2/2] Extend recognized types of SDT probe's arguments

[Pedro Alves]

On 05/01/2014 10:52 PM, Sergio Durigan Junior wrote:
> This commit is actually an update to make the parser in
> gdb/stap-probe.c be aware of all the possible prefixes that a probe
> argument can have.  According to the section "Argument Format" in:
> 
>   <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>
> 
> The bitness of the arguments can be 8, 16, 32 or 64 bits, signed or
> unsigned.  Currently GDB recognizes only 32 and 64-bit arguments.

Looks good.

> This commit extends this.  Since this is a straightforward extension,
> I am not submitting a testcase; I can do that if anybody wants.

I think it'd be good to have a test -- the code that triggered the other
bug was also supposedly straightforward.  :-)  Can we do this in C ?
Ideally we'd also test that we don't crash with an invalid bitness
(the complaint path).

> 
> gdb/
> 2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	stap-probe.c (enum stap_arg_bitness): New enums to represent 8

Missing '*'-

> 	and 16-bit signed and unsigned arguments.
> 	(stap_parse_probe_arguments): Extend code to handle such
> 	arguments.

-- 
Pedro Alves

Re: [PATCH 2/2] Extend recognized types of SDT probe's arguments

[Sergio Durigan Junior]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On Friday, May 02 2014, Pedro Alves wrote:

> On 05/01/2014 10:52 PM, Sergio Durigan Junior wrote:
>> This commit is actually an update to make the parser in
>> gdb/stap-probe.c be aware of all the possible prefixes that a probe
>> argument can have.  According to the section "Argument Format" in:
>> 
>>   <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>
>> 
>> The bitness of the arguments can be 8, 16, 32 or 64 bits, signed or
>> unsigned.  Currently GDB recognizes only 32 and 64-bit arguments.
>
> Looks good.

Thanks.

>> This commit extends this.  Since this is a straightforward extension,
>> I am not submitting a testcase; I can do that if anybody wants.
>
> I think it'd be good to have a test -- the code that triggered the other
> bug was also supposedly straightforward.  :-)  Can we do this in C ?
> Ideally we'd also test that we don't crash with an invalid bitness
> (the complaint path).

Hm, OK, here is the testcase then.

>> 
>> gdb/
>> 2014-05-01  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	stap-probe.c (enum stap_arg_bitness): New enums to represent 8
>
> Missing '*'-

Writing ChangeLog entries while at "git commit" buffer is tricky...
Second mistake in a row!  Thanks.

>> 	and 16-bit signed and unsigned arguments.
>> 	(stap_parse_probe_arguments): Extend code to handle such
>> 	arguments.
>
> -- 
> Pedro Alves

Here is what I will push if there are no other comments.

Thanks,

-- 
Sergio


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 111.patch --]
[-- Type: text/x-patch, Size: 7532 bytes --]

commit 4fdce9c1c58588209e26cd690e7e653b6056d458
Author: Sergio Durigan Junior <sergiodj@redhat.com>
Date:   Thu May 1 18:39:28 2014 -0300

    Extend recognized types of SDT probe's arguments
    
    This commit is actually an update to make the parser in
    gdb/stap-probe.c be aware of all the possible prefixes that a probe
    argument can have.  According to the section "Argument Format" in:
    
      <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>
    
    The bitness of the arguments can be 8, 16, 32 or 64 bits, signed or
    unsigned.  Currently GDB recognizes only 32 and 64-bit arguments.
    This commit extends this.  Since this is a straightforward extension,
    I am not submitting a testcase; I can do that if anybody wants.
    
    gdb/
    2014-05-02  Sergio Durigan Junior  <sergiodj@redhat.com>
    
    	* stap-probe.c (enum stap_arg_bitness): New enums to represent 8
    	and 16-bit signed and unsigned arguments.  Update comment.
    	(stap_parse_probe_arguments): Extend code to handle such
    	arguments.  Use warning instead of complaint to notify about
    	unrecognized bitness.
    
    gdb/testsuite/
    2014-05-02  Sergio Durigan Junior  <sergiodj@redhat.com>
    
    	* gdb.arch/amd64-stap-optional-prefix.S (main): Add several
    	probes to test for bitness recognition.
    	* gdb.arch/amd64-stap-optional-prefix.exp
    	(test_probe_value_without_reg): New procedure.
    	Add code to test for different kinds of bitness.

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index ef45495..84714b5 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -60,6 +60,10 @@ static unsigned int stap_expression_debug = 0;
    The relationship is:
 
    - STAP_ARG_BITNESS_UNDEFINED:  The user hasn't specified the bitness.
+   - STAP_ARG_BITNESS_8BIT_UNSIGNED:  argument string starts with `1@'.
+   - STAP_ARG_BITNESS_8BIT_SIGNED:  argument string starts with `-1@'.
+   - STAP_ARG_BITNESS_16BIT_UNSIGNED:  argument string starts with `2@'.
+   - STAP_ARG_BITNESS_16BIT_SIGNED:  argument string starts with `-2@'.
    - STAP_ARG_BITNESS_32BIT_UNSIGNED:  argument string starts with `4@'.
    - STAP_ARG_BITNESS_32BIT_SIGNED:  argument string starts with `-4@'.
    - STAP_ARG_BITNESS_64BIT_UNSIGNED:  argument string starts with `8@'.
@@ -68,6 +72,10 @@ static unsigned int stap_expression_debug = 0;
 enum stap_arg_bitness
 {
   STAP_ARG_BITNESS_UNDEFINED,
+  STAP_ARG_BITNESS_8BIT_UNSIGNED,
+  STAP_ARG_BITNESS_8BIT_SIGNED,
+  STAP_ARG_BITNESS_16BIT_UNSIGNED,
+  STAP_ARG_BITNESS_16BIT_SIGNED,
   STAP_ARG_BITNESS_32BIT_UNSIGNED,
   STAP_ARG_BITNESS_32BIT_SIGNED,
   STAP_ARG_BITNESS_64BIT_UNSIGNED,
@@ -329,6 +337,18 @@ stap_get_expected_argument_type (struct gdbarch *gdbarch,
       else
 	return builtin_type (gdbarch)->builtin_uint64;
 
+    case STAP_ARG_BITNESS_8BIT_UNSIGNED:
+      return builtin_type (gdbarch)->builtin_uint8;
+
+    case STAP_ARG_BITNESS_8BIT_SIGNED:
+      return builtin_type (gdbarch)->builtin_int8;
+
+    case STAP_ARG_BITNESS_16BIT_UNSIGNED:
+      return builtin_type (gdbarch)->builtin_uint16;
+
+    case STAP_ARG_BITNESS_16BIT_SIGNED:
+      return builtin_type (gdbarch)->builtin_int16;
+
     case STAP_ARG_BITNESS_32BIT_SIGNED:
       return builtin_type (gdbarch)->builtin_int32;
 
@@ -1095,7 +1115,7 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 
 	 N@OP
 
-	 Where `N' can be [+,-][4,8].  This is not mandatory, so
+	 Where `N' can be [+,-][1,2,4,8].  This is not mandatory, so
 	 we check it here.  If we don't find it, go to the next
 	 state.  */
       if ((cur[0] == '-' && isdigit (cur[1]) && cur[2] == '@')
@@ -1108,20 +1128,37 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
 	      got_minus = 1;
 	    }
 
-	  if (*cur == '4')
-	    b = (got_minus ? STAP_ARG_BITNESS_32BIT_SIGNED
-		 : STAP_ARG_BITNESS_32BIT_UNSIGNED);
-	  else if (*cur == '8')
-	    b = (got_minus ? STAP_ARG_BITNESS_64BIT_SIGNED
-		 : STAP_ARG_BITNESS_64BIT_UNSIGNED);
-	  else
+	  /* Defining the bitness.  */
+	  switch (*cur)
 	    {
-	      /* We have an error, because we don't expect anything
-		 except 4 and 8.  */
-	      complaint (&symfile_complaints,
-			 _("unrecognized bitness `%c' for probe `%s'"),
-			 *cur, probe->p.name);
-	      return;
+	    case '1':
+	      b = (got_minus ? STAP_ARG_BITNESS_8BIT_SIGNED
+		   : STAP_ARG_BITNESS_8BIT_UNSIGNED);
+	      break;
+
+	    case '2':
+	      b = (got_minus ? STAP_ARG_BITNESS_16BIT_SIGNED
+		   : STAP_ARG_BITNESS_16BIT_UNSIGNED);
+	      break;
+
+	    case '4':
+	      b = (got_minus ? STAP_ARG_BITNESS_32BIT_SIGNED
+		   : STAP_ARG_BITNESS_32BIT_UNSIGNED);
+	      break;
+
+	    case '8':
+	      b = (got_minus ? STAP_ARG_BITNESS_64BIT_SIGNED
+		   : STAP_ARG_BITNESS_64BIT_UNSIGNED);
+	      break;
+
+	    default:
+	      {
+		/* We have an error, because we don't expect anything
+		   except 1, 2, 4 and 8.  */
+		warning (_("unrecognized bitness %s%c' for probe `%s'"),
+			 got_minus ? "`-" : "`", *cur, probe->p.name);
+		return;
+	      }
 	    }
 
 	  arg.bitness = b;
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
index 66689cb..3cdb4c2 100644
--- a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
@@ -28,5 +28,15 @@ main:
 	STAP_PROBE1(probe, foo_prefix, 8@(%rsp))
 	STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp))
 	mov	%rbp,%rsp
+	STAP_PROBE1(probe, uint8_probe, 1@$8)
+	STAP_PROBE1(probe, int8_probe, -1@$-8)
+	STAP_PROBE1(probe, uint16_probe, 2@$16)
+	STAP_PROBE1(probe, int16_probe, -2@$-16)
+	STAP_PROBE1(probe, uint32_probe, 4@$32)
+	STAP_PROBE1(probe, int32_probe, -4@$-32)
+	STAP_PROBE1(probe, uint64_probe, 8@$64)
+	STAP_PROBE1(probe, int64_probe, -8@$-64)
+	STAP_PROBE1(probe, fail_probe, -7@$16)
+	STAP_PROBE1(probe, fail2_probe, 23-@$16)
 	xor	%rax,%rax
 	ret
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
index cc9d6c3..b7f1505 100644
--- a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
@@ -43,6 +43,11 @@ proc test_probe_value { value reg_val } {
     gdb_test "print \$_probe_arg0 == *((unsigned int *) (${reg_val}))" "= 1"
 }
 
+proc test_probe_value_without_reg { value } {
+    gdb_test "print \$_probe_argc" "= 1"
+    gdb_test "print \$_probe_arg0" "= $value"
+}
+
 if { ![runto_main] } {
     return -1
 }
@@ -55,3 +60,32 @@ foreach probe_name [list "foo" "bar" "foo_prefix" "bar_prefix"] \
 	test_probe_value $probe_val $probe_reg_val
     }
 }
+
+# Testing normal probes, with several prefixes.
+
+set normal_probes_names { }
+
+foreach bit [list 8 16 32 64] {
+    lappend normal_probes_names "uint${bit}_probe"
+    lappend normal_probes_names "int${bit}_probe"
+}
+
+foreach probe_name $normal_probes_names \
+    probe_val [list 8 -8 16 -16 32 -32 64 -64] {
+    with_test_prefix $probe_name {
+	goto_probe $probe_name
+	test_probe_value_without_reg $probe_val
+    }
+}
+
+# Testing the fail probes.
+
+with_test_prefix "fail_probe" {
+    goto_probe "fail_probe"
+    gdb_test "print \$_probe_arg0" "warning: unrecognized bitness `-7' for probe `fail_probe'\r\nInvalid probe argument 0 -- probe has 0 arguments available"
+}
+
+with_test_prefix "fail2_probe" {
+    goto_probe "fail2_probe"
+    gdb_test "print \$_probe_arg0" "Unknown numeric token on expression `23-@\\\$16'."
+}

Re: [PATCH 2/2] Extend recognized types of SDT probe's arguments

[Pedro Alves]

On 05/02/2014 08:25 PM, Sergio Durigan Junior wrote:
> On Friday, May 02 2014, Pedro Alves wrote:

>> I think it'd be good to have a test -- the code that triggered the other
>> bug was also supposedly straightforward.  :-)  Can we do this in C ?
>> Ideally we'd also test that we don't crash with an invalid bitness
>> (the complaint path).
> 
> Hm, OK, here is the testcase then.

Thanks!

> Here is what I will push if there are no other comments.

Looks great.

>     This commit extends this.  Since this is a straightforward extension,
>     I am not submitting a testcase; I can do that if anybody wants.

This sentence is stale.  ;-)

-- 
Pedro Alves

Re: [PATCH 2/2] Extend recognized types of SDT probe's arguments

[Sergio Durigan Junior]

On Friday, May 02 2014, Pedro Alves wrote:

>> Here is what I will push if there are no other comments.
>
> Looks great.

Thanks!

>>     This commit extends this.  Since this is a straightforward extension,
>>     I am not submitting a testcase; I can do that if anybody wants.
>
> This sentence is stale.  ;-)

Removed :-).

Pushed:

  <https://sourceware.org/ml/gdb-cvs/2014-05/msg00004.html>

Thanks,

-- 
Sergio