From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13601 invoked by alias); 15 Feb 2017 12:13:43 -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 13588 invoked by uid 89); 15 Feb 2017 12:13:42 -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_50,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=inspired, facing, sk:PPC_FEA, sk:ppc_fea X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 15 Feb 2017 12:13:32 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cdySg-0000HZ-Tt from Luis_Gustavo@mentor.com ; Wed, 15 Feb 2017 04:13:30 -0800 Received: from [172.30.12.49] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Wed, 15 Feb 2017 04:13:27 -0800 Reply-To: Luis Machado Subject: Re: [PATCH v2] [ppc64] Add POWER8 atomic sequences single-stepping support References: <1486350182-13993-1-git-send-email-emachado@linux.vnet.ibm.com> <38fe05a1-770c-e9f1-ecc0-042dfe0a470e@codesourcery.com> <7fe00f19-6922-fd6c-fa97-a779ffebac25@linux.vnet.ibm.com> <98042cab-d856-6426-e9a6-f7256ed789d0@linux.vnet.ibm.com> To: Edjunior Barbosa Machado , CC: From: Luis Machado Message-ID: Date: Wed, 15 Feb 2017 12:13:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <98042cab-d856-6426-e9a6-f7256ed789d0@linux.vnet.ibm.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-04.mgc.mentorg.com (147.34.90.204) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00408.txt.bz2 On 02/15/2017 06:00 AM, Edjunior Barbosa Machado wrote: > Hi Luis, > thanks for the review once again. Just few doubts below. > > On 02/15/2017 08:00 AM, Luis Machado wrote: >> On 02/13/2017 06:47 PM, Edjunior Barbosa Machado wrote: >>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.S >>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.S >>> new file mode 100644 >>> index 0000000..daa3337 >>> --- /dev/null >>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.S >> >> I don't know if there are other powerpc initiatives out there other than >> IBM's power 8/9 that are using these instructions. If there are, >> renaming power8 to something generic would be best. Otherwise i don't >> see a problem with leaving this and fixing it in the future if some >> other manufacturer shows up using ISA 2.06/2.07. >> >> I thought i'd mention it though. > > > I'm also not aware of other initiatives that implement these > instructions. This name was more inspired on others testcases from gas > focused on these POWER8/ISA 2.07 instructions like > gas/testsuite/gas/ppc/power8.*. Any suggestion about what would be a > better name here? > I don't have anything off the top of my head. Only ppc-atomic-inst2, which is probably not a great name either. >> >>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.c >>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.c >>> new file mode 100644 >>> index 0000000..535e057 >>> --- /dev/null >>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.c >> >> Same as above about mentioning power8 in the filename. >> >>> @@ -0,0 +1,42 @@ >>> +/* Copyright 2017 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 >>> . */ >>> + >>> +#include >>> + >>> +typedef Elf64_auxv_t auxv_t; >>> + >>> +#ifndef PPC_FEATURE2_ARCH_2_07 >>> +#define PPC_FEATURE2_ARCH_2_07 0x80000000 >>> +#endif >>> + >>> +extern void test_atomic_sequences (void); >>> + >>> +int >>> +main (int argc, char *argv[], char *envp[], auxv_t auxv[]) >>> +{ >>> + int i; >>> + >>> + for (i = 0; auxv[i].a_type != AT_NULL; i++) >>> + if (auxv[i].a_type == AT_HWCAP2) { >>> + if (!(auxv[i].a_un.a_val & PPC_FEATURE2_ARCH_2_07)) >>> + return 1; >>> + break; >>> + } >>> + >>> + test_atomic_sequences (); >>> + return 0; >>> +} >> >> Since we've separated testing of these new instructions from the older >> ones, dropped the power8 compiler switch and are not expecting SIGILL >> anymore, do we still need a runtime check here? >> >> Checking the auxv is also Linux-specific and won't work for bare-metal. >> >> I think letting the test give a compilation error if the compiler >> doesn't support the instructions is fine and also an indication the test >> shouldn't run. >> >> If the compiler does support generating such instructions and the target >> itself doesn't support them, we will have a problem. But it would be up >> to whoever is building the program to pass the correct switches to the >> compiler. In any case, this can be handled in the future if this >> situation arises, right? >> > > > Actually this is a problem I'm already facing when testing more recent > compilers on POWER7 machines for example. It builds OK but fails with > SIGILL when running (that's why I initially tried expecting for SIGILL), > then switched to this runtime check. Do you have any suggestion about > what would be the best strategy that would work for ppc64 bare-metal too? Oh, i see. Well, i think we need the runtime check for now then. And we can handle bare-metal (if such a target is available in the future) later?