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

[Andrew Sadek <andrew dot sadek dot se at gmail dot com>]

Thanks Michael. Please find replies below.

On Sat, Mar 24, 2018 at 7:40 PM, Michael Eager <eager@eagerm.com> wrote:
> 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.
>

To Be Reworked

>>      * 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.
>

In fact, the idea behind 'adjust-insn-abs-refs' option is when making
static linking
to a position independent executable with the base program using -R,
the relocate function
checks whether the symbol is coming from the external file or not then
re-adjust the instruction
to use absolute addressing instead since the reference location in
memory is fixed and shall be
excluded from PIC/PIE.
- For function call: brlid => bralid and accordingly PCREL => Generic Reloc
- For data reference => replace base register  r0 => r20 and TEXTREL => Generic

I understand it's not the best clean way. I agree with you that it
would be better to handle it in GCC.
So, I came up with this solution: Make a variable/function declaration
attribute in microblaze.c called for example
'absolute_reference' which then excludes the symbol reference from PIC
and generate the corresponding
instruction. In addition, replace 'adjust-insn-abs-refs' in linker
with something like 'validate-insn-abs-refs'
that only checks if symbol references coming from the external file
are invoked with absolute addressing
and generate error/warning if not. What do you think ?

>>      (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.

To Be Reworked

>
>>      (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.

Indents to be fixed

>
>>      (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.

To be Done.

>
>>      (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?

To be Fixed

>
> +       && ((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.

To be Fixed

>
>>
>>
>> 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.

I get this error:

*Hi. This is the qmail-send program at sourceware.org.
*I'm afraid I wasn't able to deliver your message to the following addresses.
*This is a permanent error; I've given up. Sorry it didn't work out.

*<gcc-patches@gcc.gnu.org>:
*Invalid mime type "text/html" detected in message text or
*attachment.  Please send plain text messages only.
*See http://sourceware.org/lists.html#sourceware-list-info for more information.

I checked the website, it's stated that only plain text is allowed.


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



-- 

Andrew