From: Luis Machado via Gdb-patches <gdb-patches@sourceware.org>
To: Mike Frysinger <vapier@gentoo.org>, gdb-patches@sourceware.org
Subject: Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs
Date: Thu, 11 Nov 2021 09:41:09 -0300 [thread overview]
Message-ID: <68a4c7ee-700a-22f5-62ad-9257de03e9f9@linaro.org> (raw)
In-Reply-To: <20211107003254.4298-1-vapier@gentoo.org>
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 #<imm>,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;
>
>
next prev parent reply other threads:[~2021-11-11 12:41 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-07 0:32 Mike Frysinger via Gdb-patches
2021-11-07 0:32 ` [PATCH 2/6] sim: sh: fix unused-value warnings Mike Frysinger via Gdb-patches
2021-11-07 0:32 ` [PATCH 3/6] sim: sh: fix various parentheses warnings Mike Frysinger via Gdb-patches
2021-11-07 0:32 ` [PATCH 4/6] sim: sh: constify a few read-only lookup tables Mike Frysinger via Gdb-patches
2021-11-07 0:32 ` [PATCH 5/6] sim: sh: fix uninitialized variable usage with pdmsb Mike Frysinger via Gdb-patches
2021-11-07 0:32 ` [PATCH 6/6] sim: sh: enable -Werror everywhere Mike Frysinger via Gdb-patches
2021-11-11 12:41 ` Luis Machado via Gdb-patches [this message]
2021-11-11 22:25 ` [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs Mike Frysinger via Gdb-patches
2021-11-11 22:32 ` Luis Machado via Gdb-patches
2021-11-11 22:38 ` Luis Machado via Gdb-patches
2021-11-11 22:45 ` Mike Frysinger via Gdb-patches
2021-11-12 13:12 ` Luis Machado via Gdb-patches
2021-11-12 0:39 ` [PATCH 1/2] sim: sh: rework carry checks to not rely on integer overflows Mike Frysinger via Gdb-patches
2021-11-12 0:39 ` [PATCH 2/2] sim: sh: fix switch-bool warnings Mike Frysinger via Gdb-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=68a4c7ee-700a-22f5-62ad-9257de03e9f9@linaro.org \
--to=gdb-patches@sourceware.org \
--cc=luis.machado@linaro.org \
--cc=vapier@gentoo.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox