[PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[Mike Frysinger]

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;
 
-- 
2.33.0

[PATCH 2/6] sim: sh: fix unused-value warnings

[Mike Frysinger]

These macro expansions are deliberate in not using the computed value
so that they trigger side-effects (possible invalid memory accesses)
but while otherwise being noops.  Add a (void) cast so the compiler
knows these are intentional.
---
 sim/sh/gencode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c
index c922cfe43b96..ae44bc8825b8 100644
--- a/sim/sh/gencode.c
+++ b/sim/sh/gencode.c
@@ -1239,17 +1239,17 @@ static op tab[] =
   },
 
   { "", "n", "ocbi @<REG_N>", "0000nnnn10010011",
-    "RSBAT (R[n]); /* Take exceptions like byte load, otherwise noop.  */",
+    "(void) RSBAT (R[n]); /* Take exceptions like byte load, otherwise noop.  */",
     "/* FIXME: Cache not implemented */",
   },
 
   { "", "n", "ocbp @<REG_N>", "0000nnnn10100011",
-    "RSBAT (R[n]); /* Take exceptions like byte load, otherwise noop.  */",
+    "(void) RSBAT (R[n]); /* Take exceptions like byte load, otherwise noop.  */",
     "/* FIXME: Cache not implemented */",
   },
 
   { "", "n", "ocbwb @<REG_N>", "0000nnnn10110011",
-    "RSBAT (R[n]); /* Take exceptions like byte load, otherwise noop.  */",
+    "(void) RSBAT (R[n]); /* Take exceptions like byte load, otherwise noop.  */",
     "/* FIXME: Cache not implemented */",
   },
 
-- 
2.33.0

[PATCH 3/6] sim: sh: fix various parentheses warnings

[Mike Frysinger]

Add parentheses to a bunch of places where the compiler suggests we
do to avoid confusion to most readers.
---
 sim/sh/gencode.c | 18 +++++++++---------
 sim/sh/interp.c  |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c
index ae44bc8825b8..28b483208f3a 100644
--- a/sim/sh/gencode.c
+++ b/sim/sh/gencode.c
@@ -1863,7 +1863,7 @@ op ppi_tab[] =
     "if (i <= 16)",
     "  res = Sz << i;",
     "else if (i >= 128 - 16)",
-    "  res = (unsigned) Sz >> 128 - i;	/* no sign extension */",
+    "  res = (unsigned) Sz >> (128 - i);	/* no sign extension */",
     "else",
     "  {",
     "    RAISE_EXCEPTION (SIGILL);",
@@ -1887,7 +1887,7 @@ op ppi_tab[] =
     "    else",
     "      {",
     "        res = Sz << i;",
-    "        res_grd = Sz_grd << i | (unsigned) Sz >> 32 - i;",
+    "        res_grd = Sz_grd << i | (unsigned) Sz >> (32 - i);",
     "      }",
     "    res_grd = SEXT (res_grd);",
     "    carry = res_grd & 1;",
@@ -1902,7 +1902,7 @@ op ppi_tab[] =
     "      }",
     "    else",
     "      {",
-    "        res = Sz >> i | Sz_grd << 32 - i;",
+    "        res = Sz >> i | Sz_grd << (32 - i);",
     "        res_grd = Sz_grd >> i;",
     "      }",
     "    carry = Sz >> (i - 1) & 1;",
@@ -1973,7 +1973,7 @@ op ppi_tab[] =
     "ADD_SUB_GE;",
     "DSR &= ~0xf1;\n",
     "if (res || res_grd)\n",
-    "  DSR |= greater_equal | res_grd >> 2 & DSR_MASK_N | overflow;\n",
+    "  DSR |= greater_equal | (res_grd >> 2 & DSR_MASK_N) | overflow;\n",
     "else\n",
     "  DSR |= DSR_MASK_Z | overflow;\n",
     "DSR |= carry;\n",
@@ -1992,7 +1992,7 @@ op ppi_tab[] =
     "ADD_SUB_GE;",
     "DSR &= ~0xf1;\n",
     "if (res || res_grd)\n",
-    "  DSR |= greater_equal | res_grd >> 2 & DSR_MASK_N | overflow;\n",
+    "  DSR |= greater_equal | (res_grd >> 2 & DSR_MASK_N) | overflow;\n",
     "else\n",
     "  DSR |= DSR_MASK_Z | overflow;\n",
     "DSR |= carry;\n",
@@ -2148,7 +2148,7 @@ op ppi_tab[] =
     "if (Sy <= 16)",
     "  res = Sx << Sy;",
     "else if (Sy >= 128 - 16)",
