Re: [PATCH] [Microblaze]: PIC Data Text Relative

Re: [PATCH] [Microblaze]: PIC Data Text Relative

[Michael Eager]

On 02/25/2018 11:44 PM, Andrew Guirguis wrote:
> Dears,
> 
> Kindly find attached the patch bundle for Microblaze 
> '-mpic-data-text-relative' feature.
> 
> Description of the feature in the following link:
> https://github.com/andrewsadek/microblaze-pic-data-text-rel/blob/pic_data_text_rel/README.md 
> <https://github.com/andrewsadek/microblaze-pic-data-text-rel/blob/pic_data_text_rel/README.md>
> 
> Bundle includes:
> 1) Change logs for GCC, binutils
> 2) GCC Test results and comparison with the original.
> 3) New Test case (picdtr.c)
> 4) The Patches (against current heads)

Hi Andrew --

Thanks for the submission.  I have the following recommendations:

Submit each patch to the appropriate project mailing list.  Only submit
the patch for the specific project, without patches for other projects.

Include a description of the changes with each patch as well as the
changelog.  Include the patch in your email or as an attachment.

It isn't clear why you sent your submission to the gdb-patches mailing
list, since there don't appear to be any GDB changes.  Conversely, it is
not clear why you did not include the binutils mailing list, since you
include a patch to that project.

Be sure to follow GNU coding conventions,  Check brace placement,
indent, maximum line length, if statements, etc.  I noticed a number
of places where these conventions are not followed in your patches.

GCC regression tests should include all tests (e.g., gcc.dg), not just 
the limited number of MicroBlaze-specific tests.

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

Re: [PATCH] [Microblaze]: PIC Data Text Relative

[Andrew Sadek]

Thanks Micheal for your response.
I shall re-submit patches separately after re-running the whole GCC Test
suite and re-checking code conventions.
For sending to gdb-patches, it was a conflict from my side as actually I
thought it is also for binutils.

On Tue, Feb 27, 2018 at 2:07 AM, Michael Eager <eager@eagerm.com> wrote:

> On 02/25/2018 11:44 PM, Andrew Guirguis wrote:
>
>> Dears,
>>
>> Kindly find attached the patch bundle for Microblaze
>> '-mpic-data-text-relative' feature.
>>
>> Description of the feature in the following link:
>> https://github.com/andrewsadek/microblaze-pic-data-text-rel/
>> blob/pic_data_text_rel/README.md <https://github.com/andrewsade
>> k/microblaze-pic-data-text-rel/blob/pic_data_text_rel/README.md>
>>
>> Bundle includes:
>> 1) Change logs for GCC, binutils
>> 2) GCC Test results and comparison with the original.
>> 3) New Test case (picdtr.c)
>> 4) The Patches (against current heads)
>>
>
> Hi Andrew --
>
> Thanks for the submission.  I have the following recommendations:
>
> Submit each patch to the appropriate project mailing list.  Only submit
> the patch for the specific project, without patches for other projects.
>
> Include a description of the changes with each patch as well as the
> changelog.  Include the patch in your email or as an attachment.
>
> It isn't clear why you sent your submission to the gdb-patches mailing
> list, since there don't appear to be any GDB changes.  Conversely, it is
> not clear why you did not include the binutils mailing list, since you
> include a patch to that project.
>
> Be sure to follow GNU coding conventions,  Check brace placement,
> indent, maximum line length, if statements, etc.  I noticed a number
> of places where these conventions are not followed in your patches.
>
> GCC regression tests should include all tests (e.g., gcc.dg), not just the
> limited number of MicroBlaze-specific tests.
>
> --
> Michael Eager    eager@eagerm.com
> 1960 Park Blvd., Palo Alto, CA 94306
>



-- 

Andrew

[PATCH] [Microblaze]: PIC Data Text Relative

[Andrew Guirguis]

 Dears,

Kindly find the patch bundle for Microblaze '-mpic-data-text-relative'
feature at the following link:
https://github.com/andrewsadek/microblaze-pic-data-text-rel/tree/pic_data_text_rel/PATCH%20BUNDLE

