| Message ID | 20231013072856.638728-1-neal.frager@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/2] opcodes: microblaze: Add new bit-field instructions | expand |
Neal --
When I run the test suite I see many segfaults.
Take a look at
gas/config/tc-microblaze.c +418:
md_begin() ...
/* Insert unique names into hash table. */
for (opcode = microblaze_opcodes; opcode->name; opcode ++)
str_hash_insert (opcode_hash_control, opcode->name, opcode, 0);
compare the end of the microblaze_opcodes table:
binutils/opcodes/microblaze-opc.h +427
microblaze_opcodes[] ...
{"", 0, 0, 0, 0, 0, 0, 0, 0},
It looks like there is a mismatch on termination condition.
I changed this to
{NULL, 0, 0, 0, 0, 0, 0, 0, 0},
I'm at a loss to explain why this did not fail earlier.
On 10/13/23 00:28, Neal Frager wrote:
> This patches adds new bsefi and bsifi instructions.
> BSEFI- The instruction shall extract a bit field from a
> register and place it right-adjusted in the destination register.
> The other bits in the destination register shall be set to zero.
> BSIFI- The instruction shall insert a right-adjusted bit field
> from a register at another position in the destination register.
> The rest of the bits in the destination register shall be unchanged.
...
> diff --git a/opcodes/microblaze-dis.c b/opcodes/microblaze-dis.c
> index 12981abfea1..b2d3f19337c 100644
> --- a/opcodes/microblaze-dis.c
> +++ b/opcodes/microblaze-dis.c
> @@ -427,6 +442,14 @@ print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info)
> /* For mbar 16 or sleep insn. */
> case INST_TYPE_NONE:
> break;
> + /* For bit field insns. */
> + case INST_TYPE_RD_R1_IMMW_IMMS:
> + print_func (stream, "\t%s, %s, %s, %s",
> + get_field_rd (&buf, inst),
> + get_field_r1 (&buf, inst),
> + get_field_immw (&buf, inst),
> + get_field_imm5 (&buf, inst));
> + break;
Fixed indent.
Trimmed commit message.
Committed.
Hi Michael, > When I run the test suite I see many segfaults. > Take a look at > gas/config/tc-microblaze.c +418: > md_begin() ... > /* Insert unique names into hash table. */ > for (opcode = microblaze_opcodes; opcode->name; opcode ++) > str_hash_insert (opcode_hash_control, opcode->name, opcode, 0); > compare the end of the microblaze_opcodes table: > binutils/opcodes/microblaze-opc.h +427 > microblaze_opcodes[] ... > {"", 0, 0, 0, 0, 0, 0, 0, 0}, > It looks like there is a mismatch on termination condition. > I changed this to > {NULL, 0, 0, 0, 0, 0, 0, 0, 0}, > I'm at a loss to explain why this did not fail earlier. I believe I know why this did not fail earlier. In the past, the array size was larger than needed with a value of MAX_OPCODES that was larger than the number of elements in the array. As part of this patch, I reduced the MAX_OPCODES to the exact number of elements in the array. -#define MAX_OPCODES 300 +#define MAX_OPCODES 291 So in the past, there were extra NULL elements beyond the end of the array. In any case, thank you for your review, fixing this issue and committing the patch! Best regards, Neal Frager AMD
On Sun, Oct 15, 2023 at 04:48:29PM +0000, Frager, Neal wrote:
> In any case, thank you for your review, fixing this issue and committing the patch!
Commit bb0d05ff74fd caused
FAIL: objdump -S
FAIL: objdump --source-comment
on microblaze-linux-gnu.
On Thu, Oct 19, 2023 at 02:10:41PM +1030, Alan Modra wrote: > On Sun, Oct 15, 2023 at 04:48:29PM +0000, Frager, Neal wrote: > > In any case, thank you for your review, fixing this issue and committing the patch! > > Commit bb0d05ff74fd caused > FAIL: objdump -S > FAIL: objdump --source-comment > on microblaze-linux-gnu. Assembling and attempting to disassemble this .text .long 0x65005f5f should give something to look at. I see $ binutils/objdump -d xxx.o xxx.o: file format elf32-microblaze Disassembly of section .text: 00000000 <.text>: Aborted (core dumped)
Hi Alan, > Le 19 oct. 2023 à 05:53, Alan Modra <amodra@gmail.com> a écrit : > > On Thu, Oct 19, 2023 at 02:10:41PM +1030, Alan Modra wrote: >>> On Sun, Oct 15, 2023 at 04:48:29PM +0000, Frager, Neal wrote: >>> In any case, thank you for your review, fixing this issue and committing the patch! >> >> Commit bb0d05ff74fd caused >> FAIL: objdump -S >> FAIL: objdump --source-comment >> on microblaze-linux-gnu. > > Assembling and attempting to disassemble this > .text > .long 0x65005f5f > should give something to look at. I see > > $ binutils/objdump -d xxx.o > xxx.o: file format elf32-microblaze > > > Disassembly of section .text: > > 00000000 <.text>: > Aborted (core dumped) > Thank you for sharing this. I believe I know the root cause. These new instructions start with the most significant byte 0x64. I think the disassembler code is ignoring bit 24 and treating 0x65 as an instruction with bad parameters. I am confident I can fix it. I just need to get in front my a PC. I will send you a patch that fixes this shortly. Thanks for sharing this! Best regards, Neal Frager AMD > > -- > Alan Modra > Australia Development Lab, IBM
On Thu, Oct 19, 2023 at 06:40:31AM +0000, Frager, Neal wrote: > I am confident I can fix it. I just need to get in front my a PC. > > I will send you a patch that fixes this shortly. Yes, it is an easy and obvious patch. ;-)
Hi Alan, > > In any case, thank you for your review, fixing this issue and committing the patch! > > Commit bb0d05ff74fd caused > FAIL: objdump -S > FAIL: objdump --source-comment > on microblaze-linux-gnu. > Assembling and attempting to disassemble this .text .long 0x65005f5f should give something to look at. I see > $ binutils/objdump -d xxx.o >xxx.o: file format elf32-microblaze > Disassembly of section .text: > 00000000 <.text>: > Aborted (core dumped) Could you please test the following patch? On my side, it resolves the issue you are reporting. https://patchwork.sourceware.org/project/binutils/patch/20231019113740.2071556-1-neal.frager@amd.com/ Thank you for finding and reporting this issue! Best regards, Neal Frager AMD
On Thu, Oct 19, 2023 at 01:20:03PM +0000, Frager, Neal wrote: > Could you please test the following patch? On my side, it resolves the issue you are reporting. > > https://patchwork.sourceware.org/project/binutils/patch/20231019113740.2071556-1-neal.frager@amd.com/ It does. I saw the email yesterday and thought, "good, he's fixed the obvious NUM_STRBUFS bug which was the source of the abort, and also the .short thing".
Hi Alan, > Le 19 oct. 2023 à 23:30, Alan Modra <amodra@gmail.com> a écrit : > > On Thu, Oct 19, 2023 at 01:20:03PM +0000, Frager, Neal wrote: >> Could you please test the following patch? On my side, it resolves the issue you are reporting. >> >> https://patchwork.sourceware.org/project/binutils/patch/20231019113740.2071556-1-neal.frager@amd.com/ > > It does. I saw the email yesterday and thought, "good, he's fixed the > obvious NUM_STRBUFS bug which was the source of the abort, and also > the .short thing". > > -- > Alan Modra > Australia Development Lab, IBM The original patch I submitted was just copy pasting from our meta-xilinx patch set. I am actually quite surprised such a bug exists since AMD has been shipping a microblaze toolchain with this bug in it for a few years now. And now that I am looking more closely at the microblaze-opc.h file, I think there are probably quite a few issues with disassembly and the objdump utility which need fixing. If you catch any other issues, please let me know, as I would like to get this cleaned up. Best regards, Neal Frager AMD
diff --git a/gas/config/tc-microblaze.c b/gas/config/tc-microblaze.c index d900a9e1d05..1703e95c452 100644 --- a/gas/config/tc-microblaze.c +++ b/gas/config/tc-microblaze.c @@ -915,7 +915,7 @@ md_assemble (char * str) unsigned reg2; unsigned reg3; unsigned isize; - unsigned int immed = 0, temp; + unsigned int immed = 0, immed2 = 0, temp; expressionS exp; char name[20]; @@ -1177,6 +1177,87 @@ md_assemble (char * str) inst |= (immed << IMM_LOW) & IMM5_MASK; break; + case INST_TYPE_RD_R1_IMMW_IMMS: + if (strcmp (op_end, "")) + op_end = parse_reg (op_end + 1, ®1); /* Get rd. */ + else + { + as_fatal (_("Error in statement syntax")); + reg1 = 0; + } + + if (strcmp (op_end, "")) + op_end = parse_reg (op_end + 1, ®2); /* Get r1. */ + else + { + as_fatal (_("Error in statement syntax")); + reg2 = 0; + } + + /* Check for spl registers. */ + if (check_spl_reg (®1)) + as_fatal (_("Cannot use special register with this instruction")); + if (check_spl_reg (®2)) + as_fatal (_("Cannot use special register with this instruction")); + + /* Width immediate value. */ + if (strcmp (op_end, "")) + op_end = parse_imm (op_end + 1, &exp, MIN_IMM_WIDTH, MAX_IMM_WIDTH); + else + as_fatal (_("Error in statement syntax")); + + if (exp.X_op != O_constant) + { + as_warn (_( + "Symbol used as immediate width value for bit field instruction")); + immed = 1; + } + else + immed = exp.X_add_number; + + if (opcode->instr == bsefi && immed > 31) + as_fatal (_("Width value must be less than 32")); + + /* Shift immediate value. */ + if (strcmp (op_end, "")) + op_end = parse_imm (op_end + 1, &exp, MIN_IMM, MAX_IMM); + else + as_fatal (_("Error in statement syntax")); + + if (exp.X_op != O_constant) + { + as_warn (_( + "Symbol used as immediate shift value for bit field instruction")); + immed2 = 0; + } + else + { + output = frag_more (isize); + immed2 = exp.X_add_number; + } + + if (immed2 != (immed2 % 32)) + { + as_warn (_("Shift value greater than 32. using <value %% 32>")); + immed2 = immed2 % 32; + } + + /* Check combined value. */ + if (immed + immed2 > 32) + as_fatal (_("Width value + shift value must not be greater than 32")); + + inst |= (reg1 << RD_LOW) & RD_MASK; + inst |= (reg2 << RA_LOW) & RA_MASK; + + if (opcode->instr == bsefi) + inst |= (immed & IMM5_MASK) << IMM_WIDTH_LOW; /* bsefi */ + else + inst |= ((immed + immed2 - 1) & IMM5_MASK) + << IMM_WIDTH_LOW; /* bsifi */ + + inst |= (immed2 << IMM_LOW) & IMM5_MASK; + break; + case INST_TYPE_R1_R2: if (strcmp (op_end, "")) op_end = parse_reg (op_end + 1, ®1); /* Get r1. */ diff --git a/opcodes/microblaze-dis.c b/opcodes/microblaze-dis.c index 12981abfea1..b2d3f19337c 100644 --- a/opcodes/microblaze-dis.c +++ b/opcodes/microblaze-dis.c @@ -90,6 +90,21 @@ get_field_imm5_mbar (struct string_buf *buf, long instr) return p; } +static char * +get_field_immw (struct string_buf *buf, long instr) +{ + char *p = strbuf (buf); + + if (instr & 0x00004000) + sprintf (p, "%d", (short)(((instr & IMM5_WIDTH_MASK) + >> IMM_WIDTH_LOW))); /* bsefi */ + else + sprintf (p, "%d", (short)(((instr & IMM5_WIDTH_MASK) >> + IMM_WIDTH_LOW) - ((instr & IMM5_MASK) >> + IMM_LOW) + 1)); /* bsifi */ + return p; +} + static char * get_field_rfsl (struct string_buf *buf, long instr) { @@ -427,6 +442,14 @@ print_insn_microblaze (bfd_vma memaddr, struct disassemble_info * info) /* For mbar 16 or sleep insn. */ case INST_TYPE_NONE: break; + /* For bit field insns. */ + case INST_TYPE_RD_R1_IMMW_IMMS: + print_func (stream, "\t%s, %s, %s, %s", + get_field_rd (&buf, inst), + get_field_r1 (&buf, inst), + get_field_immw (&buf, inst), + get_field_imm5 (&buf, inst)); + break; /* For tuqula instruction */ case INST_TYPE_RD: print_func (stream, "\t%s", get_field_rd (&buf, inst)); diff --git a/opcodes/microblaze-opc.h b/opcodes/microblaze-opc.h index 7398e9e246a..19bbbdaf68e 100644 --- a/opcodes/microblaze-opc.h +++ b/opcodes/microblaze-opc.h @@ -59,6 +59,9 @@ /* For mbar. */ #define INST_TYPE_IMM5 20 +/* For bsefi and bsifi */ +#define INST_TYPE_RD_R1_IMMW_IMMS 21 + #define INST_TYPE_NONE 25 @@ -90,6 +93,7 @@ #define OPCODE_MASK_H1234 0xFFFFFFFF /* All 32 bits. */ #define OPCODE_MASK_H3 0xFC000600 /* High 6 bits and bits 21, 22. */ #define OPCODE_MASK_H32 0xFC00FC00 /* High 6 bits and bit 16-21. */ +#define OPCODE_MASK_H32B 0xFC00C000 /* High 6 bits and bit 16, 17. */ #define OPCODE_MASK_H34B 0xFC0000FF /* High 6 bits and low 8 bits. */ #define OPCODE_MASK_H35B 0xFC0004FF /* High 6 bits and low 9 bits. */ #define OPCODE_MASK_H34C 0xFC0007E0 /* High 6 bits and bits 21-26. */ @@ -102,7 +106,7 @@ #define DELAY_SLOT 1 #define NO_DELAY_SLOT 0 -#define MAX_OPCODES 300 +#define MAX_OPCODES 291 const struct op_code_struct { @@ -159,6 +163,8 @@ const struct op_code_struct {"bslli", INST_TYPE_RD_R1_IMM5, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64000400, OPCODE_MASK_H3, bslli, barrel_shift_inst }, {"bsrai", INST_TYPE_RD_R1_IMM5, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64000200, OPCODE_MASK_H3, bsrai, barrel_shift_inst }, {"bsrli", INST_TYPE_RD_R1_IMM5, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64000000, OPCODE_MASK_H3, bsrli, barrel_shift_inst }, + {"bsefi", INST_TYPE_RD_R1_IMMW_IMMS, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64004000, OPCODE_MASK_H32B, bsefi, barrel_shift_inst }, + {"bsifi", INST_TYPE_RD_R1_IMMW_IMMS, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x64008000, OPCODE_MASK_H32B, bsifi, barrel_shift_inst }, {"or", INST_TYPE_RD_R1_R2, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x80000000, OPCODE_MASK_H4, microblaze_or, logical_inst }, {"and", INST_TYPE_RD_R1_R2, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x84000000, OPCODE_MASK_H4, microblaze_and, logical_inst }, {"xor", INST_TYPE_RD_R1_R2, INST_NO_OFFSET, NO_DELAY_SLOT, IMMVAL_MASK_NON_SPECIAL, 0x88000000, OPCODE_MASK_H4, microblaze_xor, logical_inst }, @@ -438,5 +444,8 @@ char pvr_register_prefix[] = "rpvr"; #define MIN_IMM5 ((int) 0x00000000) #define MAX_IMM5 ((int) 0x0000001f) +#define MIN_IMM_WIDTH ((int) 0x00000001) +#define MAX_IMM_WIDTH ((int) 0x00000020) + #endif /* MICROBLAZE_OPC */ diff --git a/opcodes/microblaze-opcm.h b/opcodes/microblaze-opcm.h index c91b002d951..3c4f8948c76 100644 --- a/opcodes/microblaze-opcm.h +++ b/opcodes/microblaze-opcm.h @@ -29,7 +29,7 @@ enum microblaze_instr addi, rsubi, addic, rsubic, addik, rsubik, addikc, rsubikc, mul, mulh, mulhu, mulhsu, swapb, swaph, idiv, idivu, bsll, bsra, bsrl, get, put, nget, nput, cget, cput, - ncget, ncput, muli, bslli, bsrai, bsrli, mului, + ncget, ncput, muli, bslli, bsrai, bsrli, bsefi, bsifi, mului, /* 'or/and/xor' are C++ keywords. */ microblaze_or, microblaze_and, microblaze_xor, andn, pcmpbf, pcmpbc, pcmpeq, pcmpne, sra, src, srl, sext8, sext16, @@ -130,6 +130,7 @@ enum microblaze_instr_type #define RB_LOW 11 /* Low bit for RB. */ #define IMM_LOW 0 /* Low bit for immediate. */ #define IMM_MBAR 21 /* low bit for mbar instruction. */ +#define IMM_WIDTH_LOW 6 /* Low bit for immediate width */ #define RD_MASK 0x03E00000 #define RA_MASK 0x001F0000 @@ -142,6 +143,9 @@ enum microblaze_instr_type /* Imm mask for mbar. */ #define IMM5_MBAR_MASK 0x03E00000 +/* Imm mask for extract/insert width. */ +#define IMM5_WIDTH_MASK 0x000007C0 + /* FSL imm mask for get, put instructions. */ #define RFSL_MASK 0x000000F
This patches adds new bsefi and bsifi instructions. BSEFI- The instruction shall extract a bit field from a register and place it right-adjusted in the destination register. The other bits in the destination register shall be set to zero. BSIFI- The instruction shall insert a right-adjusted bit field from a register at another position in the destination register. The rest of the bits in the destination register shall be unchanged. Further documentation of these instructions can be found here: https://docs.xilinx.com/v/u/en-US/ug984-vivado-microblaze-ref With version 6 of the patch, no new relocation types are added as this was unnecessary for adding the bsefi and bsifi instructions. By removing any changes to relocation, there are no longer any regressions to the binutils testsuite. Both before this patch and after lead to the following testsuite results with the following binutils configure command: ./configure --disable-nls --disable-gdb --disable-gdbserver \ --disable-gprofng --disable-libbacktrace \ --disable-libdecnumber --disable-readline \ --disable-sim --enable-obsolete --enable-plugins \ --build=powerpc64le-linux --target=microblaze-xilinx-elf === binutils Summary === # of expected passes 220 # of expected failures 2 # of untested testcases 17 # of unsupported tests 14 === gas Summary === # of expected passes 272 # of unexpected failures 1 # of expected failures 1 # of unsupported tests 8 === ld Summary === # of expected passes 379 # of unexpected failures 4 # of expected failures 13 # of untested testcases 26 # of unsupported tests 217 === libctf Summary === # of expected passes 5 # of unsupported tests 3 === libsframe Summary === # of expected passes 57 Signed-off-by: nagaraju <nagaraju.mekala@amd.com> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com> Signed-off-by: Neal Frager <neal.frager@amd.com> --- V1->V2: -corrected relocation values for the linker V2->V3: - fixed build issue for 32-bit hosts - added test cases for bsefi and bsifi instructions V3->V4: - fixed GNU coding standard issues V4->V5: - fixed a remaining line of code > 80 chars V5->V6: - removed unnecessary relocation changes --- gas/config/tc-microblaze.c | 83 +++++++++++++++++++++++++++++++++++++- opcodes/microblaze-dis.c | 23 +++++++++++ opcodes/microblaze-opc.h | 11 ++++- opcodes/microblaze-opcm.h | 6 ++- 4 files changed, 120 insertions(+), 3 deletions(-)