-    "  res = (unsigned) Sx >> 128 - Sy;	/* no sign extension */",
+    "  res = (unsigned) Sx >> (128 - Sy);	/* no sign extension */",
     "else",
     "  {",
     "    RAISE_EXCEPTION (SIGILL);",
@@ -2171,7 +2171,7 @@ op ppi_tab[] =
     "    else",
     "      {",
     "        res = Sx << Sy;",
-    "        res_grd = Sx_grd << Sy | (unsigned) Sx >> 32 - Sy;",
+    "        res_grd = Sx_grd << Sy | (unsigned) Sx >> (32 - Sy);",
     "      }",
     "    res_grd = SEXT (res_grd);",
     "    carry = res_grd & 1;",
@@ -2186,7 +2186,7 @@ op ppi_tab[] =
     "      }",
     "    else",
     "      {",
-    "        res = Sx >> Sy | Sx_grd << 32 - Sy;",
+    "        res = Sx >> Sy | Sx_grd << (32 - Sy);",
     "        res_grd = Sx_grd >> Sy;",
     "      }",
     "    carry = Sx >> (Sy - 1) & 1;",
@@ -3347,7 +3347,7 @@ ppi_gensim (void)
   printf ("  }\n");
   printf ("  DSR &= ~0xf1;\n");
   printf ("  if (res || res_grd)\n");
-  printf ("    DSR |= greater_equal | res_grd >> 2 & DSR_MASK_N | overflow;\n");
+  printf ("    DSR |= greater_equal | (res_grd >> 2 & DSR_MASK_N) | overflow;\n");
   printf ("  else\n");
   printf ("    DSR |= DSR_MASK_Z | overflow;\n");
   printf (" assign_dc:\n");
