On Friday 09 December 2011 15:14:26 Yann Vernier wrote:
> Confirmed bug #1 and that the patch given works. It's not a very clean
> solution; the file should be rewritten with proper care for data types.
> Explanatory comments are missing and or32_extract is duplicated (with
> different functionality!).
>
> Attaching a slight variant which should also make sign extension for signed
> fields work properly in case long is used.
>
> I don't have permissions to edit bugs in the bugzilla, so I couldn't post
> the patch there. Jeremy, care to check this out and in?
I think I've found each of the places this bug particular occurs in (severe
code duplication - shouldn't this be used from binutils libopcodes somehow?).
Attached is a patch that changes a bunch of literals to values with semantics
more like intented. The clean solution would probably involve rebuilding the
cgen model, making sure cgen doesn't generate this bug, and rebuilding all
these different places from the same source. Proper handling would probably
mean using uint32_t rather than unsigned long (which in this case was 64bit).
The bug had previously been addressed by masking the output values in a few
places, which is why we saw it in gdb but not binutils. Those changes are
commented 090430 - jb, and should be removed when this works correctly.
Please try it and let me know how it goes (about to test myself, waiting for
compilation - but second eyes and all that).
Index: gnu-src/binutils-2.18.50/opcodes/or32-dis.c
===================================================================
--- gnu-src/binutils-2.18.50/opcodes/or32-dis.c (revision 663)
+++ gnu-src/binutils-2.18.50/opcodes/or32-dis.c (working copy)
@@ -28,7 +28,7 @@
#include <string.h>
#include <stdlib.h>
-#define EXTEND28(x) ((x) & (unsigned long) 0x08000000 ? ((x) | (unsigned long) 0xf0000000) : ((x)))
+#define EXTEND28(x) ((x) & 0x08000000UL ? ((x) | ~0x0fffffffUL) : ((x)))
/* Now find the four bytes of INSN_CH and put them in *INSN. */
@@ -100,7 +100,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
@@ -120,7 +120,7 @@
printf ("\n ret=%x opc_pos=%x, param_pos=%x\n",
ret, opc_pos, param_pos);
#endif
- ret |= 0xffffffff << letter_range(param_ch);
+ ret |= -1L << letter_range(param_ch);
#if DEBUG
printf ("\n after conversion to signed: ret=%x\n", ret);
#endif
Index: gnu-src/binutils-2.18.50/opcodes/or32-opc.c
===================================================================
--- gnu-src/binutils-2.18.50/opcodes/or32-opc.c (revision 663)
+++ gnu-src/binutils-2.18.50/opcodes/or32-opc.c (working copy)
@@ -890,7 +890,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -942,7 +942,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
Index: gnu-src/binutils-2.20.1/opcodes/or32-dis.c
===================================================================
--- gnu-src/binutils-2.20.1/opcodes/or32-dis.c (revision 663)
+++ gnu-src/binutils-2.20.1/opcodes/or32-dis.c (working copy)
@@ -28,7 +28,7 @@
#include <string.h>
#include <stdlib.h>
-#define EXTEND28(x) ((x) & (unsigned long) 0x08000000 ? ((x) | (unsigned long) 0xf0000000) : ((x)))
+#define EXTEND28(x) ((x) & 0x08000000UL ? ((x) | ~0x0fffffffUL) : ((x)))
/* Now find the four bytes of INSN_CH and put them in *INSN. */
@@ -100,7 +100,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
@@ -120,7 +120,7 @@
printf ("\n ret=%x opc_pos=%x, param_pos=%x\n",
ret, opc_pos, param_pos);
#endif
- ret |= 0xffffffff << letter_range(param_ch);
+ ret |= -1L << letter_range(param_ch);
#if DEBUG
printf ("\n after conversion to signed: ret=%x\n", ret);
#endif
Index: gnu-src/binutils-2.20.1/opcodes/or32-opc.c
===================================================================
--- gnu-src/binutils-2.20.1/opcodes/or32-opc.c (revision 663)
+++ gnu-src/binutils-2.20.1/opcodes/or32-opc.c (working copy)
@@ -891,7 +891,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -943,7 +943,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
Index: gnu-src/gdb-6.8/opcodes/or32-dis.c
===================================================================
--- gnu-src/gdb-6.8/opcodes/or32-dis.c (revision 663)
+++ gnu-src/gdb-6.8/opcodes/or32-dis.c (working copy)
@@ -28,7 +28,7 @@
#include <string.h>
#include <stdlib.h>
-#define EXTEND28(x) ((x) & (unsigned long) 0x08000000 ? ((x) | (unsigned long) 0xf0000000) : ((x)))
+#define EXTEND28(x) ((x) & 0x08000000UL ? ((x) | ~0x0fffffffUL) : ((x)))
/* Now find the four bytes of INSN_CH and put them in *INSN. */
@@ -100,7 +100,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
@@ -120,7 +120,7 @@
printf ("\n ret=%x opc_pos=%x, param_pos=%x\n",
ret, opc_pos, param_pos);
#endif
- ret |= 0xffffffff << letter_range(param_ch);
+ ret |= -1L << letter_range(param_ch);
#if DEBUG
printf ("\n after conversion to signed: ret=%x\n", ret);
#endif
Index: gnu-src/gdb-6.8/opcodes/or32-opc.c
===================================================================
--- gnu-src/gdb-6.8/opcodes/or32-opc.c (revision 663)
+++ gnu-src/gdb-6.8/opcodes/or32-opc.c (working copy)
@@ -891,7 +891,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -943,7 +943,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
Index: gnu-src/gdb-7.1/opcodes/or32-dis.c
===================================================================
--- gnu-src/gdb-7.1/opcodes/or32-dis.c (revision 663)
+++ gnu-src/gdb-7.1/opcodes/or32-dis.c (working copy)
@@ -28,7 +28,7 @@
#include <string.h>
#include <stdlib.h>
-#define EXTEND29(x) ((x) & (unsigned long) 0x10000000 ? ((x) | (unsigned long) 0xf0000000) : ((x)))
+#define EXTEND29(x) ((x) & 0x10000000UL ? ((x) | ~0x0fffffffUL) : ((x)))
/* Now find the four bytes of INSN_CH and put them in *INSN. */
@@ -100,7 +100,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
@@ -120,7 +120,7 @@
printf ("\n ret=%x opc_pos=%x, param_pos=%x\n",
ret, opc_pos, param_pos);
#endif
- ret |= 0xffffffff << letter_range(param_ch);
+ ret |= -1L << letter_range(param_ch);
#if DEBUG
printf ("\n after conversion to signed: ret=%x\n", ret);
#endif
Index: gnu-src/gdb-7.1/opcodes/or32-opc.c
===================================================================
--- gnu-src/gdb-7.1/opcodes/or32-opc.c (revision 663)
+++ gnu-src/gdb-7.1/opcodes/or32-opc.c (working copy)
@@ -858,7 +858,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -910,7 +910,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
Index: gnu-src/gdb-7.2/opcodes/or32-dis.c
===================================================================
--- gnu-src/gdb-7.2/opcodes/or32-dis.c (revision 663)
+++ gnu-src/gdb-7.2/opcodes/or32-dis.c (working copy)
@@ -28,7 +28,7 @@
#include <string.h>
#include <stdlib.h>
-#define EXTEND29(x) ((x) & (unsigned long) 0x10000000 ? ((x) | (unsigned long) 0xf0000000) : ((x)))
+#define EXTEND29(x) ((x) & 0x10000000UL ? ((x) | ~0x0fffffffUL) : ((x)))
/* Now find the four bytes of INSN_CH and put them in *INSN. */
@@ -57,6 +57,8 @@
typedef void (*find_byte_func_type) (unsigned char *, unsigned long *);
+/* Extract a field according to an opcode format string (see or32-opc.c)
+ * Sign extends those that are letter_signed() despite unsigned return type */
static unsigned long
or32_extract (char param_ch, char *enc_initial, unsigned long insn)
{
@@ -100,7 +102,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
@@ -120,7 +122,7 @@
printf ("\n ret=%x opc_pos=%x, param_pos=%x\n",
ret, opc_pos, param_pos);
#endif
- ret |= 0xffffffff << letter_range(param_ch);
+ ret |= -1L << letter_range(param_ch);
#if DEBUG
printf ("\n after conversion to signed: ret=%x\n", ret);
#endif
Index: gnu-src/gdb-7.2/opcodes/or32-opc.c
===================================================================
--- gnu-src/gdb-7.2/opcodes/or32-opc.c (revision 663)
+++ gnu-src/gdb-7.2/opcodes/or32-opc.c (working copy)
@@ -858,7 +858,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -868,6 +868,8 @@
return imm;
}
+/* Extract a field according to an opcode format string (see or32_opcodes)
+ * Note: a nearly identical function is also present in or32-dis.c */
static unsigned long
or32_extract (char param_ch, char *enc_initial, unsigned long insn)
{
@@ -910,7 +912,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
Index: gnu-src/unisrc/opcodes/or32-dis.c
===================================================================
--- gnu-src/unisrc/opcodes/or32-dis.c (revision 663)
+++ gnu-src/unisrc/opcodes/or32-dis.c (working copy)
@@ -28,7 +28,7 @@
#include <string.h>
#include <stdlib.h>
-#define EXTEND28(x) ((x) & (unsigned long) 0x08000000 ? ((x) | (unsigned long) 0xf0000000) : ((x)))
+#define EXTEND28(x) ((x) & 0x08000000UL ? ((x) | ~0x0fffffffUL) : ((x)))
/* Now find the four bytes of INSN_CH and put them in *INSN. */
@@ -100,7 +100,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
@@ -120,7 +120,7 @@
printf ("\n ret=%x opc_pos=%x, param_pos=%x\n",
ret, opc_pos, param_pos);
#endif
- ret |= 0xffffffff << letter_range(param_ch);
+ ret |= -1L << letter_range(param_ch);
#if DEBUG
printf ("\n after conversion to signed: ret=%x\n", ret);
#endif
Index: gnu-src/unisrc/opcodes/or32-opc.c
===================================================================
--- gnu-src/unisrc/opcodes/or32-opc.c (revision 663)
+++ gnu-src/unisrc/opcodes/or32-opc.c (working copy)
@@ -891,7 +891,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -943,7 +943,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
Index: or1ksim/cpu/or32/or32.c
===================================================================
--- or1ksim/cpu/or32/or32.c (revision 663)
+++ or1ksim/cpu/or32/or32.c (working copy)
@@ -1101,7 +1101,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -1156,7 +1156,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
Index: orpsocv2/sw/utils/or32-idecode/or32-dis.c
===================================================================
--- orpsocv2/sw/utils/or32-idecode/or32-dis.c (revision 663)
+++ orpsocv2/sw/utils/or32-idecode/or32-dis.c (working copy)
@@ -29,7 +29,7 @@
#include <string.h>
#include <stdlib.h>
#include <stdio.h>
-#define EXTEND28(x) ((x) & (unsigned long) 0x08000000 ? ((x) | (unsigned long) 0xf0000000) : ((x)))
+#define EXTEND28(x) ((x) & 0x08000000UL ? ((x) | ~0x0fffffffUL) : ((x)))
/* Now find the four bytes of INSN_CH and put them in *INSN. */
@@ -101,7 +101,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
@@ -121,7 +121,7 @@
printf ("\n ret=%x opc_pos=%x, param_pos=%x\n",
ret, opc_pos, param_pos);
#endif
- ret |= 0xffffffff << letter_range(param_ch);
+ ret |= -1L << letter_range(param_ch);
#if DEBUG
printf ("\n after conversion to signed: ret=%x\n", ret);
#endif
Index: orpsocv2/sw/utils/or32-idecode/or32-opc.c
===================================================================
--- orpsocv2/sw/utils/or32-idecode/or32-opc.c (revision 663)
+++ orpsocv2/sw/utils/or32-idecode/or32-opc.c (working copy)
@@ -895,7 +895,7 @@
/* First truncate all bits above valid range for this letter
in case it is zero extend. */
letter_bits = letter_range (l);
- mask = (1 << letter_bits) - 1;
+ mask = (1UL << letter_bits) - 1;
imm &= mask;
/* Do sign extend if this is the right one. */
@@ -947,7 +947,7 @@
{
opc_pos--;
if (param_ch == *enc)
- ret |= 1 << opc_pos;
+ ret |= 1UL << opc_pos;
enc++;
}
else if (*enc == param_ch)
_______________________________________________
OpenRISC mailing list
[email protected]
http://lists.openrisc.net/listinfo/openrisc
