Hi Andrew, Like you mention in the email, indeed we are a little keen to respect our initial design decisions regarding how the relocations are handled. ;-) After reviewing your patch, IMHO the following things would be nicer. - Your new function should not be allowed to change the relocation value. Moreover, I believe that the masking you do is not necessary as replace_bits16 function, used for the new relocation in arc-reloc.def, already performs the masking on the 16 less significant bits. - I would prefer the checks to be done next to the existing relocation overflow detection, inside the do_relocation function. You can find all this suggestions in the attached patch with the refereed changes. Please be aware the changes in the patch might not generate the exact same stdout/stderr messages, so the tests must be changed. Best regards, Cupertino On 04/07/2016 01:05 PM, Andrew Burgess wrote: > This patch adds some new arc/nps instructions. These new instructions > require a new relocation, and this new relocation requires a specific > check of the computed relocation value. > > To achieve this specific check (that the most significant 2 bytes are > a particular value), I added a new function, arc_final_relocation_checks. > > I know that the Synopsys team have a particular design in mind for how > they want ARC relocations to be handled, and this new function might > not fit within their desired design. > > I'm not particularly attached to the new function, I'm open to any > alternative suggestions for how to achieve the same end result if the > Synopsys team would like to make a suggestion. In the absence of any > alternative suggestions though, I believe the code I offer below > should be acceptable for inclusion. > > Thanks, > Andrew > > > --- > > Add support for arc/nps400 cmem instructions, these load and store > instructions are hard-wired to access "0x57f00000 + 16-bit-offset". > > Supporting this relocation required some additions to the arc relocation > handling in the bfd library, as well as the standard changes required to > add a new relocation type. > > There's a test of the new instructions in the assembler, and a test of > the relocation in the linker. > > bfd/ChangeLog: > > * reloc.c: Add BFD_RELOC_ARC_NPS_CMEM16 entry. > * bfd-in2.h: Regenerate. > * libbfd.h: Regenerate. > * elf32-arc.c: Add 'opcode/arc.h' include. > (struct arc_relocation_data): Add symbol_name. > (arc_final_relocation_checks): New function. > (arc_do_relocation): Update ARC_RELOC_HOWTO definition to call > arc_final_relocation_checks. > (elf_arc_relocate_section): Setup symbol_name in reloc_data. > > gas/ChangeLog: > > * testsuite/gas/arc/nps400-3.d: New file. > * testsuite/gas/arc/nps400-3.s: New file. > > include/ChangeLog: > > * elf/arc-reloc.def: Add ARC_NPS_CMEM16 reloc. > * opcode/arc.h (NPS_CMEM_HIGH_VALUE): Define. > > ld/ChangeLog: > > * testsuite/ld-arc/arc.exp: New file. > * testsuite/ld-arc/nps-1.s: New file. > * testsuite/ld-arc/nps-1a.d: New file. > * testsuite/ld-arc/nps-1b.d: New file. > * testsuite/ld-arc/nps-1b.err: New file. > > opcodes/ChangeLog: > > * arc-nps400-tbl.h: Add xldb, xldw, xld, xstb, xstw, and xst > instructions. > * arc-opc.c (insert_nps_cmem_uimm16): New function. > (extract_nps_cmem_uimm16): New function. > (arc_operands): Add NPS_XLDST_UIMM16 operand. > --- > bfd/ChangeLog | 12 +++++++++ > bfd/bfd-in2.h | 1 + > bfd/elf32-arc.c | 58 ++++++++++++++++++++++++++++++++++++++++ > bfd/libbfd.h | 1 + > bfd/reloc.c | 2 ++ > gas/ChangeLog | 5 ++++ > gas/testsuite/gas/arc/nps400-3.d | 56 ++++++++++++++++++++++++++++++++++++++ > gas/testsuite/gas/arc/nps400-3.s | 23 ++++++++++++++++ > include/ChangeLog | 5 ++++ > include/elf/arc-reloc.def | 7 +++++ > include/opcode/arc.h | 3 +++ > ld/ChangeLog | 8 ++++++ > ld/testsuite/ld-arc/arc.exp | 30 +++++++++++++++++++++ > ld/testsuite/ld-arc/nps-1.s | 10 +++++++ > ld/testsuite/ld-arc/nps-1a.d | 16 +++++++++++ > ld/testsuite/ld-arc/nps-1b.d | 4 +++ > ld/testsuite/ld-arc/nps-1b.err | 1 + > opcodes/ChangeLog | 8 ++++++ > opcodes/arc-nps400-tbl.h | 12 +++++++++ > opcodes/arc-opc.c | 22 +++++++++++++++ > 20 files changed, 284 insertions(+) > create mode 100644 gas/testsuite/gas/arc/nps400-3.d > create mode 100644 gas/testsuite/gas/arc/nps400-3.s > create mode 100644 ld/testsuite/ld-arc/arc.exp > create mode 100644 ld/testsuite/ld-arc/nps-1.s > create mode 100644 ld/testsuite/ld-arc/nps-1a.d > create mode 100644 ld/testsuite/ld-arc/nps-1b.d > create mode 100644 ld/testsuite/ld-arc/nps-1b.err > > diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h > index f02e2aa..6532f6e 100644 > --- a/bfd/bfd-in2.h > +++ b/bfd/bfd-in2.h > @@ -3721,6 +3721,7 @@ pc-relative or some form of GOT-indirect relocation. */ > BFD_RELOC_ARC_TLS_LE_32, > BFD_RELOC_ARC_S25W_PCREL_PLT, > BFD_RELOC_ARC_S21H_PCREL_PLT, > + BFD_RELOC_ARC_NPS_CMEM16, > > /* ADI Blackfin 16 bit immediate absolute reloc. */ > BFD_RELOC_BFIN_16_IMM, > diff --git a/bfd/elf32-arc.c b/bfd/elf32-arc.c > index 263effc..52bf5dc 100644 > --- a/bfd/elf32-arc.c > +++ b/bfd/elf32-arc.c > @@ -26,6 +26,7 @@ > #include "elf/arc.h" > #include "libiberty.h" > #include "opcode/arc-func.h" > +#include "opcode/arc.h" > #include "arc-plt.h" > > #ifdef DEBUG > @@ -722,6 +723,8 @@ struct arc_relocation_data > bfd_signed_vma got_symbol_vma; > > bfd_boolean should_relocate; > + > + const char * symbol_name; > }; > > static void > @@ -873,11 +876,61 @@ middle_endian_convert (bfd_vma insn, bfd_boolean do_it) > ARC_DEBUG ("after = 0x%08x\n", (unsigned int) insn); \ > } > > +/* Perform final checks, and possible adjustment to the relocation value > + before applying the relocation to the instruction. */ > + > +static inline bfd_reloc_status_type > +arc_final_relocation_checks (bfd_signed_vma *relocation, > + const struct arc_relocation_data reloc_data, > + struct bfd_link_info *info ATTRIBUTE_UNUSED) > +{ > + switch (reloc_data.howto->type) > + { > + case R_ARC_NPS_CMEM16: > + if (((*relocation >> 16) & 0xffff) != NPS_CMEM_HIGH_VALUE) > + { > + if (reloc_data.reloc_addend == 0) > + (*_bfd_error_handler) > + (_("%B(%A+0x%lx): CMEM relocation to `%s' is invalid, " > + "16 MSB should be 0x%04x (value is 0x%lx)"), > + reloc_data.input_section->owner, > + reloc_data.input_section, > + reloc_data.reloc_offset, > + reloc_data.symbol_name, > + NPS_CMEM_HIGH_VALUE, > + (*relocation)); > + else > + (*_bfd_error_handler) > + (_("%B(%A+0x%lx): CMEM relocation to `%s+0x%lx' is invalid, " > + "16 MSB should be 0x%04x (value is 0x%lx)"), > + reloc_data.input_section->owner, > + reloc_data.input_section, > + reloc_data.reloc_offset, > + reloc_data.symbol_name, > + reloc_data.reloc_addend, > + NPS_CMEM_HIGH_VALUE, > + (*relocation)); > + return bfd_reloc_overflow; > + } > + *relocation &= 0xffff; > + break; > + > + default: > + break; > + } > + > + return bfd_reloc_ok; > +} > + > #define ARC_RELOC_HOWTO(TYPE, VALUE, SIZE, BITSIZE, RELOC_FUNCTION, OVERFLOW, FORMULA) \ > case R_##TYPE: \ > { \ > bfd_signed_vma bitsize ATTRIBUTE_UNUSED = BITSIZE; \ > + bfd_reloc_status_type status; \ > relocation = FORMULA ; \ > + status = arc_final_relocation_checks (&relocation, reloc_data, info); \ > + if (status != bfd_reloc_ok) \ > + return status; \ > PRINT_DEBUG_RELOC_INFO_BEFORE (#FORMULA, #TYPE); \ > insn = middle_endian_convert (insn, IS_ME (#FORMULA, abfd)); \ > insn = (* get_replace_function (abfd, TYPE)) (insn, relocation); \ > @@ -1168,6 +1221,10 @@ elf_arc_relocate_section (bfd * output_bfd, > > reloc_data.sym_value = sym->st_value; > reloc_data.sym_section = sec; > + reloc_data.symbol_name = > + bfd_elf_string_from_elf_section (input_bfd, > + symtab_hdr->sh_link, > + sym->st_name); > > /* Mergeable section handling. */ > if ((sec->flags & SEC_MERGE) > @@ -1284,6 +1341,7 @@ elf_arc_relocate_section (bfd * output_bfd, > h = (struct elf_link_hash_entry *) h->root.u.i.link; > > BFD_ASSERT ((h->dynindx == -1) >= (h->forced_local != 0)); > + reloc_data.symbol_name = h->root.root.string; > /* If we have encountered a definition for this symbol. */ > if (h->root.type == bfd_link_hash_defined > || h->root.type == bfd_link_hash_defweak) > diff --git a/bfd/libbfd.h b/bfd/libbfd.h > index d7183d3..16c0aee 100644 > --- a/bfd/libbfd.h > +++ b/bfd/libbfd.h > @@ -1738,6 +1738,7 @@ static const char *const bfd_reloc_code_real_names[] = { "@@uninitialized@@", > "BFD_RELOC_ARC_TLS_LE_32", > "BFD_RELOC_ARC_S25W_PCREL_PLT", > "BFD_RELOC_ARC_S21H_PCREL_PLT", > + "BFD_RELOC_ARC_NPS_CMEM16", > "BFD_RELOC_BFIN_16_IMM", > "BFD_RELOC_BFIN_16_HIGH", > "BFD_RELOC_BFIN_4_PCREL", > diff --git a/bfd/reloc.c b/bfd/reloc.c > index 0135c04..c3b713b 100644 > --- a/bfd/reloc.c > +++ b/bfd/reloc.c > @@ -3668,6 +3668,8 @@ ENUMX > BFD_RELOC_ARC_S25W_PCREL_PLT > ENUMX > BFD_RELOC_ARC_S21H_PCREL_PLT > +ENUMX > + BFD_RELOC_ARC_NPS_CMEM16 > ENUMDOC > ARC relocs. > > diff --git a/gas/testsuite/gas/arc/nps400-3.d b/gas/testsuite/gas/arc/nps400-3.d > new file mode 100644 > index 0000000..ea52554 > --- /dev/null > +++ b/gas/testsuite/gas/arc/nps400-3.d > @@ -0,0 +1,56 @@ > +#as: -mcpu=nps400 > +#objdump: -dr > + > +.*: +file format .*arc.* > + > +Disassembly of section .text: > + > +[0-9a-f]+ <.*>: > + 0: 5988 0000 xldb r12,\[0x57f00000\] > + 4: 5ae8 ffff xldb r23,\[0x57f0ffff\] > + 8: 5868 0000 xldb r3,\[0x57f00000\] > + c: 5968 ffff xldb r11,\[0x57f0ffff\] > + 10: 5a88 0000 xldb r20,\[0x57f00000\] > + 10: R_ARC_NPS_CMEM16 foo > + 14: 5828 0000 xldb r1,\[0x57f00000\] > + 14: R_ARC_NPS_CMEM16 foo\+0x20 > + 18: 5989 0000 xldw r12,\[0x57f00000\] > + 1c: 5ae9 ffff xldw r23,\[0x57f0ffff\] > + 20: 5869 0000 xldw r3,\[0x57f00000\] > + 24: 5969 ffff xldw r11,\[0x57f0ffff\] > + 28: 5a89 0000 xldw r20,\[0x57f00000\] > + 28: R_ARC_NPS_CMEM16 foo > + 2c: 5829 0000 xldw r1,\[0x57f00000\] > + 2c: R_ARC_NPS_CMEM16 foo\+0x20 > + 30: 598a 0000 xld r12,\[0x57f00000\] > + 34: 5aea ffff xld r23,\[0x57f0ffff\] > + 38: 586a 0000 xld r3,\[0x57f00000\] > + 3c: 596a ffff xld r11,\[0x57f0ffff\] > + 40: 5a8a 0000 xld r20,\[0x57f00000\] > + 40: R_ARC_NPS_CMEM16 foo > + 44: 582a 0000 xld r1,\[0x57f00000\] > + 44: R_ARC_NPS_CMEM16 foo\+0x20 > + 48: 598c 0000 xstb r12,\[0x57f00000\] > + 4c: 5aec ffff xstb r23,\[0x57f0ffff\] > + 50: 586c 0000 xstb r3,\[0x57f00000\] > + 54: 596c ffff xstb r11,\[0x57f0ffff\] > + 58: 5a8c 0000 xstb r20,\[0x57f00000\] > + 58: R_ARC_NPS_CMEM16 foo > + 5c: 582c 0000 xstb r1,\[0x57f00000\] > + 5c: R_ARC_NPS_CMEM16 foo\+0x20 > + 60: 598d 0000 xstw r12,\[0x57f00000\] > + 64: 5aed ffff xstw r23,\[0x57f0ffff\] > + 68: 586d 0000 xstw r3,\[0x57f00000\] > + 6c: 596d ffff xstw r11,\[0x57f0ffff\] > + 70: 5a8d 0000 xstw r20,\[0x57f00000\] > + 70: R_ARC_NPS_CMEM16 foo > + 74: 582d 0000 xstw r1,\[0x57f00000\] > + 74: R_ARC_NPS_CMEM16 foo\+0x20 > + 78: 598e 0000 xst r12,\[0x57f00000\] > + 7c: 5aee ffff xst r23,\[0x57f0ffff\] > + 80: 586e 0000 xst r3,\[0x57f00000\] > + 84: 596e ffff xst r11,\[0x57f0ffff\] > + 88: 5a8e 0000 xst r20,\[0x57f00000\] > + 88: R_ARC_NPS_CMEM16 foo > + 8c: 582e 0000 xst r1,\[0x57f00000\] > + 8c: R_ARC_NPS_CMEM16 foo\+0x20 > diff --git a/gas/testsuite/gas/arc/nps400-3.s b/gas/testsuite/gas/arc/nps400-3.s > new file mode 100644 > index 0000000..6840223 > --- /dev/null > +++ b/gas/testsuite/gas/arc/nps400-3.s > @@ -0,0 +1,23 @@ > + .macro xldst_test mnem > + \mnem r12, [ 0x0 ] > + \mnem r23, [ 0xffff ] > + \mnem r3, [ 0x57f00000 ] > + \mnem r11, [ 0x57f0ffff ] > + \mnem r20, [ foo ] > + \mnem r1, [ foo + 0x20 ] > + .endm > + > + .text > + ;; xldb > + xldst_test xldb > + ;; xldw > + xldst_test xldw > + ;; xld > + xldst_test xld > + ;; xstb > + xldst_test xstb > + ;; xstw > + xldst_test xstw > + ;; xst > + xldst_test xst > + > diff --git a/include/elf/arc-reloc.def b/include/elf/arc-reloc.def > index 36a3516..b271e0e 100644 > --- a/include/elf/arc-reloc.def > +++ b/include/elf/arc-reloc.def > @@ -490,3 +490,10 @@ ARC_RELOC_HOWTO(ARC_S21H_PCREL_PLT, 77, \ > replace_disp21h, \ > signed, \ > ( ME ( ( ( ( L + A ) - P ) >> 1 ) ) )) > + > +ARC_RELOC_HOWTO(ARC_NPS_CMEM16, 78, \ > + 2, \ > + 16, \ > + replace_bits16, \ > + bitfield, \ > + ( S + A )) > diff --git a/include/opcode/arc.h b/include/opcode/arc.h > index bc0e1ad..2827c96 100644 > --- a/include/opcode/arc.h > +++ b/include/opcode/arc.h > @@ -423,6 +423,9 @@ extern const unsigned arc_num_aux_regs; > extern const struct arc_opcode arc_relax_opcodes[]; > extern const unsigned arc_num_relax_opcodes; > > +/* Macro used for generating one class of NPS instructions. */ > +#define NPS_CMEM_HIGH_VALUE 0x57f0 > + > /* Macros to help generating regular pattern instructions. */ > #define FIELDA(word) (word & 0x3F) > #define FIELDB(word) (((word & 0x07) << 24) | (((word >> 3) & 0x07) << 12)) > diff --git a/ld/testsuite/ld-arc/arc.exp b/ld/testsuite/ld-arc/arc.exp > new file mode 100644 > index 0000000..0cf6228 > --- /dev/null > +++ b/ld/testsuite/ld-arc/arc.exp > @@ -0,0 +1,30 @@ > +# Copyright (C) 2016 Free Software Foundation, Inc. > +# > +# This file is part of the GNU Binutils. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, > +# MA 02110-1301, USA. > +# > + > +if { ![istarget arc*-*-*] } { > + return > +} > + > +set arc_test_list [lsort [glob -nocomplain $srcdir/$subdir/*.d]] > +foreach arc_test $arc_test_list { > + verbose [file rootname $arc_test] > + run_dump_test [file rootname $arc_test] > +} > + > diff --git a/ld/testsuite/ld-arc/nps-1.s b/ld/testsuite/ld-arc/nps-1.s > new file mode 100644 > index 0000000..295fa2c > --- /dev/null > +++ b/ld/testsuite/ld-arc/nps-1.s > @@ -0,0 +1,10 @@ > + .text > + .global __start > +__start: > + xldb r10, [ foo ] > + xldw r10, [ foo ] > + xld r10, [ foo ] > + xstb r10, [ foo ] > + xstw r10, [ foo ] > + xst r10, [ foo ] > + > diff --git a/ld/testsuite/ld-arc/nps-1a.d b/ld/testsuite/ld-arc/nps-1a.d > new file mode 100644 > index 0000000..932842b > --- /dev/null > +++ b/ld/testsuite/ld-arc/nps-1a.d > @@ -0,0 +1,16 @@ > +#source: nps-1.s > +#as: -mcpu=nps400 > +#ld: -defsym=foo=0x57f03000 > +#objdump: -d > + > +.*: +file format .*arc.* > + > +Disassembly of section .text: > + > +[0-9a-f]+ <.*>: > + 10074: 5948 3000 xldb r10,\[0x57f03000\] > + 10078: 5949 3000 xldw r10,\[0x57f03000\] > + 1007c: 594a 3000 xld r10,\[0x57f03000\] > + 10080: 594c 3000 xstb r10,\[0x57f03000\] > + 10084: 594d 3000 xstw r10,\[0x57f03000\] > + 10088: 594e 3000 xst r10,\[0x57f03000\] > diff --git a/ld/testsuite/ld-arc/nps-1b.d b/ld/testsuite/ld-arc/nps-1b.d > new file mode 100644 > index 0000000..56c29ae > --- /dev/null > +++ b/ld/testsuite/ld-arc/nps-1b.d > @@ -0,0 +1,4 @@ > +#source: nps-1.s > +#as: -mcpu=nps400 > +#ld: -defsym=foo=0x56f03000 > +#error_output: nps-1b.err > diff --git a/ld/testsuite/ld-arc/nps-1b.err b/ld/testsuite/ld-arc/nps-1b.err > new file mode 100644 > index 0000000..a44b3c1 > --- /dev/null > +++ b/ld/testsuite/ld-arc/nps-1b.err > @@ -0,0 +1 @@ > +.*\.o\(\.text\+0x0\): CMEM relocation to `foo' is invalid, 16 MSB should be 0x57f0 \(value is 0x56f03000\) > diff --git a/opcodes/arc-nps400-tbl.h b/opcodes/arc-nps400-tbl.h > index dc7b066..f925ca3 100644 > --- a/opcodes/arc-nps400-tbl.h > +++ b/opcodes/arc-nps400-tbl.h > @@ -123,3 +123,15 @@ > > /* crc32<.r> 0,limm,u6 00111 110 01 110100 R 111 uuuuuu 111110 */ > { "crc32", 0x3e74703e, 0xffff703f, ARC_OPCODE_NPS400, BITOP, NONE, { ZA, LIMM, UIMM6_20 }, { C_NPS_R }}, > + > +/**** Load / Store From (0x57f00000 + Offset) Instructions ****/ > + > +#define XLDST_LIKE(NAME,SUBOP2) \ > + { NAME, (0x58000000 | (SUBOP2 << 16)), 0xf81f0000, ARC_OPCODE_NPS400, MEMORY, NONE, { NPS_R_DST, BRAKET, NPS_XLDST_UIMM16, BRAKETdup }, { 0 }}, > + > +XLDST_LIKE("xldb", 0x8) > +XLDST_LIKE("xldw", 0x9) > +XLDST_LIKE("xld", 0xa) > +XLDST_LIKE("xstb", 0xc) > +XLDST_LIKE("xstw", 0xd) > +XLDST_LIKE("xst", 0xe) > diff --git a/opcodes/arc-opc.c b/opcodes/arc-opc.c > index f182318..8173f8a 100644 > --- a/opcodes/arc-opc.c > +++ b/opcodes/arc-opc.c > @@ -838,6 +838,25 @@ extract_nps_dst_pos_and_size (unsigned insn ATTRIBUTE_UNUSED, > return (insn & 0x1f); > } > > +static unsigned > +insert_nps_cmem_uimm16 (unsigned insn ATTRIBUTE_UNUSED, > + int value ATTRIBUTE_UNUSED, > + const char **errmsg ATTRIBUTE_UNUSED) > +{ > + int top = (value >> 16) & 0xffff; > + if (top != 0x0 && top != NPS_CMEM_HIGH_VALUE) > + *errmsg = _("invalid value for CMEM ld/st immediate"); > + insn |= (value & 0xffff); > + return insn; > +} > + > +static int > +extract_nps_cmem_uimm16 (unsigned insn ATTRIBUTE_UNUSED, > + bfd_boolean * invalid ATTRIBUTE_UNUSED) > +{ > + return (NPS_CMEM_HIGH_VALUE << 16) | (insn & 0xffff); > +} > + > /* Include the generic extract/insert functions. Order is important > as some of the functions present in the .h may be disabled via > defines. */ > @@ -1442,6 +1461,9 @@ const struct arc_operand arc_operands[] = > > #define NPS_RFLT_UIMM6 (NPS_UIMM16 + 1) > { 6, 6, 0, ARC_OPERAND_UNSIGNED | ARC_OPERAND_NCHK, insert_nps_rflt_uimm6, extract_nps_rflt_uimm6 }, > + > +#define NPS_XLDST_UIMM16 (NPS_RFLT_UIMM6 + 1) > + { 16, 0, BFD_RELOC_ARC_NPS_CMEM16, ARC_OPERAND_UNSIGNED | ARC_OPERAND_NCHK, insert_nps_cmem_uimm16, extract_nps_cmem_uimm16 }, > }; > > const unsigned arc_num_operands = ARRAY_SIZE (arc_operands);