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

[Palmer Dabbelt <palmer@rivosinc.com>]

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 

> +           }
>         }
>      }
>
> 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]" "" \