Re: [PATCHv2] aarch64: detect atomic sequences like other ll/sc architectures

[Joel Brobecker <brobecker at adacore dot com>]

Hi Kyle,

> 2014-03-26  Kyle McMartin  <kyle@redhat.com>
> 
> gdb:
> 	* aarch64-tdep.c (aarch64_deal_with_atomic_sequence): New function.
> 	(aarch64_gdbarch_init): Handle single stepping of atomic sequences
> 	with aarch64_deal_with_atomic_sequence.
>
> 
> gdb/testsuite:
> 	* gdb.arch/aarch64-atomic-inst.c: New file.
> 	* gdb.arch/aarch64-atomic-inst.exp: New file.

A few comments on the testcase. I have no further comments on the code
itself.

> +#include <stdio.h>

Do you really need stdio.h, here? You do not seem to be making
any function call in your function, so I do not see why it would
be needed. On the other hand, having a dependency on stdio.h means
that the testcase will not compile on many targets (eg: bare metal).

> +
> +int main()

Can you use "(void)" instead of "()"?

> +{
> +  unsigned long tmp, cond;
> +  unsigned long dword = 0;
> +
> +  /* Test that we can step over ldxr/stxr. This sequence should step from
> +     ldxr to the following __asm __volatile.  */
> +  __asm __volatile ("1:     ldxr    %0,%2\n"                             \
> +                    "       cmp     %0,#1\n"                             \
> +                    "       b.eq    out\n"                               \
> +                    "       add     %0,%0,1\n"                           \
> +                    "       stxr    %w1,%0,%2\n"                         \
> +                    "       cbnz    %w1,1b"                              \
> +                    : "=&r" (tmp), "=&r" (cond), "+Q" (dword)            \
> +                    : : "memory");
> +
> +  /* This sequence should take the conditional branch and step from ldxr
> +     to the return dword line.  */
> +  __asm __volatile ("1:     ldxr    %0,%2\n"                             \
> +                    "       cmp     %0,#1\n"                             \
> +                    "       b.eq    out\n"                               \
> +                    "       add     %0,%0,1\n"                           \
> +                    "       stxr    %w1,%0,%2\n"                         \
> +                    "       cbnz    %w1,1b\n"                            \
> +                    : "=&r" (tmp), "=&r" (cond), "+Q" (dword)            \
> +                    : : "memory");
> +
> +  dword = -1;
> +__asm __volatile ("out:\n");
> +  return dword;
> +}
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/aarch64-atomic-inst.exp
> @@ -0,0 +1,58 @@
> +# Copyright 2008-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, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test single stepping through atomic sequences beginning with
> +# a ldxr instruction and ending with a stxr instruction.
> +
> +if {![istarget "aarch64*"]} {
> +    verbose "Skipping testing of aarch64 single stepping over atomic sequences."
> +    return
> +}
> +
> +set testfile "aarch64-atomic-inst"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +set compile_flags {debug quiet}
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $compile_flags] != "" } {
> +    unsupported "Testcase compile failed."
> +    return -1
> +}
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}

Can you use standard_testfile and prepare_for_testing? See our testcase
cookbook page at:
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

It should replace all of the above after the "if istarget"...

> +if ![runto_main] then {
> +    perror "Couldn't run to breakpoint"
> +    continue

Also according to the cookbook, use:

    untested "could not run to main"
    return -1

IIRC, with tcl, there isn't a huge distinction in this case between
continue and return -1, but might as well follow the cookbook.

> +gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> +  "Continue until breakpoint"
> +
> +gdb_test next ".*__asm __volatile.*" \
> +  "Step through the ldxr/stxr sequence"
> +
> +gdb_test next ".*return dword.*" \
> +  "Stepped through sequence through conditional branch"

Can you put the "continue"/"next" inside double-quotes.
It looks like it's all the same to tcl, but I think it'll make
things more consistent and also allow editors to (syntax)-highlight
those as strings...

Thanks,
-- 
Joel