diff --git a/sim/sh/interp.c b/sim/sh/interp.c
index 4cac8de89d53..2bae4484e349 100644
--- a/sim/sh/interp.c
+++ b/sim/sh/interp.c
@@ -195,11 +195,11 @@ do { \
 #define SET_SR_CS(EXP) SET_SR_BIT ((EXP), SR_MASK_CS)
 #define SET_BANKN(EXP) \
 do { \
-  IBNR = (IBNR & 0xfe00) | (EXP & 0x1f); \
+  IBNR = (IBNR & 0xfe00) | ((EXP) & 0x1f); \
 } while (0)
 #define SET_ME(EXP) \
 do { \
-  IBNR = (IBNR & 0x3fff) | ((EXP & 0x3) << 14); \
+  IBNR = (IBNR & 0x3fff) | (((EXP) & 0x3) << 14); \
 } while (0)
 #define SET_SR_M(EXP) SET_SR_BIT ((EXP), SR_MASK_M)
 #define SET_SR_Q(EXP) SET_SR_BIT ((EXP), SR_MASK_Q)
-- 
2.33.0

[PATCH 4/6] sim: sh: constify a few read-only lookup tables

[Mike Frysinger]

---
 sim/sh/gencode.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c
index 28b483208f3a..9ef8b4610be1 100644
--- a/sim/sh/gencode.c
+++ b/sim/sh/gencode.c
@@ -3244,17 +3244,17 @@ ppi_gensim (void)
   printf ("ppi_insn (int iword)\n");
   printf ("{\n");
   printf ("  /* 'ee' = [x0, x1, y0, a1] */\n");
-  printf ("  static char e_tab[] = { 8,  9, 10,  5};\n");
+  printf ("  static char const e_tab[] = { 8,  9, 10,  5};\n");
   printf ("  /* 'ff' = [y0, y1, x0, a1] */\n");
-  printf ("  static char f_tab[] = {10, 11,  8,  5};\n");
+  printf ("  static char const f_tab[] = {10, 11,  8,  5};\n");
   printf ("  /* 'xx' = [x0, x1, a0, a1]  */\n");
-  printf ("  static char x_tab[] = { 8,  9,  7,  5};\n");
+  printf ("  static char const x_tab[] = { 8,  9,  7,  5};\n");
   printf ("  /* 'yy' = [y0, y1, m0, m1]  */\n");
-  printf ("  static char y_tab[] = {10, 11, 12, 14};\n");
+  printf ("  static char const y_tab[] = {10, 11, 12, 14};\n");
   printf ("  /* 'gg' = [m0, m1, a0, a1]  */\n");
-  printf ("  static char g_tab[] = {12, 14,  7,  5};\n");
+  printf ("  static char const g_tab[] = {12, 14,  7,  5};\n");
   printf ("  /* 'uu' = [x0, y0, a0, a1]  */\n");
-  printf ("  static char u_tab[] = { 8, 10,  7,  5};\n");
+  printf ("  static char const u_tab[] = { 8, 10,  7,  5};\n");
   printf ("\n");
   printf ("  int z;\n");
   printf ("  int res, res_grd;\n");
-- 
2.33.0

[PATCH 5/6] sim: sh: fix uninitialized variable usage with pdmsb

[Mike Frysinger]

This block of code relies on i to control which bits to test and how
many times to run through the loop, but it never actually initialized
it.  There is another chunk of code that handles the pdmsb instruction
that sets i to 16, so use that here too assuming it's correct.  The
programming manual suggests this is the right value too, but I am by
no means a SuperH DSP expert.  The tests are still passing though ...
---
 sim/sh/gencode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c
index 9ef8b4610be1..80eecfdf1d36 100644
--- a/sim/sh/gencode.c
+++ b/sim/sh/gencode.c
@@ -2363,7 +2363,7 @@ op ppi_tab[] =
   },
   { "","", "(if cc) pdmsb Sy,Dz",	"101111cc..yyzzzz",
     "unsigned Sy = DSP_R (y);",
-    "int i;",
+    "int i = 16;",
     "",
     "if (Sy < 0)",
     "  Sy = ~Sy;",
-- 
2.33.0

[PATCH 6/6] sim: sh: enable -Werror everywhere

[Mike Frysinger]

With most of the warnings fixed in interp.c, we can enable -Werror
here too now.  There are some -Wmaybe-uninitialized warnings still
lurking that look legitimate, but we don't flag those are fatal,
and I don't have the expertise to dive into each opcode to figure
out the right way to clean them up.
---
 sim/sh/Makefile.in | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/sim/sh/Makefile.in b/sim/sh/Makefile.in
index 4c5b9ad9a0af..e58cd48f1ae2 100644
--- a/sim/sh/Makefile.in
+++ b/sim/sh/Makefile.in
@@ -27,7 +27,4 @@ SIM_OBJS = \
 SIM_EXTRA_LIBS = -lm
 SIM_EXTRA_DEPS = table.c code.c ppi.c
 
-# Some modules don't build cleanly yet.
-interp.o: SIM_WERROR_CFLAGS =
-
 ## COMMON_POST_CONFIG_FRAG
-- 
2.33.0

Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[Luis Machado]

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;
>   
> 

Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

On 11 Nov 2021 09:41, Luis Machado wrote:
> I can't pinpoint the exact SH patch, but builds are broken for 
> --enable-targets=all in Ubuntu 18.04 with GCC 7.5:

pretty sure the warnings have always been there for you, we just turned
-Werror in more cases recently

> 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;
>         ^~~

how exactly are you building things ?  we don't enable -Wstrict-overflow.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[Luis Machado]

On 11/11/21 7:25 PM, Mike Frysinger wrote:
> On 11 Nov 2021 09:41, Luis Machado wrote:
>> I can't pinpoint the exact SH patch, but builds are broken for
>> --enable-targets=all in Ubuntu 18.04 with GCC 7.5:
> 
> pretty sure the warnings have always been there for you, we just turned
> -Werror in more cases recently
> 

That's fine and I'm all for more -Werror. But builds should be able to 
complete regardless, right?

>> 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;
>>          ^~~
> 
> how exactly are you building things ?  we don't enable -Wstrict-overflow.

No special way. I just ./configure with --enable-targets=all and let it 
run. I can bisect it or gather a bit more information if you think it 
would be useful.

Worth mentioning that it doesn't run into such warnings/failures on 
Ubuntu 20.04 with gcc 9.3.

Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[Luis Machado]

On 11/11/21 7:32 PM, Luis Machado wrote:
> On 11/11/21 7:25 PM, Mike Frysinger wrote:
>> On 11 Nov 2021 09:41, Luis Machado wrote:
>>> I can't pinpoint the exact SH patch, but builds are broken for
>>> --enable-targets=all in Ubuntu 18.04 with GCC 7.5:
>>
>> pretty sure the warnings have always been there for you, we just turned
>> -Werror in more cases recently
>>
> 
> That's fine and I'm all for more -Werror. But builds should be able to 
> complete regardless, right?
> 
>>> 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;
>>>          ^~~
>>
>> how exactly are you building things ?  we don't enable -Wstrict-overflow.
> 
> No special way. I just ./configure with --enable-targets=all and let it 
> run. I can bisect it or gather a bit more information if you think it 
> would be useful.
> 
> Worth mentioning that it doesn't run into such warnings/failures on 
> Ubuntu 20.04 with gcc 9.3.

If you don't have a handy setup on your end, please let me know and I 
can take a look at it, of course.

Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 2780 bytes --]

