RE: ARC port broken reloc processing

[Cupertino Miranda <Cupertino dot Miranda at synopsys dot com>]

Hi Alan and Andrew,

Thanks a lot for calling attention to this issues.
We have read through the thread and came out with a patch that fix the dst_mask issue in a way that we think cleaner and more convenient for the future patches coming from us. ;-)

We basically reuse the fix relocation function in order to at runtime compute the right mask. 
I believe in this way we set the mask to a correct value for all the relocation types, without possibly introducing inconsistencies (repetitions) in the reloc definition.
Nevertheless, the generic relocation patching cannot be used to fix relocations, as FORMULA would not be applied.

Hope you agree with the patch and decide to apply this fix instead. :-)

Best regards,
Cupertino Miranda

PS: I include the patch and changelog entry in attach just in case our outlook server destroys it.

bfd/ChangeLog:
2015-11-30  Cupertino Miranda  <cupertino.miranda@synopsys.com>
	* elf32-arc: Initialized dst_masks for ARC relocation types.

diff --git a/bfd/elf32-arc.c b/bfd/elf32-arc.c
index 37a426c..c02db9d 100644
--- a/bfd/elf32-arc.c
+++ b/bfd/elf32-arc.c
@@ -230,7 +230,8 @@ static void arc_elf_howto_init (void)
 {
 #define ARC_RELOC_HOWTO(TYPE, VALUE, SIZE, BITSIZE, RELOC_FUNCTION, OVERFLOW, FORMULA) \
   elf_arc_howto_table[TYPE].pc_relative = \
-    (strstr (#FORMULA, " P ") != NULL || strstr (#FORMULA, " PDATA ") != NULL);
+    (strstr (#FORMULA, " P ") != NULL || strstr (#FORMULA, " PDATA ") != NULL); \
+  elf_arc_howto_table[TYPE].dst_mask = RELOC_FUNCTION(0, ~0);
 
   #include "elf/arc-reloc.def"
 }


-----Original Message-----
From: Alan Modra [mailto:amodra@gmail.com] 
Sent: Monday, November 30, 2015 3:13 AM
To: Andrew Burgess
Cc: binutils@sourceware.org; cupertino.miranda@synopsys.com; Claudiu Zissulescu
Subject: Re: ARC port broken reloc processing

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

Attachment: dst_mask.patch
Description: dst_mask.patch

Attachment: Changelog_entry
Description: Changelog_entry