public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* i386-dis.c UB shift and other tidies
@ 2023-04-26  4:39 Alan Modra
  2023-04-26  8:09 ` Jan Beulich
  0 siblings, 1 reply; 3+ messages in thread
From: Alan Modra @ 2023-04-26  4:39 UTC (permalink / raw)
  To: binutils

1) i386-dis.c:12055:11: runtime error: left shift of negative value -1
Bit twiddling is best done unsigned, due to UB on overflow of signed
expressions.  Fix this by using bfd_vma rather than bfd_signed_vma
everywhere in i386-dis.c except print_displacement.

2) Return get32s and get16 value in a bfd_vma, reducing the need for
temp variables.

3) Introduce get16s and get8s functions to simplify the code.

4) With some optimisation options gcc-13 legitimately complains about
a fall-through in OP_I.  Fix that.  OP_I also doesn't need to use
"mask" which was wrong for w_mode anyway.

5) Masking with & 0xffffffff is better than casting to unsigned.  We
don't know for sure that unsigned int is 32-bit.

6) We also don't know that unsigned char is 8 bits.  Mask codep
accesses everywhere.  I don't expect binutils will work on anything
other than an 8-bit char host, but if we are masking codep accesses in
some places we might as well be consistent.  (Better would be to use
stdint.h types more in binutils.)

diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index 01e5ba81723..100ec43b189 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -47,8 +47,10 @@ static void oappend_with_style (instr_info *, const char *,
 				enum disassembler_style);
 static void oappend (instr_info *, const char *);
 static void append_seg (instr_info *);
-static bool get32s (instr_info *, bfd_signed_vma *);
-static bool get16 (instr_info *, int *);
+static bool get32s (instr_info *, bfd_vma *);
+static bool get16 (instr_info *, bfd_vma *);
+static bool get16s (instr_info *, bfd_vma *);
+static bool get8s (instr_info *, bfd_vma *);
 static void set_op (instr_info *, bfd_vma, bool);
 
 static bool OP_E (instr_info *, int, int);
@@ -8934,7 +8936,7 @@ ckprefix (instr_info *ins)
       if (!fetch_code (ins->info, ins->codep + 1))
 	return ckp_fetch_error;
       newrex = 0;
-      switch (*ins->codep)
+      switch (*ins->codep & 0xff)
 	{
 	/* REX prefixes family.  */
 	case 0x40:
@@ -8954,7 +8956,7 @@ ckprefix (instr_info *ins)
 	case 0x4e:
 	case 0x4f:
 	  if (ins->address_mode == mode_64bit)
-	    newrex = *ins->codep;
+	    newrex = *ins->codep & 0xff;
 	  else
 	    return ckp_okay;
 	  ins->last_rex_prefix = i;
@@ -9034,8 +9036,8 @@ ckprefix (instr_info *ins)
       /* Rex is ignored when followed by another prefix.  */
       if (ins->rex)
 	return ckp_bogus;
-      if (*ins->codep != FWAIT_OPCODE)
-	ins->all_prefixes[i++] = *ins->codep;
+      if ((*ins->codep & 0xff) != FWAIT_OPCODE)
+	ins->all_prefixes[i++] = *ins->codep & 0xff;
       ins->rex = newrex;
       ins->codep++;
       length++;
@@ -9266,7 +9268,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
     case USE_3BYTE_TABLE:
       if (!fetch_code (ins->info, ins->codep + 2))
 	return &err_opcode;
-      vindex = *ins->codep++;
+      vindex = *ins->codep++ & 0xff;
       dp = &three_byte_table[dp->op[1].bytemode][vindex];
       ins->end_codep = ins->codep;
       if (!fetch_modrm (ins))
@@ -9373,7 +9375,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
 	}
       ins->need_vex = true;
       ins->codep++;
-      vindex = *ins->codep++;
+      vindex = *ins->codep++ & 0xff;
       dp = &xop_table[vex_table_index][vindex];
 
       ins->end_codep = ins->codep;
@@ -9438,7 +9440,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
 	}
       ins->need_vex = true;
       ins->codep++;
-      vindex = *ins->codep++;
+      vindex = *ins->codep++ & 0xff;
       dp = &vex_table[vex_table_index][vindex];
       ins->end_codep = ins->codep;
       /* There is no MODRM byte for VEX0F 77.  */
@@ -9473,7 +9475,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
 	}
       ins->need_vex = true;
       ins->codep++;
-      vindex = *ins->codep++;
+      vindex = *ins->codep++ & 0xff;
       dp = &vex_table[dp->op[1].bytemode][vindex];
       ins->end_codep = ins->codep;
       /* There is no MODRM byte for VEX 77.  */
@@ -9565,7 +9567,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
 
       ins->need_vex = true;
       ins->codep++;
-      vindex = *ins->codep++;
+      vindex = *ins->codep++ & 0xff;
       dp = &evex_table[vex_table_index][vindex];
       ins->end_codep = ins->codep;
       if (!fetch_modrm (ins))
@@ -9904,10 +9906,11 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
       goto out;
     }
 
-  ins.two_source_ops = (*ins.codep == 0x62) || (*ins.codep == 0xc8);
+  ins.two_source_ops = ((*ins.codep & 0xff) == 0x62
+			|| (*ins.codep & 0xff) == 0xc8);
 
-  if (((ins.prefixes & PREFIX_FWAIT)
-       && ((*ins.codep < 0xd8) || (*ins.codep > 0xdf))))
+  if ((ins.prefixes & PREFIX_FWAIT)
+      && ((*ins.codep & 0xff) < 0xd8 || (*ins.codep & 0xff) > 0xdf))
     {
       /* Handle ins.prefixes before fwait.  */
       for (i = 0; i < ins.fwait_prefix && ins.all_prefixes[i];
@@ -9919,22 +9922,22 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
       goto out;
     }
 
-  if (*ins.codep == 0x0f)
+  if ((*ins.codep & 0xff) == 0x0f)
     {
       unsigned char threebyte;
 
       ins.codep++;
       if (!fetch_code (info, ins.codep + 1))
 	goto fetch_error_out;
-      threebyte = *ins.codep;
+      threebyte = *ins.codep & 0xff;
       dp = &dis386_twobyte[threebyte];
       ins.need_modrm = twobyte_has_modrm[threebyte];
       ins.codep++;
     }
   else
     {
-      dp = &dis386[*ins.codep];
-      ins.need_modrm = onebyte_has_modrm[*ins.codep];
+      dp = &dis386[*ins.codep & 0xff];
+      ins.need_modrm = onebyte_has_modrm[*ins.codep & 0xff];
       ins.codep++;
     }
 
@@ -10635,7 +10638,7 @@ dofloat (instr_info *ins, int sizeflag)
   const struct dis386 *dp;
   unsigned char floatop;
 
-  floatop = ins->codep[-1];
+  floatop = ins->codep[-1] & 0xff;
 
   if (ins->modrm.mod != 3)
     {
@@ -10656,7 +10659,7 @@ dofloat (instr_info *ins, int sizeflag)
       putop (ins, fgrps[dp->op[0].bytemode][ins->modrm.rm], sizeflag);
 
       /* Instruction fnstsw is only one with strange arg.  */
-      if (floatop == 0xdf && ins->codep[-1] == 0xe0)
+      if (floatop == 0xdf && (ins->codep[-1] & 0xff) == 0xe0)
 	strcpy (ins->op_out[0], att_names16[0] + ins->intel_syntax);
     }
   else
@@ -11399,10 +11402,9 @@ print_operand_value (instr_info *ins, bfd_vma disp,
 {
   char tmp[30];
 
-  if (ins->address_mode == mode_64bit)
-    sprintf (tmp, "0x%" PRIx64, (uint64_t) disp);
-  else
-    sprintf (tmp, "0x%x", (unsigned int) disp);
+  if (ins->address_mode != mode_64bit)
+    disp &= 0xffffffff;
+  sprintf (tmp, "0x%" PRIx64, (uint64_t) disp);
   oappend_with_style (ins, tmp, style);
 }
 
@@ -11944,7 +11946,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
   if ((sizeflag & AFLAG) || ins->address_mode == mode_64bit)
     {
       /* 32/64 bit address mode */
-      bfd_signed_vma disp = 0;
+      bfd_vma disp = 0;
       int havedisp;
       int havebase;
       int needindex;
@@ -12046,11 +12048,8 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
 	    }
 	  break;
 	case 1:
-	  if (!fetch_code (ins->info, ins->codep + 1))
+	  if (!get8s (ins, &disp))
 	    return false;
-	  disp = *ins->codep++;
-	  if ((disp & 0x80) != 0)
-	    disp -= 0x100;
 	  if (ins->vex.evex && shift > 0)
 	    disp <<= shift;
 	  break;
@@ -12073,7 +12072,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
 		{
 		  /* Without base nor index registers, zero-extend the
 		     lower 32-bit displacement to 64 bits.  */
-		  disp = (unsigned int) disp;
+		  disp &= 0xffffffff;
 		  needindex = 1;
 		}
 	      needaddr32 = 1;
@@ -12162,7 +12161,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
 	  if (ins->intel_syntax
 	      && (disp || ins->modrm.mod != 0 || base == 5))
 	    {
-	      if (!havedisp || disp >= 0)
+	      if (!havedisp || (bfd_signed_vma) disp >= 0)
 		  oappend_char (ins, '+');
 	      if (havedisp)
 		print_displacement (ins, disp);
@@ -12211,7 +12210,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
   else
     {
       /* 16 bit address mode */
-      int disp = 0;
+      bfd_vma disp = 0;
 
       ins->used_prefixes |= ins->prefixes & PREFIX_ADDR;
       switch (ins->modrm.mod)
@@ -12220,18 +12219,13 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
 	  if (ins->modrm.rm == 6)
 	    {
 	case 2:
-	      if (!get16 (ins, &disp))
+	      if (!get16s (ins, &disp))
 		return false;
-	      if ((disp & 0x8000) != 0)
-		disp -= 0x10000;
 	    }
 	  break;
 	case 1:
-	  if (!fetch_code (ins->info, ins->codep + 1))
+	  if (!get8s (ins, &disp))
 	    return false;
-	  disp = *ins->codep++;
-	  if ((disp & 0x80) != 0)
-	    disp -= 0x100;
 	  if (ins->vex.evex && shift > 0)
 	    disp <<= shift;
 	  break;
@@ -12249,7 +12243,7 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
 	  if (ins->intel_syntax
 	      && (disp || ins->modrm.mod != 0 || ins->modrm.rm == 6))
 	    {
-	      if (disp >= 0)
+	      if ((bfd_signed_vma) disp >= 0)
 		oappend_char (ins, '+');
 	      print_displacement (ins, disp);
 	    }
@@ -12397,7 +12391,7 @@ get64 (instr_info *ins, uint64_t *res)
 }
 
 static bool
-get32 (instr_info *ins, bfd_signed_vma *res)
+get32 (instr_info *ins, bfd_vma *res)
 {
   if (!fetch_code (ins->info, ins->codep + 4))
     return false;
@@ -12409,7 +12403,7 @@ get32 (instr_info *ins, bfd_signed_vma *res)
 }
 
 static bool
-get32s (instr_info *ins, bfd_signed_vma *res)
+get32s (instr_info *ins, bfd_vma *res)
 {
   if (!get32 (ins, res))
     return false;
@@ -12420,12 +12414,30 @@ get32s (instr_info *ins, bfd_signed_vma *res)
 }
 
 static bool
-get16 (instr_info *ins, int *res)
+get16 (instr_info *ins, bfd_vma *res)
 {
   if (!fetch_code (ins->info, ins->codep + 2))
     return false;
-  *res = *ins->codep++ & 0xff;
-  *res |= (*ins->codep++ & 0xff) << 8;
+  *res = (bfd_vma) *ins->codep++ & 0xff;
+  *res |= ((bfd_vma) *ins->codep++ & 0xff) << 8;
+  return true;
+}
+
+static bool
+get16s (instr_info *ins, bfd_vma *res)
+{
+  if (!get16 (ins, res))
+    return false;
+  *res = (*res ^ 0x8000) - 0x8000;
+  return true;
+}
+
+static bool
+get8s (instr_info *ins, bfd_vma *res)
+{
+  if (!fetch_code (ins->info, ins->codep + 1))
+    return false;
+  *res = (((bfd_vma) *ins->codep++ & 0xff) ^ 0x80) - 0x80;
   return true;
 }
 
@@ -12552,16 +12564,14 @@ OP_IMREG (instr_info *ins, int code, int sizeflag)
 static bool
 OP_I (instr_info *ins, int bytemode, int sizeflag)
 {
-  bfd_signed_vma op;
-  bfd_signed_vma mask = -1;
+  bfd_vma op;
 
   switch (bytemode)
     {
     case b_mode:
       if (!fetch_code (ins->info, ins->codep + 1))
 	return false;
-      op = *ins->codep++;
-      mask = 0xff;
+      op = *ins->codep++ & 0xff;
       break;
     case v_mode:
       USED_REX (REX_W);
@@ -12578,17 +12588,13 @@ OP_I (instr_info *ins, int bytemode, int sizeflag)
     case d_mode:
 	      if (!get32 (ins, &op))
 		return false;
-	      mask = 0xffffffff;
 	    }
 	  else
 	    {
-	      int num;
-
+	      /* Fall through.  */
     case w_mode:
-	      if (!get16 (ins, &num))
+	      if (!get16 (ins, &op))
 		return false;
-	      op = num;
-	      mask = 0xfffff;
 	    }
 	}
       break;
@@ -12601,7 +12607,6 @@ OP_I (instr_info *ins, int bytemode, int sizeflag)
       return true;
     }
 
-  op &= mask;
   oappend_immediate (ins, op);
   return true;
 }
@@ -12627,17 +12632,14 @@ OP_I64 (instr_info *ins, int bytemode, int sizeflag)
 static bool
 OP_sI (instr_info *ins, int bytemode, int sizeflag)
 {
-  bfd_signed_vma op;
+  bfd_vma op;
 
   switch (bytemode)
     {
     case b_mode:
     case b_T_mode:
-      if (!fetch_code (ins->info, ins->codep + 1))
+      if (!get8s (ins, &op))
 	return false;
-      op = *ins->codep++;
-      if ((op & 0x80) != 0)
-	op -= 0x100;
       if (bytemode == b_T_mode)
 	{
 	  if (ins->address_mode != mode_64bit
@@ -12665,11 +12667,8 @@ OP_sI (instr_info *ins, int bytemode, int sizeflag)
       /* The operand-size prefix is overridden by a REX prefix.  */
       if (!(sizeflag & DFLAG) && !(ins->rex & REX_W))
 	{
-	  int val;
-
-	  if (!get16 (ins, &val))
+	  if (!get16 (ins, &op))
 	    return false;
-	  op = val;
 	}
       else if (!get32s (ins, &op))
 	return false;
@@ -12693,11 +12692,8 @@ OP_J (instr_info *ins, int bytemode, int sizeflag)
   switch (bytemode)
     {
     case b_mode:
-      if (!fetch_code (ins->info, ins->codep + 1))
+      if (!get8s (ins, &disp))
 	return false;
-      disp = *ins->codep++;
-      if ((disp & 0x80) != 0)
-	disp -= 0x100;
       break;
     case v_mode:
     case dqw_mode:
@@ -12706,19 +12702,13 @@ OP_J (instr_info *ins, int bytemode, int sizeflag)
 	      && ((ins->isa64 == intel64 && bytemode != dqw_mode)
 		  || (ins->rex & REX_W))))
 	{
-	  bfd_signed_vma val;
-
-	  if (!get32s (ins, &val))
+	  if (!get32s (ins, &disp))
 	    return false;
-	  disp = val;
 	}
       else
 	{
-	  int val;
-
-	  if (!get16 (ins, &val))
+	  if (!get16s (ins, &disp))
 	    return false;
-	  disp = val & 0x8000 ? val - 0x10000 : val;
 	  /* In 16bit mode, address is wrapped around at 64k within
 	     the same segment.  Otherwise, a data16 prefix on a jump
 	     instruction means that the pc is masked to 16 bits after
@@ -12757,16 +12747,14 @@ OP_SEG (instr_info *ins, int bytemode, int sizeflag)
 static bool
 OP_DIR (instr_info *ins, int dummy ATTRIBUTE_UNUSED, int sizeflag)
 {
-  int seg, offset, res;
+  bfd_vma seg, offset;
+  int res;
   char scratch[24];
 
   if (sizeflag & DFLAG)
     {
-      bfd_signed_vma val;
-
-      if (!get32 (ins, &val))
+      if (!get32 (ins, &offset))
 	return false;;
-      offset = val;
     }
   else if (!get16 (ins, &offset))
     return false;
@@ -12776,7 +12764,7 @@ OP_DIR (instr_info *ins, int dummy ATTRIBUTE_UNUSED, int sizeflag)
 
   res = snprintf (scratch, ARRAY_SIZE (scratch),
 		  ins->intel_syntax ? "0x%x:0x%x" : "$0x%x,$0x%x",
-		  seg, offset);
+		  (unsigned) seg, (unsigned) offset);
   if (res < 0 || (size_t) res >= ARRAY_SIZE (scratch))
     abort ();
   oappend (ins, scratch);
@@ -12794,19 +12782,13 @@ OP_OFF (instr_info *ins, int bytemode, int sizeflag)
 
   if ((sizeflag & AFLAG) || ins->address_mode == mode_64bit)
     {
-      bfd_signed_vma val;
-
-      if (!get32 (ins, &val))
+      if (!get32 (ins, &off))
 	return false;
-      off = val;
     }
   else
     {
-      int val;
-
-      if (!get16 (ins, &val))
+      if (!get16 (ins, &off))
 	return false;
-      off = val;
     }
 
   if (ins->intel_syntax)
@@ -12876,7 +12858,7 @@ OP_ESreg (instr_info *ins, int code, int sizeflag)
 {
   if (ins->intel_syntax)
     {
-      switch (ins->codep[-1])
+      switch (ins->codep[-1] & 0xff)
 	{
 	case 0x6d:	/* insw/insl */
 	  intel_operand_size (ins, z_mode, sizeflag);
@@ -12902,7 +12884,7 @@ OP_DSreg (instr_info *ins, int code, int sizeflag)
 {
   if (ins->intel_syntax)
     {
-      switch (ins->codep[-1])
+      switch (ins->codep[-1] & 0xff)
 	{
 	case 0x6f:	/* outsw/outsl */
 	  intel_operand_size (ins, z_mode, sizeflag);
@@ -13872,7 +13854,7 @@ OP_REG_VexI4 (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED)
 
   if (!fetch_code (ins->info, ins->codep + 1))
     return false;
-  reg = *ins->codep++;
+  reg = *ins->codep++ & 0xff;
 
   if (bytemode != x_mode && bytemode != scalar_mode)
     abort ();

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: i386-dis.c UB shift and other tidies
  2023-04-26  4:39 i386-dis.c UB shift and other tidies Alan Modra
@ 2023-04-26  8:09 ` Jan Beulich
  2023-04-26  9:56   ` Alan Modra
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2023-04-26  8:09 UTC (permalink / raw)
  To: Alan Modra; +Cc: binutils

On 26.04.2023 06:39, Alan Modra via Binutils wrote:
> 1) i386-dis.c:12055:11: runtime error: left shift of negative value -1
> Bit twiddling is best done unsigned, due to UB on overflow of signed
> expressions.  Fix this by using bfd_vma rather than bfd_signed_vma
> everywhere in i386-dis.c except print_displacement.
> 
> 2) Return get32s and get16 value in a bfd_vma, reducing the need for
> temp variables.

While I'm okay with most other changes, I find it pretty odd for any of
...

> 3) Introduce get16s and get8s functions to simplify the code.

... the get<N>s() functions to return an unsigned quantity. This then
leads to ugly casting in at least OP_E_memory() (once explicit, once
implicit). If they want to do some of the calculations (shifts in
particular, as you say) using unsigned intermediate types, that's of
course fine.

As a minor remark - because of you switching some of the get16() to
get16s(), get16() doesn't need a forward declaration anymore (just
like get32() and get64() don't have such [anymore]). (Ultimately I'd
like to get rid of as many forward declarations of static functions
as possible, because this always requires touching yet one more place
when changing their signatures.)

> 4) With some optimisation options gcc-13 legitimately complains about
> a fall-through in OP_I.  Fix that.  OP_I also doesn't need to use
> "mask" which was wrong for w_mode anyway.

While making the earlier recent change I was puzzled by that, too,
but I deliberately left it untouched. I'm afraid we don't really have
any test for it, and it looked to me as if that masking was
intentionally done that (odd) way. (This isn't an objection to the
change, but I wouldn't be surprised if something subtly broke, which
we'd then need to take care of later on.)

> 5) Masking with & 0xffffffff is better than casting to unsigned.  We
> don't know for sure that unsigned int is 32-bit.
> 
> 6) We also don't know that unsigned char is 8 bits.  Mask codep
> accesses everywhere.  I don't expect binutils will work on anything
> other than an 8-bit char host, but if we are masking codep accesses in
> some places we might as well be consistent.  (Better would be to use
> stdint.h types more in binutils.)

Would there be anything wrong with switching codep to uint8_t * right
away, in place of all the masking by 0xff that you add? When also done
for its *_codep siblings, at the first glance this looks to not require
much further touching (and hence presumably overall less code churn;
existing masking that then clearly isn't necessary anymore could of
course be purged right away, but this could also be left for later).

Jan

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: i386-dis.c UB shift and other tidies
  2023-04-26  8:09 ` Jan Beulich
