From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id AiAiBH8PjWFTKgAAWB0awg (envelope-from ) for ; Thu, 11 Nov 2021 07:41:35 -0500 Received: by simark.ca (Postfix, from userid 112) id F343F1F0C1; Thu, 11 Nov 2021 07:41:34 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 984A01ECEB for ; Thu, 11 Nov 2021 07:41:33 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D25A7385802B for ; Thu, 11 Nov 2021 12:41:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D25A7385802B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1636634492; bh=ZN6q604e1/d+5NMMREGKgwwl6qYWanjrPdjzn5qqqJU=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=SBoCff6eyjMpZ7JPVMA2sbMkiBqpgo/1KssCavMYe+LjXRI0Gg+xi4Ka9Fri41L1n S22ioEVe4iOTCj7QQN1oa4S4DkyrLNLUy4uOZA3ek/8fUc+3MNOHLJhOP75WqcZ9WQ U3FWCRXN7qn3X0fIJ/nwZoVHC6hKFHdnLbhesZ1E= Received: from mail-ua1-x92e.google.com (mail-ua1-x92e.google.com [IPv6:2607:f8b0:4864:20::92e]) by sourceware.org (Postfix) with ESMTPS id 27422385840A for ; Thu, 11 Nov 2021 12:41:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 27422385840A Received: by mail-ua1-x92e.google.com with SMTP id az37so11470637uab.13 for ; Thu, 11 Nov 2021 04:41:12 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=ZN6q604e1/d+5NMMREGKgwwl6qYWanjrPdjzn5qqqJU=; b=SOG83GrnpPxIdBK0DlwpulkAqvYug+L5ujw+OQvFZaSmrDz0jtD2jyRVA4IY2iqAp6 +aJFK9ubysUtigBILHIHF9fT7baPNKfxXAxfLIlDpvHEC8KycbEBwCFh82fgiy7iET+Z g984nXRFCGiyz5OWSbGqBNBJjg3asFAAdn2vWXbFnYjTPV0tNLqK2qwQl3EH1E7Bc73R Si7yQ7rOfgZviYYQFUrH99uMoR7azzYXZaGjzOVzOWd1GWHJJiiD5RFNmcBld2LvsTOx gIXakRXxSMMIADzO5GUXO3nkpFf/fyssYYE2PkeKhLWDSrLSq1UNDzEry6/pdPygDRwT Jc7Q== X-Gm-Message-State: AOAM530nkmfitgzerkDSLv970xwIXUz8Fhrrfon+WZG+riKm6VxOAMbb Ywjkrburuja7W8NWSPpEgvkqBDjkSDYlQQ== X-Google-Smtp-Source: ABdhPJww5Gl7LwBYkeQlo3gcsdBa32nIENMsm2FLXhqvy1wPbIWwi60cD9b5Veltuv2zkLKZb+EA6w== X-Received: by 2002:ab0:45a8:: with SMTP id u37mr10394952uau.24.1636634471646; Thu, 11 Nov 2021 04:41:11 -0800 (PST) Received: from ?IPv6:2804:7f0:4841:487c:f853:f29d:cfe6:370b? ([2804:7f0:4841:487c:f853:f29d:cfe6:370b]) by smtp.gmail.com with ESMTPSA id bc18sm1903303vkb.34.2021.11.11.04.41.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 11 Nov 2021 04:41:11 -0800 (PST) Subject: Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs To: Mike Frysinger , gdb-patches@sourceware.org References: <20211107003254.4298-1-vapier@gentoo.org> Message-ID: <68a4c7ee-700a-22f5-62ad-9257de03e9f9@linaro.org> Date: Thu, 11 Nov 2021 09:41:09 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20211107003254.4298-1-vapier@gentoo.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Luis Machado via Gdb-patches Reply-To: Luis Machado Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Hi Mike, I can't pinpoint the exact SH patch, but builds are broken for --enable-targets=all in Ubuntu 18.04 with GCC 7.5: binutils-gdb/sim/sh/interp.c: In function ‘ppi_insn’: ./ppi.c:875:21: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow] carry = res < Sy; ~~~~^~~~ ./ppi.c:849:21: error: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Werror=strict-overflow] carry = res > Sy; ~~~~^~~~ ./ppi.c:823:21: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow] carry = res < Sx; ~~~~^~~~ ./ppi.c:797:21: error: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Werror=strict-overflow] carry = res > Sx; ~~~~^~~~ binutils-gdb-arm64-bionic/sim/../../../repos/binutils-gdb/sim/sh/interp.c: In function ‘sim_resume’: ./ppi.c:1178:28: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized] MACL = DSP_R (z) = res; ./ppi.c:44:7: note: ‘res’ was declared here int res, res_grd; ^~~ Could you please take a look? On 11/6/21 9:32 PM, Mike Frysinger via Gdb-patches wrote: > Now that we require C11, we can leverage anonymous unions & structs > to fix a long standing issue with the SH register layout. The use > of sregs.i for sh-dsp has generated a lot of compiler warnings about > the access being out of bounds -- it only has 7 elements declared, > but code goes beyond that to reach into the fregs that follow. But > now that we have anonymous unions, we can reduce the nested names > and have sregs cover all of these registers. > --- > sim/sh/gencode.c | 10 ++--- > sim/sh/interp.c | 108 +++++++++++++++++++++++----------------------- > sim/sh/sim-main.h | 54 ++++++++++------------- > 3 files changed, 82 insertions(+), 90 deletions(-) > > diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c > index 77a83d637685..c922cfe43b96 100644 > --- a/sim/sh/gencode.c > +++ b/sim/sh/gencode.c > @@ -354,7 +354,7 @@ static op tab[] = > > /* sh4a */ > { "", "", "clrdmxy", "0000000010001000", > - "saved_state.asregs.cregs.named.sr &= ~(SR_MASK_DMX | SR_MASK_DMY);" > + "saved_state.asregs.sr &= ~(SR_MASK_DMX | SR_MASK_DMY);" > }, > > { "", "0", "cmp/eq #,R0", "10001000i8*1....", > @@ -1342,14 +1342,14 @@ static op tab[] = > > /* sh4a */ > { "", "", "setdmx", "0000000010011000", > - "saved_state.asregs.cregs.named.sr |= SR_MASK_DMX;" > - "saved_state.asregs.cregs.named.sr &= ~SR_MASK_DMY;" > + "saved_state.asregs.sr |= SR_MASK_DMX;" > + "saved_state.asregs.sr &= ~SR_MASK_DMY;" > }, > > /* sh4a */ > { "", "", "setdmy", "0000000011001000", > - "saved_state.asregs.cregs.named.sr |= SR_MASK_DMY;" > - "saved_state.asregs.cregs.named.sr &= ~SR_MASK_DMX;" > + "saved_state.asregs.sr |= SR_MASK_DMY;" > + "saved_state.asregs.sr &= ~SR_MASK_DMX;" > }, > > /* sh-dsp */ > diff --git a/sim/sh/interp.c b/sim/sh/interp.c > index 264e9b1de465..4cac8de89d53 100644 > --- a/sim/sh/interp.c > +++ b/sim/sh/interp.c > @@ -120,23 +120,23 @@ static int maskl = 0; > #define UR (unsigned int) R > #define UR (unsigned int) R > #define SR0 saved_state.asregs.regs[0] > -#define CREG(n) (saved_state.asregs.cregs.i[(n)]) > -#define GBR saved_state.asregs.cregs.named.gbr > -#define VBR saved_state.asregs.cregs.named.vbr > -#define DBR saved_state.asregs.cregs.named.dbr > -#define TBR saved_state.asregs.cregs.named.tbr > -#define IBCR saved_state.asregs.cregs.named.ibcr > -#define IBNR saved_state.asregs.cregs.named.ibnr > -#define BANKN (saved_state.asregs.cregs.named.ibnr & 0x1ff) > -#define ME ((saved_state.asregs.cregs.named.ibnr >> 14) & 0x3) > -#define SSR saved_state.asregs.cregs.named.ssr > -#define SPC saved_state.asregs.cregs.named.spc > -#define SGR saved_state.asregs.cregs.named.sgr > -#define SREG(n) (saved_state.asregs.sregs.i[(n)]) > -#define MACH saved_state.asregs.sregs.named.mach > -#define MACL saved_state.asregs.sregs.named.macl > -#define PR saved_state.asregs.sregs.named.pr > -#define FPUL saved_state.asregs.sregs.named.fpul > +#define CREG(n) (saved_state.asregs.cregs[(n)]) > +#define GBR saved_state.asregs.gbr > +#define VBR saved_state.asregs.vbr > +#define DBR saved_state.asregs.dbr > +#define TBR saved_state.asregs.tbr > +#define IBCR saved_state.asregs.ibcr > +#define IBNR saved_state.asregs.ibnr > +#define BANKN (saved_state.asregs.ibnr & 0x1ff) > +#define ME ((saved_state.asregs.ibnr >> 14) & 0x3) > +#define SSR saved_state.asregs.ssr > +#define SPC saved_state.asregs.spc > +#define SGR saved_state.asregs.sgr > +#define SREG(n) (saved_state.asregs.sregs[(n)]) > +#define MACH saved_state.asregs.mach > +#define MACL saved_state.asregs.macl > +#define PR saved_state.asregs.pr > +#define FPUL saved_state.asregs.fpul > > #define PC insn_ptr > > @@ -145,8 +145,8 @@ static int maskl = 0; > /* Alternate bank of registers r0-r7 */ > > /* Note: code controling SR handles flips between BANK0 and BANK1 */ > -#define Rn_BANK(n) (saved_state.asregs.cregs.named.bank[(n)]) > -#define SET_Rn_BANK(n, EXP) do { saved_state.asregs.cregs.named.bank[(n)] = (EXP); } while (0) > +#define Rn_BANK(n) (saved_state.asregs.bank[(n)]) > +#define SET_Rn_BANK(n, EXP) do { saved_state.asregs.bank[(n)] = (EXP); } while (0) > > > /* Manipulate SR */ > @@ -167,28 +167,28 @@ static int maskl = 0; > #define SR_MASK_RC 0x0fff0000 > #define SR_RC_INCREMENT -0x00010000 > > -#define BO ((saved_state.asregs.cregs.named.sr & SR_MASK_BO) != 0) > -#define CS ((saved_state.asregs.cregs.named.sr & SR_MASK_CS) != 0) > -#define M ((saved_state.asregs.cregs.named.sr & SR_MASK_M) != 0) > -#define Q ((saved_state.asregs.cregs.named.sr & SR_MASK_Q) != 0) > -#define S ((saved_state.asregs.cregs.named.sr & SR_MASK_S) != 0) > -#define T ((saved_state.asregs.cregs.named.sr & SR_MASK_T) != 0) > -#define LDST ((saved_state.asregs.cregs.named.ldst) != 0) > - > -#define SR_BL ((saved_state.asregs.cregs.named.sr & SR_MASK_BL) != 0) > -#define SR_RB ((saved_state.asregs.cregs.named.sr & SR_MASK_RB) != 0) > -#define SR_MD ((saved_state.asregs.cregs.named.sr & SR_MASK_MD) != 0) > -#define SR_DMY ((saved_state.asregs.cregs.named.sr & SR_MASK_DMY) != 0) > -#define SR_DMX ((saved_state.asregs.cregs.named.sr & SR_MASK_DMX) != 0) > -#define SR_RC ((saved_state.asregs.cregs.named.sr & SR_MASK_RC)) > +#define BO ((saved_state.asregs.sr & SR_MASK_BO) != 0) > +#define CS ((saved_state.asregs.sr & SR_MASK_CS) != 0) > +#define M ((saved_state.asregs.sr & SR_MASK_M) != 0) > +#define Q ((saved_state.asregs.sr & SR_MASK_Q) != 0) > +#define S ((saved_state.asregs.sr & SR_MASK_S) != 0) > +#define T ((saved_state.asregs.sr & SR_MASK_T) != 0) > +#define LDST ((saved_state.asregs.ldst) != 0) > + > +#define SR_BL ((saved_state.asregs.sr & SR_MASK_BL) != 0) > +#define SR_RB ((saved_state.asregs.sr & SR_MASK_RB) != 0) > +#define SR_MD ((saved_state.asregs.sr & SR_MASK_MD) != 0) > +#define SR_DMY ((saved_state.asregs.sr & SR_MASK_DMY) != 0) > +#define SR_DMX ((saved_state.asregs.sr & SR_MASK_DMX) != 0) > +#define SR_RC ((saved_state.asregs.sr & SR_MASK_RC)) > > /* Note: don't use this for privileged bits */ > #define SET_SR_BIT(EXP, BIT) \ > do { \ > if ((EXP) & 1) \ > - saved_state.asregs.cregs.named.sr |= (BIT); \ > + saved_state.asregs.sr |= (BIT); \ > else \ > - saved_state.asregs.cregs.named.sr &= ~(BIT); \ > + saved_state.asregs.sr &= ~(BIT); \ > } while (0) > > #define SET_SR_BO(EXP) SET_SR_BIT ((EXP), SR_MASK_BO) > @@ -205,16 +205,16 @@ do { \ > #define SET_SR_Q(EXP) SET_SR_BIT ((EXP), SR_MASK_Q) > #define SET_SR_S(EXP) SET_SR_BIT ((EXP), SR_MASK_S) > #define SET_SR_T(EXP) SET_SR_BIT ((EXP), SR_MASK_T) > -#define SET_LDST(EXP) (saved_state.asregs.cregs.named.ldst = ((EXP) != 0)) > +#define SET_LDST(EXP) (saved_state.asregs.ldst = ((EXP) != 0)) > > /* stc currently relies on being able to read SR without modifications. */ > -#define GET_SR() (saved_state.asregs.cregs.named.sr - 0) > +#define GET_SR() (saved_state.asregs.sr - 0) > > #define SET_SR(x) set_sr (x) > > #define SET_RC(x) \ > - (saved_state.asregs.cregs.named.sr \ > - = (saved_state.asregs.cregs.named.sr & 0xf000ffff) | ((x) & 0xfff) << 16) > + (saved_state.asregs.sr \ > + = (saved_state.asregs.sr & 0xf000ffff) | ((x) & 0xfff) << 16) > > /* Manipulate FPSCR */ > > @@ -229,10 +229,10 @@ do { \ > static void > set_fpscr1 (int x) > { > - int old = saved_state.asregs.sregs.named.fpscr; > - saved_state.asregs.sregs.named.fpscr = (x); > + int old = saved_state.asregs.fpscr; > + saved_state.asregs.fpscr = (x); > /* swap the floating point register banks */ > - if ((saved_state.asregs.sregs.named.fpscr ^ old) & FPSCR_MASK_FR > + if ((saved_state.asregs.fpscr ^ old) & FPSCR_MASK_FR > /* Ignore bit change if simulating sh-dsp. */ > && ! target_dsp) > { > @@ -243,13 +243,13 @@ set_fpscr1 (int x) > } > > /* sts relies on being able to read fpscr directly. */ > -#define GET_FPSCR() (saved_state.asregs.sregs.named.fpscr) > +#define GET_FPSCR() (saved_state.asregs.fpscr) > #define SET_FPSCR(x) \ > do { \ > set_fpscr1 (x); \ > } while (0) > > -#define DSR (saved_state.asregs.sregs.named.fpscr) > +#define DSR (saved_state.asregs.fpscr) > > #define RAISE_EXCEPTION(x) \ > (saved_state.asregs.exception = x, saved_state.asregs.insn_end = 0) > @@ -410,15 +410,15 @@ set_dr (int n, double exp) > #define XF(n) (saved_state.asregs.fregs[(n) >> 5].i[(n) & 0x1f]) > #define SET_XF(n,EXP) (saved_state.asregs.fregs[(n) >> 5].i[(n) & 0x1f] = (EXP)) > > -#define RS saved_state.asregs.cregs.named.rs > -#define RE saved_state.asregs.cregs.named.re > -#define MOD (saved_state.asregs.cregs.named.mod) > +#define RS saved_state.asregs.rs > +#define RE saved_state.asregs.re > +#define MOD (saved_state.asregs.mod) > #define SET_MOD(i) \ > (MOD = (i), \ > MOD_ME = (unsigned) MOD >> 16 | (SR_DMY ? ~0xffff : (SR_DMX ? 0 : 0x10000)), \ > MOD_DELTA = (MOD & 0xffff) - ((unsigned) MOD >> 16)) > > -#define DSP_R(n) saved_state.asregs.sregs.i[(n)] > +#define DSP_R(n) saved_state.asregs.sregs[(n)] > #define DSP_GRD(n) DSP_R ((n) + 8) > #define GET_DSP_GRD(n) ((n | 2) == 7 ? SEXT (DSP_GRD (n)) : SIGN32 (DSP_R (n))) > #define A1 DSP_R (5) > @@ -485,12 +485,12 @@ set_sr (int new_sr) > int i, tmp; > for (i = 0; i < 8; i++) > { > - tmp = saved_state.asregs.cregs.named.bank[i]; > - saved_state.asregs.cregs.named.bank[i] = saved_state.asregs.regs[i]; > + tmp = saved_state.asregs.bank[i]; > + saved_state.asregs.bank[i] = saved_state.asregs.regs[i]; > saved_state.asregs.regs[i] = tmp; > } > } > - saved_state.asregs.cregs.named.sr = new_sr; > + saved_state.asregs.sr = new_sr; > SET_MOD (MOD); > } > > @@ -1768,7 +1768,7 @@ sim_resume (SIM_DESC sd, int step, int siggnal) > CHECK_INSN_PTR (insn_ptr); > > #ifndef PR > - PR = saved_state.asregs.sregs.named.pr; > + PR = saved_state.asregs.pr; > #endif > /*T = GET_SR () & SR_MASK_T;*/ > prevlock = saved_state.asregs.prevlock; > @@ -1849,7 +1849,7 @@ sim_resume (SIM_DESC sd, int step, int siggnal) > } > if (saved_state.asregs.insn_end == loop.end) > { > - saved_state.asregs.cregs.named.sr += SR_RC_INCREMENT; > + saved_state.asregs.sr += SR_RC_INCREMENT; > if (SR_RC) > insn_ptr = loop.start; > else > @@ -1876,7 +1876,7 @@ sim_resume (SIM_DESC sd, int step, int siggnal) > saved_state.asregs.insts += insts; > saved_state.asregs.pc = PH2T (insn_ptr); > #ifndef PR > - saved_state.asregs.sregs.named.pr = PR; > + saved_state.asregs.pr = PR; > #endif > > saved_state.asregs.prevlock = prevlock; > diff --git a/sim/sh/sim-main.h b/sim/sh/sim-main.h > index 9453e62f6d27..da9d72decb74 100644 > --- a/sim/sh/sim-main.h > +++ b/sim/sh/sim-main.h > @@ -36,34 +36,26 @@ typedef union > int pc; > > /* System registers. For sh-dsp this also includes A0 / X0 / X1 / Y0 / Y1 > - which are located in fregs, i.e. strictly speaking, these are > - out-of-bounds accesses of sregs.i . This wart of the code could be > - fixed by making fregs part of sregs, and including pc too - to avoid > - alignment repercussions - but this would cause very onerous union / > - structure nesting, which would only be managable with anonymous > - unions and structs. */ > - union > - { > - struct > - { > - int mach; > - int macl; > - int pr; > - int dummy3, dummy4; > - int fpul; /* A1 for sh-dsp - but only for movs etc. */ > - int fpscr; /* dsr for sh-dsp */ > - } named; > - int i[7]; > - } sregs; > - > - /* sh3e / sh-dsp */ > - union fregs_u > - { > - float f[16]; > - double d[8]; > - int i[16]; > - } > - fregs[2]; > + which are located in fregs. Probably should include pc too - to avoid > + alignment repercussions. */ > + union { > + struct { > + int mach; > + int macl; > + int pr; > + int dummy3, dummy4; > + int fpul; /* A1 for sh-dsp - but only for movs etc. */ > + int fpscr; /* dsr for sh-dsp */ > + > + /* sh3e / sh-dsp */ > + union fregs_u { > + float f[16]; > + double d[8]; > + int i[16]; > + } fregs[2]; > + }; > + int sregs[39]; > + }; > > /* Control registers; on the SH4, ldc / stc is privileged, except when > accessing gbr. */ > @@ -88,9 +80,9 @@ typedef union > int tbr; > int ibcr; /* sh2a bank control register */ > int ibnr; /* sh2a bank number register */ > - } named; > - int i[16]; > - } cregs; > + }; > + int cregs[16]; > + }; > > unsigned char *insn_end; > >