Re: ARC port broken reloc processing

[Alan Modra <amodra at gmail dot com>]

On Sat, Nov 28, 2015 at 09:18:26PM +0000, Andrew Burgess wrote:
> * Alan Modra <amodra@gmail.com> [2015-11-27 14:09:38 +1030]:
> 
> > On Fri, Nov 27, 2015 at 01:14:12PM +1030, Alan Modra wrote:
> > > Why allow zero for the base address, and the variant ranges?
> > 
> > I had a look.  If it was for arc-elf, please don't weaken the test.
> > arc-elf reloc processing via howto is broken.
> > 
> > This..
> > 
> > #define ARC_RELOC_HOWTO(TYPE, VALUE, RSIZE, BITSIZE, RELOC_FUNCTION, OVERFLOW, FORMULA) \
> >   [TYPE] = HOWTO (R_##TYPE, 0, RSIZE, BITSIZE, FALSE, 0, complain_overflow_##OVERFLOW, arc_elf_reloc, #TYPE, FALSE, 0, 0, FALSE),
> > 
> > static struct reloc_howto_struct elf_arc_howto_table[] =
> > {
> > #include "elf/arc-reloc.def"
> > }
> > 
> > ..results in all reloc howtos having 0 for dst_mask, ie. don't
> > actually apply the relocation.  Clearly this needs to be fixed by
> > modifying arc-reloc.def to add the appropriate bits to be used in
> > dst_mask.
> 
> Thanks for this pointer.  I've attached a patch below that fills in
> the dst_mask field for some of the relocations, and the hack in my
> original test is no longer required.

Thanks for doing this.  When I posted the above I wasn't asking you to
fix the ARC problem before applying the objdump patch, and the subject
change was to hopefully alert someone who cares about ARC to do the
work..

> What do you think of this patch?  The commit message is a little long,
> but I hope it makes sense, I wanted to capture the thinking behind
> this patch, as filling in the dst_mask is not the only possible
> solution.
> 
> If we look at the xtensa target then we see that they make use of the
> special function hook to patch all the relocations, and this would
> actually seem like a better way to go for arc, given that some of the
> relocations are too complex to be patched using the generic mechanism.

Right.  A wrapper on arc_do_relocation might be possible as the howto
special_function.

> However, when using the generic mechanism, I don't think that it's
> really possible to patch all of the relocations, for example, the
> GOT/PLT relocations would I think be hard to patch from the special
> function.

Yes, that is generally true for all targets.  Linking directly to a
foreign output format, one of the cases where reloc howtos are used,
can only be done for objects using a very limited range of relocs.

> For inspiration, I looked at the arm target, but for that target the
> special function hook is not used (like it is for xtensa) and instead
> arm relies on the generic patching mechanism.  However, I don't think
> that the GOT/PLT relocation patched using the generic mechanism will
> actually be correct, but I suspect this is not actually important, I
> figure the only relocations that really matter are those that end up
> being applied to the dwarf.
> 
> So, in the end, having considered the special function approach, I
> took your advice :) and just filled in the dst_mask for those
> relocations that have a very simple dst_mask value.  For any
> relocations that would require a more complex dst_mask, I just leave
> the value as 0.

By that reasoning you should leave ARC_N8,N16,N24,N32,SECTOFF,SDA32
etc. as zero too.  ie. any reloc with a formula other than S+A will
do the wrong thing.

Here's a prototype patch to wrap arc_do_relocation in the howto
special_function.  I haven't tested it beyond noting that it fixes a
number of testsuite FAILs and causes no testsuite regressions (but I
don't have an arc-elf C compiler installed so not a full test).
Testing I leave to Synopsys..

diff --git a/bfd/elf32-arc.c b/bfd/elf32-arc.c
index 37a426c..7de937a 100644
--- a/bfd/elf32-arc.c
+++ b/bfd/elf32-arc.c
@@ -167,31 +167,8 @@ arc_bfd_put_32 (bfd * abfd, long insn, void *loc, asection * input_section)
   bfd_put_32 (abfd, insn, loc);
 }
 