Description of the feature:
https://github.com/andrewsadek/microblaze-pic-data-text-rel/
blob/pic_data_text_rel/README.md

Bundle includes:
1) Change logs for GCC, binutils
2) GCC Test results and comparison with the original.
3) New Test case (picdtr.c)
4) The Patches (against current heads)

Thanks

-- 

Andrew

Re: [PATCH] [Microblaze]: PIC Data Text Relative

[Andrew Sadek]

Change logs below.
There is a problem in attaching the bundle. I get permanent error from
sourceware.org

GCC ChangeLog:
Microblaze Target: PIC data text relative
* gcc/config/microblaze/microblaze.opt: add new option -mpic-data-text-rel.

* gcc/config/microblaze/microblaze-protos.h (microblaze_constant_address_p
):
Add microblaze_constant_address_p function instead of the macro in
microblaze.h
* gcc/config/microblaze/microblaze.c (TARGET_PIC_DATA_TEXT_REL): New
addressing mode
for data-text relative position indepenedent code.
(microblaze_classify_unspec): add 'UNSPEC_TEXT' case ->
'ADDRESS_SYMBOLIC_TXT_REL'.
(microblaze_classify_address): add handling for UNSPEC + CONST_INT.
(microblaze_legitimate_pic_operand): exclude function calls from pic
operands
in case of TARGET_PIC_DATA_TEXT_REL option.
(microblaze_legitimize_address): generate 'UNSPEC_TEXT' for all possible
addresses cases.
(microblaze_address_insns): add 'ADDRESS_SYMBOLIC_TXT_REL' case.
(print_operand): add 'ADDRESS_SYMBOLIC_TXT_REL' case.
(print_operand_address): add 'ADDRESS_SYMBOLIC_TXT_REL' case + handling for
'address + offset'.
(microblaze_expand_prologue): add new function prologue call for 'r20'
assignation.
(microblaze_asm_generate_pic_addr_dif_vec): override new target hook
'TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC' to disable address diff vector
table in case of TARGET_PIC_DATA_TEXT_REL.
(expand_pic_symbol_ref): add handling for 'UNSPEC_TEXT'.
* gcc/config/microblaze/microblaze.md (TARGET_PIC_DATA_TEXT_REL): Add new
macros 'UNSPEC_TEXT',
'UNSPEC_SET_TEXT' + add rule for setting r20 in function prologue + exclude
function calls
from 'UNSPEC_PLT' in case of data text relative mode.
* gcc/doc/tm.texi.in (TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC): Add new
target hook for generating
address diff vector tables in case of flag_pic.
* gcc/doc/tm.texi : Regenerate.
* gcc/stmt.c (TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC): append new condition
'targetm.asm_out.generate_pic_addr_diff_vec' to flag_pic in case of addr
diff vector generation.
* gcc/target.def (TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC): add target hook
definition.
* gcc/targhooks.h, gcc/targhooks.c (TARGET_ASM_GENERATE_PIC_ADDR_DIFF_VEC):
add default function for generate_pic_addr_diff_vec -> flag_pic.

Binutils ChangeLog:
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'
* 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'.


On Mon, Feb 26, 2018 at 9:56 AM, Andrew Guirguis <andrew.sadek.se@gmail.com>
wrote:

> Dears,
>
> Kindly find the patch bundle for Microblaze '-mpic-data-text-relative'
> feature at the following link:
> https://github.com/andrewsadek/microblaze-pic-data-text-rel/
> tree/pic_data_text_rel/PATCH%20BUNDLE
>
> Description of the feature:
> https://github.com/andrewsadek/microblaze-pic-data-text-rel/
> blob/pic_data_text_rel/README.md
>
> Bundle includes:
> 1) Change logs for GCC, binutils
> 2) GCC Test results and comparison with the original.
> 3) New Test case (picdtr.c)
> 4) The Patches (against current heads)
>
> Thanks
>
> --
>
> Andrew
>



-- 

Andrew