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

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

Hello Michael,

As per our discussion to postpone 'adjust-insn-abs-refs' for later discussion.
Kindly find below updated patch/Change Log

Change Log
==========
2018-02-25 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/reloc.c (2 new BFD relocations):
'BFD_RELOC_MICROBLAZE_64_TEXTPCREL' +
'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'
    * bfd/bfd-in2.h: Regenerate
    * bfd/libbfd.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_elf_relocate_section): Handle new relocs in case of
elf relocation.
    (microblaze_elf_relax_section): Handle new relocs for elf relaxation.
    * 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'
    (tc_gen_reloc): generate relocations for
'BFD_RELOC_MICROBLAZE_64_TEXTPCREL' ,
'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'
    Add new linker option: disable-multiple-abs-defs
    * ld/lexsup.c (Add new ld option):
    (ld_options): add disable-multiple-abs-defs @ 'ld_options' array
    (parse_args): parse new option and pass flag to 'link_info' struct.
    * ld/ldlex.h (Add enum): add new enum @ 'option_values' enum.
    * include/bfdlink.h (Add flag): Add new flag @ 'bfd_link_info' struct.
    * ld/main.c: Initialize flags with false @ 'main'. Handle
disable-multiple-abs-defs @ 'mutiple_definition'.

Patch:
======
Patch attached here and updated as well in
https://github.com/andrewsadek/microblaze-pic-data-text-rel/blob/pic_data_text_rel/PATCH_BUNDLE/binutils.patch



On Fri, Mar 30, 2018 at 4:07 PM, Andrew Sadek <andrew.sadek.se@gmail.com> wrote:
> Hello Michael,
>
> Please find below the patch/ Change Log after fixes.
> Regarding the 'adjust-insn-abs-refs' option, I m just waiting for your reply
> whether we keep it or generate the correct opcode from gcc.
> Thanks.
>
> Change Log:
> ===============
>
> 2018-03-30 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/reloc.c (2 new BFD relocations):
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL' +
> 'BFD_RELOC_MICROBLAZE_64_TEXTPCREL'
>     * bfd/bfd-in2.h: Regenerate
>     * bfd/libbfd.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
>     (microblaze_elf_relocate_section): Handle new relocs in case of
> elf relocation.
>     (microblaze_elf_relax_section): Handle new relocs for elf relaxation.
>     * 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'
>     (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
>     (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'.
>
> Patch:
> ======
> Patch attached here and updated as well in
> https://github.com/andrewsadek/microblaze-pic-data-text-rel/blob/pic_data_text_rel/PATCH_BUNDLE/binutils.patch
>
>
>
>
> On Mon, Mar 26, 2018 at 8:16 AM, Andrew Sadek <andrew.sadek.se@gmail.com> wrote:
>> On Mon, Mar 26, 2018 at 1:48 AM, Michael Eager <eager@eagercon.com> wrote:
>>>
>>>>>>       (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 ?
>>>
>>>
>>> I'll have to give this some thought.
>>>
>>> Do any other targets support a text relative addressing mode?
>>
>> I could find it in ARM but the instructions that retrieve the data
>> support pc-relative addressing also could not find the feature of
>> switching between relative and absolute addressing in PIC.
>> I found this discussion in gnu archive:
>> https://lists.gnu.org/archive/html/help-gplusplus/2012-03/msg00000.html
>>
>>>
>>>> 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.
>>>
>>> Set your email client to send text only.  If you are using gmail, click
>>> the arrow at the bottom right of the compose window and select "Plain
>>> Text Mode".
>>
>> Thanks. It's working now. I m just attaching the same patch.
>>
>>> --
>>> Michael Eager    eager@eagercon.com
>>>
>>> 1960 Park Blvd., Palo Alto, CA 94306
>>
>>
>>
>> --
>>
>> Andrew
>
>
>
> --
>
> Andrew



-- 

Andrew

Attachment: binutils.patch.txt
Description: Text document