On 11 Nov 2021 19:32, Luis Machado via Gdb-patches wrote:
> On 11/11/21 7:25 PM, Mike Frysinger wrote:
> > On 11 Nov 2021 09:41, Luis Machado wrote:
> >> I can't pinpoint the exact SH patch, but builds are broken for
> >> --enable-targets=all in Ubuntu 18.04 with GCC 7.5:
> > 
> > pretty sure the warnings have always been there for you, we just turned
> > -Werror in more cases recently
> 
> That's fine and I'm all for more -Werror. But builds should be able to 
> complete regardless, right?

they do build for me in a huge variety of configurations.  but i don't test
every random distro version that people might have.  plus for release, we
turn off automatic -Werror, so the issue you highlight only affects the
live development tree.  it's not going to break releases that distros use.

> >> 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;
> >>          ^~~
> > 
> > how exactly are you building things ?  we don't enable -Wstrict-overflow.
> 
> No special way. I just ./configure with --enable-targets=all and let it 
> run. I can bisect it or gather a bit more information if you think it 
> would be useful.

bisect would't really help.  as i said, the warnings have probably long been
there.  what changed is using -Werror in more places, and that commit is easy
to pick out.

> Worth mentioning that it doesn't run into such warnings/failures on 
> Ubuntu 20.04 with gcc 9.3.

it looks like gcc changed behavior starting in gcc-8.  in older versions,
-fstrict-overflow was enabled at -O2.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/2] sim: sh: rework carry checks to not rely on integer overflows

[Mike Frysinger]

In <=gcc-7 versions, -fstrict-overflow is enabled by default, and that
triggers warnings in this code that relies on integer overflows to test
for carries.  Change the logic to test against the limit directly.
---
 sim/sh/gencode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sim/sh/gencode.c b/sim/sh/gencode.c
index 80eecfdf1d36..5eb7caf25893 100644
--- a/sim/sh/gencode.c
+++ b/sim/sh/gencode.c
@@ -2266,7 +2266,7 @@ op ppi_tab[] =
     "int Sx_grd = GET_DSP_GRD (x);",
     "",
     "res = Sx - 0x10000;",
-    "carry = res > Sx;",
+    "carry = Sx < (INT_MIN + 0x10000);",
     "res_grd = Sx_grd - carry;",
     "COMPUTE_OVERFLOW;",
     "ADD_SUB_GE;",
@@ -2277,7 +2277,7 @@ op ppi_tab[] =
     "int Sx_grd = GET_DSP_GRD (x);",
     "",
     "res = Sx + 0x10000;",
-    "carry = res < Sx;",
+    "carry = Sx > (INT_MAX - 0x10000);",
     "res_grd = Sx_grd + carry;",
     "COMPUTE_OVERFLOW;",
     "ADD_SUB_GE;",
@@ -2288,7 +2288,7 @@ op ppi_tab[] =
     "int Sy_grd = SIGN32 (Sy);",
     "",
     "res = Sy - 0x10000;",
-    "carry = res > Sy;",
+    "carry = Sy < (INT_MIN + 0x10000);",
     "res_grd = Sy_grd - carry;",
     "COMPUTE_OVERFLOW;",
     "ADD_SUB_GE;",
@@ -2299,7 +2299,7 @@ op ppi_tab[] =
     "int Sy_grd = SIGN32 (Sy);",
     "",
     "res = Sy + 0x10000;",
-    "carry = res < Sy;",
+    "carry = Sy > (INT_MAX - 0x10000);",
     "res_grd = Sy_grd + carry;",
     "COMPUTE_OVERFLOW;",
     "ADD_SUB_GE;",
-- 
2.33.0

[PATCH 2/2] sim: sh: fix switch-bool warnings

[Mike Frysinger]

This code triggers -Werror=switch-bool warnings with <=gcc-5 versions.
Rework it to use if statements instead as it also simplifies a bit.
---
 sim/sh/interp.c | 79 ++++++++++++++++++-------------------------------
 1 file changed, 28 insertions(+), 51 deletions(-)

diff --git a/sim/sh/interp.c b/sim/sh/interp.c
index 93923fa2c56c..559b39a63226 100644
--- a/sim/sh/interp.c
+++ b/sim/sh/interp.c
@@ -1104,74 +1104,51 @@ div1 (int *R, int iRn2, int iRn1/*, int T*/)
   R[iRn1] <<= 1;
   R[iRn1] |= (unsigned long) T;
 
