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

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

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

Attachment: binutils.patch.txt
Description: Text document