From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31925 invoked by alias); 27 Mar 2014 13:07:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 31914 invoked by uid 89); 27 Mar 2014 13:07:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.6 required=5.0 tests=AWL,BAYES_00,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 27 Mar 2014 13:07:17 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id E8A1B1163E2; Thu, 27 Mar 2014 09:07:15 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 5mGgKhJi56ix; Thu, 27 Mar 2014 09:07:15 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id BD3231163C6; Thu, 27 Mar 2014 09:07:15 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 2F7EDE104A; Thu, 27 Mar 2014 06:07:14 -0700 (PDT) Date: Thu, 27 Mar 2014 13:07:00 -0000 From: Joel Brobecker To: Kyle McMartin Cc: gdb-patches@sourceware.org Subject: Re: [PATCHv2] aarch64: detect atomic sequences like other ll/sc architectures Message-ID: <20140327130714.GB4030@adacore.com> References: <20140327015125.GE3075@redacted.bos.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140327015125.GE3075@redacted.bos.redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-03/txt/msg00628.txt.bz2 Hi Kyle, > 2014-03-26 Kyle McMartin > > 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 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