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

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

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

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"