From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53079 invoked by alias); 14 Apr 2016 10:00:15 -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 52970 invoked by uid 89); 14 Apr 2016 10:00:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=UD:G, H*r:ip*10.253.24.26, H*RU:HELO, Hx-spam-relays-external:HELO X-HELO: mga09.intel.com Received: from mga09.intel.com (HELO mga09.intel.com) (134.134.136.24) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Apr 2016 09:59:57 +0000 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga102.jf.intel.com with ESMTP; 14 Apr 2016 02:59:54 -0700 X-ExtLoop1: 1 Received: from wtedesch-mobl2.ger.corp.intel.com (HELO [172.28.205.68]) ([172.28.205.68]) by fmsmga002.fm.intel.com with ESMTP; 14 Apr 2016 02:59:54 -0700 Subject: Re: [PATCH V2 1/2] Add redundant target descriptor for tdesc(amd64|i386)_avx_mpx_* To: Pedro Alves , Yao Qi References: <1457025942-23711-1-git-send-email-walfred.tedeschi@intel.com> <1457025942-23711-2-git-send-email-walfred.tedeschi@intel.com> <570E360F.7060501@redhat.com> Cc: gdb-patches@sourceware.org From: Walfred Tedeschi Message-ID: <570F6A19.6090808@intel.com> Date: Thu, 14 Apr 2016 10:00:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <570E360F.7060501@redhat.com> Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-04/txt/msg00320.txt.bz2 Am 4/13/2016 um 2:05 PM schrieb Pedro Alves: > On 03/03/2016 05:25 PM, Walfred Tedeschi wrote: >> Add a redundant target description for the MPX and AVX case using a >> combined feature name to reflect that, i.e. avx-mpx. > > It's better when commit logs are self-contained and don't > depend on info in the cover letter. The cover letter doesn't > make it to git. > > E.g., > > ~~ > Subject: Add target descriptions for AVX + MPX > > The current MPX target descriptions assume that MPX is always > combined with AVX, however that's not correct. We can have > machines with MPX and without AVX; or machines with AVX > and without MPX. > > This patch adds new target descriptions for machines that > support both MPX and AVX, as duplicates of the existing > MPX descriptions. The following commit will remove AVX from > the MPX-only descriptions. > ~~ > > (Note s/descriptors/descriptions in subject) > >> >> Usage of both series of target descriptor (avx-mpx and mpx) were also fi= xed. > > I don't understand what this means. > >> +++ b/gdb/features/i386/i386-avx-mpx-linux.xml >> @@ -0,0 +1,19 @@ >> + >> + >> + >> + > > Missing space before "-". (Please audit the other added files.) > >> +++ b/gdb/features/i386/i386-avx-mpx.xml >> @@ -0,0 +1,17 @@ >> + >> + >> + >> + > > Shouldn't this say "and AVX" as well? > > >> + >> + >> + >> + i386 >> + >> + >> + >> + >> + >> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in > >> + case X86_XSTATE_AVX_MPX_MASK: >> + if (is_x32) >> + return tdesc_x32_linux; /* No AVX and MPX is not available in x32.= */ > > > The comment sounds odd to me. Does this intend to say: > > /* Neither AVX nor MPX are available on x32. */ > > ? > > Is it really true that x32 does not support AVX? > > Thanks, > Pedro Alves > Pedro and all, Thanks again for your review! V3 will come soon, however we have a finding that needs to be addressed! The reading of registers and the definition for the target descriptions=20 can trigger an assertion. I will address it on an additional patch. Here we selected the target description according to the XCR0 bits. But=20 we might select one target description that has the XCR0 bits for a=20 feature but registers are not named in there. E.G. XCR0 =3D=20 X86_XSTATE_AVX_MPX_MASK returning a tdesc_x32_avx_linux. switch (xcr0_features_bits) { case X86_XSTATE_MPX_AVX512_MASK: case X86_XSTATE_AVX512_MASK: if (is_x32) return tdesc_x32_avx512_linux; else return tdesc_amd64_avx512_linux; case X86_XSTATE_MPX_MASK: if (is_x32) return tdesc_x32_avx_linux; /* No MPX on x32 using AVX. */ else return tdesc_amd64_mpx_linux; case X86_XSTATE_AVX_MPX_MASK: if (is_x32) return tdesc_x32_avx_linux; /* No MPX on x32 using AVX. */ else return tdesc_amd64_avx_mpx_linux; case X86_XSTATE_AVX_MASK: if (is_x32) return tdesc_x32_avx_linux; else return tdesc_amd64_avx_linux; default: if (is_x32) return tdesc_x32_linux; else return tdesc_amd64_linux; } Here based on XCR0 we look for the register, but it is not in the target=20 description. if ((x86_xcr0 & X86_XSTATE_MPX)) { int ymm0h_regnum =3D find_regno (regcache->tdesc, "bnd0raw"); Here we assert. int find_regno (const struct target_desc *tdesc, const char *name) { int i; for (i =3D 0; i < tdesc->num_registers; i++) if (strcmp (name, tdesc->reg_defs[i].name) =3D=3D 0) return i; internal_error (__FILE__, __LINE__, "Unknown register %s requested", name); } My proposal would be to create another variable to keep the actual=20 feature bits when detecting the target description to be used. Thoughts? Thanks and regards, -Fred Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928