-static bfd_reloc_status_type
-arc_elf_reloc (bfd *abfd ATTRIBUTE_UNUSED,
-	       arelent *reloc_entry,
-	       asymbol *symbol_in,
-	       void *data ATTRIBUTE_UNUSED,
-	       asection *input_section,
-	       bfd *output_bfd,
-	       char ** error_message ATTRIBUTE_UNUSED)
-{
-  if (output_bfd != NULL)
-    {
-      reloc_entry->address += input_section->output_offset;
-
-      /* In case of relocateable link and if the reloc is against a
-	 section symbol, the addend needs to be adjusted according to
-	 where the section symbol winds up in the output section.  */
-      if ((symbol_in->flags & BSF_SECTION_SYM) && symbol_in->section)
-	reloc_entry->addend += symbol_in->section->output_offset;
-
-      return bfd_reloc_ok;
-    }
-
-  return bfd_reloc_continue;
-}
-
+static bfd_reloc_status_type arc_elf_reloc (bfd *, arelent *, asymbol *,
+					    void *, asection *, bfd *, char **);
 
 #define ARC_RELOC_HOWTO(TYPE, VALUE, SIZE, BITSIZE, RELOC_FUNCTION, OVERFLOW, FORMULA) \
   TYPE = VALUE,
@@ -628,6 +605,68 @@ arc_do_relocation (bfd_byte * contents, struct arc_relocation_data reloc_data)
 
 #undef ARC_RELOC_HOWTO
 
+static bfd_reloc_status_type
+arc_elf_reloc (bfd *abfd ATTRIBUTE_UNUSED,
+	       arelent *reloc_entry,
+	       asymbol *symbol_in,
+	       void *data,
+	       asection *input_section,
+	       bfd *output_bfd,
+	       char ** error_message ATTRIBUTE_UNUSED)
+{
+  struct arc_relocation_data reloc_data;
+
+  if (output_bfd != NULL)
+    {
+      reloc_entry->address += input_section->output_offset;
+
+      /* In case of relocateable link and if the reloc is against a
+	 section symbol, the addend needs to be adjusted according to
+	 where the section symbol winds up in the output section.  */
+      if ((symbol_in->flags & BSF_SECTION_SYM) && symbol_in->section)
+	reloc_entry->addend += symbol_in->section->output_offset;
+
+      return bfd_reloc_ok;
+    }
+
+  reloc_data.reloc_offset = reloc_entry->address;
+  reloc_data.reloc_addend = reloc_entry->addend;
+  reloc_data.got_offset_value = 0;
+  reloc_data.sym_value = symbol_in->value;
+  reloc_data.sym_section = symbol_in->section;
+  reloc_data.howto = reloc_entry->howto;
+  reloc_data.input_section = input_section;
+  reloc_data.sdata_begin_symbol_vma = 0;
+  reloc_data.sdata_begin_symbol_vma_set = FALSE;
+  reloc_data.got_symbol_vma = 0;
+  reloc_data.should_relocate = TRUE;
+  if (input_section->output_section != NULL
+      && input_section->output_section != input_section)
+    {
+      output_bfd = input_section->output_section->owner;
+      if (output_bfd != NULL
+	  && output_bfd->link.hash != NULL)
+	{
+	  struct bfd_link_hash_entry *h;
+
+	  h = bfd_link_hash_lookup (output_bfd->link.hash, "__SDATA_BEGIN__",
+				    FALSE, FALSE, TRUE);
+	  if (h != NULL
+	      && (h->type == bfd_link_hash_defined
+		  || h->type == bfd_link_hash_defweak))
+	    {
+	      reloc_data.sdata_begin_symbol_vma
+		= (h->u.def.value
+		   + h->u.def.section->output_offset
+		   + h->u.def.section->output_section->vma);
+	      reloc_data.sdata_begin_symbol_vma_set = TRUE;
+	    }
+	}
+    }
+
+  return arc_do_relocation (data, reloc_data);
+}
+
 static bfd_vma *
 arc_get_local_got_offsets (bfd * abfd)
 {

-- 
Alan Modra
Australia Development Lab, IBM