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