Re: [PATCH] RISC-V: Do not gp relax against an ABS symbol if it is far away.

[Nelson Chu <nelson@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 6660 bytes --]

Maybe, if the shrink code is more than the data segment alignment in the
default linker script (0x1000), then probably still have a chance to get
the truncated gp by using default linker script...  I will suggest if that
is so, then we just have a linker option to disallow/allow the gp relax for
any abs symbol, that should be more intuitive.

On Fri, Jul 28, 2023 at 9:02 AM Nelson Chu <nelson@rivosinc.com> wrote:

> Relaxation should shrink the text, but since the default linker script has
> huge data segment alignment, data after the segment alignment shouldn't be
> affected and shrinked, so ideally  it should be safe by using default
> linker script.
>
> Do you have a reduced test that shows we still have a chance to get the
> relax error by using the default linker script?
>
> On Fri, Jul 28, 2023 at 5:08 AM Palmer Dabbelt <palmer@rivosinc.com>
> wrote:
>
>> On Thu, 27 Jul 2023 09:48:24 PDT (-0700), Joseph.Faulls@imgtec.com wrote:
>> > Relaxations can cause the gp to move after it has been decided to gp
>> > relax. Against an absolute symbol, the distance may change such that the
>> > offset can no longer fit in the 12-bit immediate field.
>> >
>> > bfd/
>> >         * elfnn-riscv.c (_bfd_riscv_relax_pc) Do not gp relax against
>> and ABS
>> >           symbol if it is far away.
>> > ld/
>> >         * ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated.
>> >         * ld/testsuite/ld-riscv-elf/gp-relax-abs*: New testcases.
>> > ---
>> > bfd/elfnn-riscv.c                            | 12 ++++++++++++
>> > ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s |  3 +++
>> > ld/testsuite/ld-riscv-elf/gp-relax-abs.d     | 14 ++++++++++++++
>> > ld/testsuite/ld-riscv-elf/gp-relax-abs.s     |  5 +++++
>> > ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp   |  1 +
>> > 5 files changed, 35 insertions(+)
>> > create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s
>> > create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax-abs.d
>> > create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax-abs.s
>> >
>> > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>> > index 09aa7be225e..79e29e8b272 100644
>> > --- a/bfd/elfnn-riscv.c
>> > +++ b/bfd/elfnn-riscv.c
>> > @@ -4885,6 +4885,18 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED,
>> >               max_alignment = _bfd_riscv_get_max_alignment (sec, gp);
>> >               htab->max_alignment_for_gp = max_alignment;
>> >             }
>> > +         /* If the symbol is in the abs section, relaxation could
>> cause the gp
>> > +          * to move such that the gp relocation is no longer possible.
>>
>> I'm not quite sure what you mean by GP moving -- GP's the register, so
>> it doesn't have a value until runtime.  GP is meant to equal
>> __global_pointer$, but that's usually a data symbol and thus doesn't
>> move as a result of relaxation.
>>
>> We do have some other issues related to SHN_ABS (for example position
>> independent vs PC relative stuff), so it's possible there's some other
>> bug here?
>>
>> > +          * Conservatively half the allowed distance, as it cannot be
>> that
>> > +          * gp moves more than this, i.e. more than half the
>> instructions be
>> > +          * deleted due to relaxation.  Do this by adjusting
>> reserve_size.  */
>> > +         if (sym_sec->output_section == bfd_abs_section_ptr)
>> > +           {
>> > +             if (symval >= gp)
>> > +               reserve_size += (symval - gp) / 2;
>> > +             else
>> > +               reserve_size += (gp - symval) / 2;
>>
>> We can have more than a factor of two from relaxation, for example from
>> alignment.  If a hueristic is all we can do then I guess we'll have to
>> live with it, but I think I'd want to understand the movement
>>
>
> An extreme case is that all the instruction can be relaxed without any
> huge alignments between text and data, so I guess that's probably why the
> half of distance comes from.  If the default linker script may cause the
> problem, then we will need to know more details and try to resolve it.  But
> if this only occurs without the default linker script, then I will suggest
> users disable that relax patterns by the .norelax directive, or related
> options of compiler/assembler/linker.  Or maybe we can have an option for
> users to reject gp relax when meeting abs symbols, if without using default
> linker script.  However, please give a reduced case by using default linker
> script that will be broken in detailed, thanks a lot.
>
> Nelson
>
>
>> > +           }
>> >         }
>> >      }
>> >
>> > diff --git a/ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s
>> b/ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s
>> > new file mode 100644
>> > index 00000000000..a018bb3a50a
>> > --- /dev/null
>> > +++ b/ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s
>> > @@ -0,0 +1,3 @@
>> > +.section .data
>> > +.globl sym
>> > +.set sym,0x10804
>> > diff --git a/ld/testsuite/ld-riscv-elf/gp-relax-abs.d
>> b/ld/testsuite/ld-riscv-elf/gp-relax-abs.d
>> > new file mode 100644
>> > index 00000000000..2c7ab3a2579
>> > --- /dev/null
>> > +++ b/ld/testsuite/ld-riscv-elf/gp-relax-abs.d
>> > @@ -0,0 +1,14 @@
>> > +#source: gp-relax-abs.s
>> > +#source: gp-relax-abs-sym.s
>> > +#as: -march=rv64ic -mabi=lp64
>> > +#ld: -Tcode-model-01.ld -melf64lriscv
>> > +#objdump: -d
>> > +
>> > +.*:[   ]+file format .*
>> > +
>> > +Disassembly of section \.text:
>> > +
>> > +[0-9a-f]+ <_start>:
>> > +.*auipc.*
>> > +.*lw.*# [0-9a-f]* <sym>
>> > +#pass
>> > diff --git a/ld/testsuite/ld-riscv-elf/gp-relax-abs.s
>> b/ld/testsuite/ld-riscv-elf/gp-relax-abs.s
>> > new file mode 100644
>> > index 00000000000..db2103bafd1
>> > --- /dev/null
>> > +++ b/ld/testsuite/ld-riscv-elf/gp-relax-abs.s
>> > @@ -0,0 +1,5 @@
>> > +.text
>> > +.global _start
>> > +_start:
>> > +    auipc t0, %pcrel_hi(sym)
>> > +    lw t0, %pcrel_lo(_start)(t0)
>> > diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> > index 947a266ba72..a53a2758991 100644
>> > --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> > +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>> > @@ -172,6 +172,7 @@ if [istarget "riscv*-*-*"] {
>> >      run_dump_test "attr-merge-priv-spec-failed-06"
>> >      run_dump_test "attr-phdr"
>> >      run_dump_test "relax-max-align-gp"
>> > +    run_dump_test "gp-relax-abs"
>> >      run_dump_test "uleb128"
>> >      run_ld_link_tests [list \
>> >         [list "Weak reference 32" "-T weakref.ld
>> -m[riscv_choose_ilp32_emul]" "" \
>>
>