Re: [PATCH] [BINUTILS] [MICROBLAZE] PIC Data Text Relative

[Michael Eager <eager at eagerm dot com>]

Hi Andrew --

See comments below.

On 03/22/2018 11:37 AM, Andrew Sadek wrote:
> Dears,
>
> Please find below the patch for microblaze new pic data text relative option.
>
>
> Description:
> This branch is regarding a new implemented feature in GCC Microblaze
> that allows Position Independent Code to run using Data Text Relative
> addressing instead of using Global Offset Table.
>
> Its aim was to make 'PIC' more efficient and flexible as elf size
> excess/ performance overhead were noticed when using GOT due to the
> indirect addressing.
> The change was tested with the dhrystone benchmark on a real Hardware
> (Xilinx FPGA Spartan 6) and the test report went successfully for all
> optimization levels.
>
> Indeed, Microblaze does not support PC-relative addressing in Hardware like ARM.
> The idea was to store the start address of current text section in
> 'r20' instead of GOT, in the function prologue. Correspondingly, data
> references will be an offset from the original reference value to the
> start of text thus being added to the 'r20' base register will resolve
> the actual address.
>
> Henceforth, 2 new relocations have been created:
> - 'R_MICROBLAZE_TEXTPCREL_64': resolves offset of current PC to start
> of text in order to set r20
> - 'R_MICROBLAZE_TEXTREL_64': resolves offset of mentioned data
> reference to start of text
>
> Accordingly, both assembler and linker (binutils) have been adapted.
>
> For extra details:
> https://github.com/andrewsadek/microblaze-pic-data-text-rel/blob/pic_data_text_rel/README.md
>
> Change Log:
> 2018-03-22 Andrew Sadek  <andrew.sadek.se@gmail.com>
>
>      Microblaze Target: PIC data text relative
>      * include/elf/microblaze.h (Add 3 new relocations):
> 'R_MICROBLAZE_TEXTPCREL_64', 'R_MICROBLAZE_TEXTREL_64'
>      and 'R_MICROBLAZE_TEXTREL_32_LO' for relax function.
>      * bfd/bfd-in2.h, bfd/libbfd.h (2 new BFD relocations):
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL' +
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'

As the headers for bfd-in2.h and libbfd.h say, do not edit these files.
The changes should be made to reloc.c and "make headers" should be run
in the build directory to regenerate bfd-in2.h.

List each file separately in the ChangeLog.  The listings for these
files should be something like
  * bfd/bfd-in2.h: Regenerate.

>      * bfd/elf32-microblaze.c (Handle new relocs): define 'HOWTO' of 3
> new relocs and handle them in both relocate
>      and relax functions.
>      (microblaze_elf_reloc_type_lookup): add mapping between for new bfd relocs.


>      (microblaze_bfd_write_branch_absolute_value_64): replace relative
> branch with absolute in case 'adjust_insn_abs_refs' is true
>      (microblaze_bfd_revert_base_reg_value_64): revert base register
> from r20 to r0 in case 'adjust_insn_abs_refs' is true

These two functions are short and only referenced in
microblaze_elf_relocate_section().  It might be clearer to put them
inline in this function.

I'm unclear what is going on in these functions or why.  Why are you
replacing PCREL or TEXTREL reloc with a generic reloc?

I'm not comfortable with editing the instructions to replace opcodes or
registers.  Shouldn't this be handled in GCC by generating the correct
instruction in the first place?  What this means is that the source
assembly generated by GCC will be different from that generated by objdump.

>      (microblaze_elf_relocate_section): Handle new relocs in case of
> elf relocation.

Use a separate case for R_MICROBLAZE_TEXTPCREL_64.  Don't use the case
for R_MICROBLAZE_GOTPC_64 and then have if statements to handle
TEXTPCREL differently.  (There's little similarity between the two
relocations.)

+			if (r_type == R_MICROBLAZE_TEXTREL_32_LO)
+			  bfd_put_16 (input_bfd, relocation & 0xffff,
+					      contents + offset + endian);
+
+			else

Check if/else if.  Use braces to show which if the else applies to.
Remove blank line before else; it makes it look like it is a dangling
else.

>      (microblaze_elf_relax_section): Handle new relocs for elf relaxation.