@ 2023-04-26  9:56   ` Alan Modra
  0 siblings, 0 replies; 3+ messages in thread
From: Alan Modra @ 2023-04-26  9:56 UTC (permalink / raw)
  To: Jan Beulich; +Cc: binutils

On Wed, Apr 26, 2023 at 10:09:13AM +0200, Jan Beulich wrote:
> On 26.04.2023 06:39, Alan Modra via Binutils wrote:
> > 1) i386-dis.c:12055:11: runtime error: left shift of negative value -1
> > Bit twiddling is best done unsigned, due to UB on overflow of signed
> > expressions.  Fix this by using bfd_vma rather than bfd_signed_vma
> > everywhere in i386-dis.c except print_displacement.
> > 
> > 2) Return get32s and get16 value in a bfd_vma, reducing the need for
> > temp variables.
> 
> While I'm okay with most other changes, I find it pretty odd for any of
> ...
> 
> > 3) Introduce get16s and get8s functions to simplify the code.
> 
> ... the get<N>s() functions to return an unsigned quantity.

Change it back if you find it ugly, but I've found it better to
default to using unsigned types of the largest width you are typically
going to manipluate.

> This then
> leads to ugly casting in at least OP_E_memory() (once explicit, once
> implicit). If they want to do some of the calculations (shifts in
> particular, as you say) using unsigned intermediate types, that's of
> course fine.

Which is ugly too, and we often forget to avoid UB by casts or
assigning to unsigned types.

> As a minor remark - because of you switching some of the get16() to
> get16s(), get16() doesn't need a forward declaration anymore (just
> like get32() and get64() don't have such [anymore]). (Ultimately I'd
> like to get rid of as many forward declarations of static functions
> as possible, because this always requires touching yet one more place
> when changing their signatures.)

I missed that.  Maybe you should move things around in the file?

> > 4) With some optimisation options gcc-13 legitimately complains about
> > a fall-through in OP_I.  Fix that.  OP_I also doesn't need to use
> > "mask" which was wrong for w_mode anyway.
> 
> While making the earlier recent change I was puzzled by that, too,
> but I deliberately left it untouched. I'm afraid we don't really have
> any test for it, and it looked to me as if that masking was
> intentionally done that (odd) way. (This isn't an objection to the
> change, but I wouldn't be surprised if something subtly broke, which
> we'd then need to take care of later on.)

I checked carefully.  It really is unneeded when you consider the
range of values returned by the get* functions.  get16 is limited to
[0,0xffff], so masking with 0xfffff does nothing.

> > 5) Masking with & 0xffffffff is better than casting to unsigned.  We
> > don't know for sure that unsigned int is 32-bit.
> > 
> > 6) We also don't know that unsigned char is 8 bits.  Mask codep
> > accesses everywhere.  I don't expect binutils will work on anything
> > other than an 8-bit char host, but if we are masking codep accesses in
> > some places we might as well be consistent.  (Better would be to use
> > stdint.h types more in binutils.)
> 
> Would there be anything wrong with switching codep to uint8_t * right
> away, in place of all the masking by 0xff that you add? When also done
> for its *_codep siblings, at the first glance this looks to not require
> much further touching (and hence presumably overall less code churn;
> existing masking that then clearly isn't necessary anymore could of
> course be purged right away, but this could also be left for later).

I think switching would be a good idea.  I've changed bfd_byte (not
yet committed) and that seems good.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-04-26  9:56 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-26  4:39 i386-dis.c UB shift and other tidies Alan Modra
2023-04-26  8:09 ` Jan Beulich
2023-04-26  9:56   ` Alan Modra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).