-  switch (old_q)
+  if (!old_q)
     {
-    case 0:
-      switch (M)
+      if (!M)
 	{
-	case 0:
 	  tmp0 = R[iRn1];
 	  R[iRn1] -= R[iRn2];
 	  tmp1 = (R[iRn1] > tmp0);
-	  switch (Q)
-	    {
-	    case 0:
-	      SET_SR_Q (tmp1);
-	      break;
-	    case 1:
-	      SET_SR_Q ((unsigned char) (tmp1 == 0));
-	      break;
-	    }
-	  break;
-	case 1:
+	  if (!Q)
+	    SET_SR_Q (tmp1);
+	  else
+	    SET_SR_Q ((unsigned char) (tmp1 == 0));
+	}
+      else
+	{
 	  tmp0 = R[iRn1];
 	  R[iRn1] += R[iRn2];
 	  tmp1 = (R[iRn1] < tmp0);
-	  switch (Q)
-	    {
-	    case 0:
-	      SET_SR_Q ((unsigned char) (tmp1 == 0));
-	      break;
-	    case 1:
-	      SET_SR_Q (tmp1);
-	      break;
-	    }
-	  break;
+	  if (!Q)
+	    SET_SR_Q ((unsigned char) (tmp1 == 0));
+	  else
+	    SET_SR_Q (tmp1);
 	}
-      break;
-    case 1:
-      switch (M)
+    }
+  else
+    {
+      if (!M)
 	{
-	case 0:
 	  tmp0 = R[iRn1];
 	  R[iRn1] += R[iRn2];
 	  tmp1 = (R[iRn1] < tmp0);
-	  switch (Q)
-	    {
-	    case 0:
-	      SET_SR_Q (tmp1);
-	      break;
-	    case 1:
-	      SET_SR_Q ((unsigned char) (tmp1 == 0));
-	      break;
-	    }
-	  break;
-	case 1:
+	  if (!Q)
+	    SET_SR_Q (tmp1);
+	  else
+	    SET_SR_Q ((unsigned char) (tmp1 == 0));
+	}
+      else
+	{
 	  tmp0 = R[iRn1];
 	  R[iRn1] -= R[iRn2];
 	  tmp1 = (R[iRn1] > tmp0);
-	  switch (Q)
-	    {
-	    case 0:
-	      SET_SR_Q ((unsigned char) (tmp1 == 0));
-	      break;
-	    case 1:
-	      SET_SR_Q (tmp1);
-	      break;
-	    }
-	  break;
+	  if (!Q)
+	    SET_SR_Q ((unsigned char) (tmp1 == 0));
+	  else
+	    SET_SR_Q (tmp1);
 	}
-      break;
     }
   /*T = (Q == M);*/
   SET_SR_T (Q == M);
-- 
2.33.0

Re: [PATCH 1/6] sim: sh: rework register layout with anonymous unions & structs

[Luis Machado]

On 11/11/21 7:45 PM, Mike Frysinger wrote:
> On 11 Nov 2021 19:32, Luis Machado via Gdb-patches wrote:
>> On 11/11/21 7:25 PM, Mike Frysinger wrote:
>>> On 11 Nov 2021 09:41, Luis Machado wrote:
>>>> I can't pinpoint the exact SH patch, but builds are broken for
>>>> --enable-targets=all in Ubuntu 18.04 with GCC 7.5:
>>>
>>> pretty sure the warnings have always been there for you, we just turned
>>> -Werror in more cases recently
>>
>> That's fine and I'm all for more -Werror. But builds should be able to
>> complete regardless, right?
> 
> they do build for me in a huge variety of configurations.  but i don't test
> every random distro version that people might have.  plus for release, we
> turn off automatic -Werror, so the issue you highlight only affects the
> live development tree.  it's not going to break releases that distros use.
> 

None of us test every possible combination. It is not easy to have high 
coverage without some sort of CI in place.

>>>> 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;
>>>>           ^~~
>>>
>>> how exactly are you building things ?  we don't enable -Wstrict-overflow.
>>
>> No special way. I just ./configure with --enable-targets=all and let it
>> run. I can bisect it or gather a bit more information if you think it
>> would be useful.
> 
> bisect would't really help.  as i said, the warnings have probably long been
> there.  what changed is using -Werror in more places, and that commit is easy
> to pick out.
> 
>> Worth mentioning that it doesn't run into such warnings/failures on
>> Ubuntu 20.04 with gcc 9.3.
> 
> it looks like gcc changed behavior starting in gcc-8.  in older versions,
> -fstrict-overflow was enabled at -O2.

Since I have it handy here, let me go through it and try to come up with 
a fix for the ones I'm seeing.