Re: Re: [PATCH] [RISC-V] Fix the valid gp range for gp relax.

["Die Li" <lidie@eswincomputing.com>]

Hi,
On 2023-07-05 20:06,  Nelson Chu wrote:
>
>Sorry, I think this patch is not correct.
>
>On Wed, Jul 5, 2023 at 4:42 PM Die Li <lidie@eswincomputing.com> wrote:
>
>> Taking alignment into consideration, the conservative addressing range
>> for gp relax is [gp - 12bit_imm + max_alignment, gp + 12bit_imm -
>> max_alignment],
>>
>
>The max_alignment is used to reserve the space in case the range is
>extended because of the alignment.  Therefore, the reserved max_alignment
>should make the value between gp and target symbol be larger or smaller, if
>it is positive or negative. 
This means that the aligned symbols need to be within the range of relative 
addressing in the GP. In code, it can be expressed as:
@@ -4630,10 +4630,8 @@ _bfd_riscv_relax_lui (bfd *abfd,
      Should we also consider the alignment issue for x0 base?  */
   if (undefined_weak
       || VALID_ITYPE_IMM (symval)
-      || (symval >= gp
-	  && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
-      || (symval < gp
-	  && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
+          || (VALID_ITYPE_IMM (ROUNDUP(symval, max_alignment) - gp)
+              && VALID_ITYPE_IMM (ROUNDUP(symval + reserve_size, max_alignment) - gp))))
Besides, we define ROUNDUP as:
/* ROUNDUP -- round an integer up to a multiple of n		    */
#define ROUNDUP(i, n) ((((i)+(n)-1)/n)*n)

Is the expression above consistent with what you said?

Finally, I don't quite understand the meaning of "symval - reserve_size" in the current code. 
Generally, reserve_size maps to the actual data type in the source code and represents the 
space occupied by the data. For example, the reserve_size of an int is 4, and the reserve_size 
of "int arr[4]" is 4*4, and so on. "symval + reserve_size" represents the end address of the symbol. 
That's why I replaced the condition "VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)" 
with "VALID_ITYPE_IMM (ROUNDUP(symval + reserve_size, max_alignment) - gp)".

>
>Besides, the offset must be based on gp, so no matter whether gp is larger
>or smaller than the target symbol, we should always use "symval - gp" as
>the offset.
>
>Thanks
>Nelson
>
>
>> which is smaller than the ideal GP addressing range [gp - 12bit_imm, gp +
>> 12bit_imm]. Therefore, when symval < gp, it is necessary to ensure that
>> both
>> symval and symval + reserve_size fall within the range [gp - 12bit_imm +
>> alignment,
>> gp + 12bit_imm - alignment]. Current linker takes the range of gp
>> conservatively,
>> this patch allows more symbols to be accessed by gp relative addressing.
>
>Take the file named with gp-relax.s from this patch as the source file.
>>
>> After assembling and linking with --relax option,
>> Before this patch:
>> 0+[0-9a-f]+ <_start>:
>> .*:[    ]+[0-9a-f]+[    ]+lui+.*
>> .*:[    ]+[0-9a-f]+[    ]+addi+.*
>> .*:[    ]+[0-9a-f]+[    ]+lui+.*
>> .*:[    ]+[0-9a-f]+[    ]+addi+.*
>> .*:[    ]+[0-9a-f]+[    ]+auipc+.*
>> .*:[    ]+[0-9a-f]+[    ]+lw+.*
>>
>> Check the symbal table after linking, we have :
>> Symbol table '.symtab' contains 19 entries:
>>    Num:    Value          Size Type    Bind   Vis      Ndx Name
>> ...
>>      7: 0000000000011900     0 NOTYPE  GLOBAL DEFAULT  ABS
>> __global_pointer$
>> ...
>>     18: 0000000000011108  3600 OBJECT  GLOBAL DEFAULT    3 global_array
>>
>> By comparing the values of `global_array` and `__global_pointer$`, the
>> symbol of
>> global_array can be accessed by gp relative addressing.
>>
>> After this patch:
>> 0+[0-9a-f]+ <_start>:
>> .*:[    ]+[0-9a-f]+[    ]+addi[         ]+a3,gp,\-[0-9]+ # [0-9a-f]+
>> <global_array>
>> .*:[    ]+[0-9a-f]+[    ]+addi[         ]+a4,gp,\-[0-9]+ # [0-9a-f]+
>> <global_array+.*
>> .*:[    ]+[0-9a-f]+[    ]+lw[   ]+a0,\-[0-9]+\(gp\) # [0-9a-f]+
>> <global_array>
>>
>> Co-Authored by: Fei Gao <gaofei@eswincomputing.com>
>> Signed-off-by: Die Li <lidie@eswincomputing.com>
>>
>> ChangeLog:
>>
>>         * bfd/elfnn-riscv.c (_bfd_riscv_relax_lui): Fix the valid gp range.
>>         (_bfd_riscv_relax_pc): Likewise.
>>         * ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp: New test.
>>         * ld/testsuite/ld-riscv-elf/gp-relax.d: New test.
>>         * ld/testsuite/ld-riscv-elf/gp-relax.s: New test.
>> ---
>>  bfd/elfnn-riscv.c                          |  6 +++--
>>  ld/testsuite/ld-riscv-elf/gp-relax.d       | 14 ++++++++++
>>  ld/testsuite/ld-riscv-elf/gp-relax.s       | 30 ++++++++++++++++++++++
>>  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp |  1 +
>>  4 files changed, 49 insertions(+), 2 deletions(-)
>>  create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax.d
>>  create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax.s
>>
>> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>> index 09aa7be225e..00e692c6924 100644
>> --- a/bfd/elfnn-riscv.c
>> +++ b/bfd/elfnn-riscv.c
>> @@ -4633,7 +4633,8 @@ _bfd_riscv_relax_lui (bfd *abfd,
>>        || (symval >= gp
>>           && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
>>        || (symval < gp
>> -         && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
>> +          && VALID_ITYPE_IMM (symval - gp - max_alignment)
>> +          && VALID_ITYPE_IMM (symval - gp + max_alignment +
>> reserve_size)))
>>      {
>>        unsigned sym = ELFNN_R_SYM (rel->r_info);
>>        switch (ELFNN_R_TYPE (rel->r_info))
>> @@ -4897,7 +4898,8 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
>>        || (symval >= gp
>>           && VALID_ITYPE_IMM (symval - gp + max_alignment + reserve_size))
>>        || (symval < gp
>> -         && VALID_ITYPE_IMM (symval - gp - max_alignment - reserve_size)))
>> +          && VALID_ITYPE_IMM (symval - gp - max_alignment)
>> +          && VALID_ITYPE_IMM (symval - gp + max_alignment +
>> reserve_size)))
>>      {
>>        unsigned sym = hi_reloc.hi_sym;
>>        switch (ELFNN_R_TYPE (rel->r_info))
>> diff --git a/ld/testsuite/ld-riscv-elf/gp-relax.d
>> b/ld/testsuite/ld-riscv-elf/gp-relax.d
>> new file mode 100644
>> index 00000000000..fa93c05ee2c
>> --- /dev/null
>> +++ b/ld/testsuite/ld-riscv-elf/gp-relax.d
>> @@ -0,0 +1,14 @@
>> +#source: gp-relax.s
>> +#as:
>> +#ld: --relax
>> +#objdump: -d -Mno-aliases
>> +
>> +.*:[   ]+file format .*
>> +
>> +
>> +Disassembly of section \.text:
>> +
>> +0+[0-9a-f]+ <_start>:
>> +.*:[   ]+[0-9a-f]+[    ]+addi[         ]+a3,gp,\-[0-9]+ # [0-9a-f]+
>> <global_array>
>> +.*:[   ]+[0-9a-f]+[    ]+addi[         ]+a4,gp,\-[0-9]+ # [0-9a-f]+
>> <global_array+.*
>> +.*:[   ]+[0-9a-f]+[    ]+lw[   ]+a0,\-[0-9]+\(gp\) # [0-9a-f]+
>> <global_array>
>> diff --git a/ld/testsuite/ld-riscv-elf/gp-relax.s
>> b/ld/testsuite/ld-riscv-elf/gp-relax.s
>> new file mode 100644
>> index 00000000000..36addbc770c
>> --- /dev/null
>> +++ b/ld/testsuite/ld-riscv-elf/gp-relax.s
>> @@ -0,0 +1,30 @@
>> +       .globl  global_var
>> +       .section        .sbss,"aw",@nobits
>> +       .align  2
>> +       .type   global_var, @object
>> +       .size   global_var, 4
>> +global_var:
>> +       .zero   4
>> +
>> +       .globl  global_array
>> +       .bss
>> +       .align  3
>> +       .type   global_array, @object
>> +       .size   global_array, 3600
>> +global_array:
>> +       .zero   3600
>> +
>> +       .text
>> +       .align  1
>> +       .globl  _start
>> +       .type   _start, @function
>> +_start:
>> +       lui     a3,%hi(global_array)
>> +       addi    a3,a3,%lo(global_array)
>> +
>> +       lui     a4,%hi(global_array + 0x600)
>> +       addi    a4,a4,%lo(global_array + 0x600)
>> +
>> +.LA0:
>> +        auipc  a5,%pcrel_hi(global_array)
>> +       lw      a0,%pcrel_lo(.LA0)(a5)
>> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> index 947a266ba72..6a04955b23b 100644
>> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> @@ -124,6 +124,7 @@ if [istarget "riscv*-*-*"] {
>>      run_dump_test "pcgp-relax-01"
>>      run_dump_test "pcgp-relax-01-norelaxgp"
>>      run_dump_test "pcgp-relax-02"
>> +    run_dump_test "gp-relax"
>>      run_dump_test "c-lui"
>>      run_dump_test "c-lui-2"
>>      run_dump_test "disas-jalr"
>> --
>> 2.17.1
>>
>>