@@ -1769,7 +1918,11 @@ microblaze_elf_relax_section (bfd *abfd,
 	    - (irel->r_offset
 	       + sec->output_section->vma
 	       + sec->output_offset);
-	}
+    }
+      else if (ELF32_R_TYPE (irel->r_info) == (int) 
R_MICROBLAZE_TEXTREL_64)
+    {
+	  symval = symval + irel->r_addend - (sec->output_section->vma);
+    }

Indent is incorrect.

@@ -1954,7 +2113,10 @@ microblaze_elf_relax_section (bfd *abfd,
 		    }
 		}
 	      else if ((ELF32_R_TYPE (irelscan->r_info) == (int) 
R_MICROBLAZE_32_PCREL_LO)
-		       || (ELF32_R_TYPE (irelscan->r_info) == (int) R_MICROBLAZE_32_LO))
+		       || (ELF32_R_TYPE (irelscan->r_info)
+				   == (int) R_MICROBLAZE_32_LO)
+			   || (ELF32_R_TYPE (irelscan->r_info)

Indent is incorrect.  Line up ||'s.

>      * gas/config/tc-microblaze.c (Handle new relocs directives in
> assembler): Handle new relocs from compiler output.
>      (imm_types): add new imm types for data text relative addressing
> 'TEXT_OFFSET', 'TEXT_PC_OFFSET'
>      (md_convert_frag): conversion for
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL' ,
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'
>      (md_apply_fix): apply fix for 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'
> , 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'
>      (md_estimate_size_before_relax): estimate size for
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL' ,
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'

@@ -2240,13 +2262,15 @@ md_estimate_size_before_relax (fragS * fragP,
       break;

     case INST_NO_OFFSET:
+    case TEXT_OFFSET:
       /* Used to be a reference to somewhere which was unknown.  */
       if (fragP->fr_symbol)
         {
 	  if (fragP->fr_opcode == NULL)
 	    {
               /* Used as an absolute value.  */
-              fragP->fr_subtype = DEFINED_ABS_SEGMENT;
+		      if (fragP->fr_subtype == INST_NO_OFFSET)
+			       fragP->fr_subtype = DEFINED_ABS_SEGMENT;

Fix indent.

>      (tc_gen_reloc): generate relocations for
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL' ,
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'
>      Add new linker options for static linking: adjust-insn-abs-refs,
> disable-multiple-abs-defs
>      * ld/lexsup.c (Add 2 ld options):
>      (ld_options): add adjust-insn-abs-refs, disable-multiple-abs-defs
> @ 'ld_options' array

Update ld/ld.texinfo to describe new options.

>      (parse_args): parse options and pass flags to 'link_info' struct.
>      * ld/ldlex.h (Add 2 enums): add new enums @ 'option_values' enum.
>      * include/bfdlink.h (Add 2 flags): Add new flags @ 'bfd_link_info' struct.
>      * ld/main.c: Initialize flags with false @ 'main'. Handle
> disable-multiple-abs-defs
>      @ 'mutiple_definition'.

@@ -970,12 +972,13 @@ multiple_definition (struct bfd_link_info *info,
      discarded, and this is not really a multiple definition at all.
      FIXME: It would be cleaner to somehow ignore symbols defined in
      sections which are being discarded.  */
-  if ((osec->output_section != NULL
-       && !bfd_is_abs_section (osec)
+  if (!info->prohibt_multiple_definition_absolute

Spelling: prohibit?

+	&& ((osec->output_section != NULL
+       && ! bfd_is_abs_section (osec)
        && bfd_is_abs_section (osec->output_section))

       || (nsec->output_section != NULL
 	  && !bfd_is_abs_section (nsec)
-	  && bfd_is_abs_section (nsec->output_section)))
+	  && bfd_is_abs_section (nsec->output_section))))

Fix indent.

> Patch:
> ----------
> Actually, I found that gmail transforms tabs to spaces even plain text
> mode, also I can not attach the patch as I receive error from
> sourceware server, so for now kindly find the patch in the link
> below,, if you have solution for that please advise.
>
> https://github.com/andrewsadek/microblaze-pic-data-text-rel/blob/pic_data_text_rel/PATCH_BUNDLE/binutils.patch

What kind of error do you get from sourceware?  I haven't
had any problem with attachments.

--
Michael Eager    eager@eagerm.com
1960 Park Blvd., Palo Alto, CA 94306