[PATCH] RISC-V: Relax PCREL to GPREL while doing other relaxations is dangerous.

[Jim Wilson]

On Tue, Nov 17, 2020 at 7:39 PM Nelson Chu <nelson.chu@sifive.com> wrote:

>         bfd/
>         * elfnn-riscv.c (_bfd_riscv_relax_section):  Add a new relax pass
>         to do the pcgp relaxation later, after the lui and call
> relaxations,
>         but before the delete and alignment relaxations.
>
>         ld/
>         * emultempl/riscvelf.em (riscv_elf_before_allocation): Change
>         link_info.relax_pass from 3 to 4.
>         * testsuite/ld-riscv-elf/pcgp-relax.d: New testcase.
>         * testsuite/ld-riscv-elf/pcgp-relax.s: Likewise.
>         * testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated.
>

This looks OK to me.

My only worry is that we might miss some relaxation opportunities if they
are in separate passes.  Suppose we have a call that is just slightly out
of range, but that there is a relaxable pc-rel reloc in the middle that if
relaxed would pull it back in range.  So doing the call relaxation first
before pc-rel relaxation means we won't be able to relax the call.  Though
I think this may be already a problem if it is a forward call, and this
currently only works for backwards calls.  Anyways, normally you would
expect a second call to _bfd_riscv_relax_section to catch what the first
one missed, as the first one will set *again if anything was relaxed.
However, there is a complication with R_RISCV_ALIGN, in that it must be
done in the very last _bfd_riscv_relax_section call.  Otherwise, the code
won't get the right alignments.  Currently, _bfd_riscv_relax_align sets
sec_flg0 to indicate that we did the alignment relaxation, and in
_bfd_riscv_relax_section it refuses to do anything if this flag was set.
So if we have any R_RISCV_ALIGN relocs the second call to
_bfd_riscv_relax_section won't do anything.  On second thought, this looks
like a pre-existing problem, it is just that your patch makes it very
slightly worse.  I think we need to refuse to do the R_RISCV_ALIGN
relaxation if *again is already set, so that the next call to
_bfd_riscv_relax_section can relax again.  Eventually we will make it to
pass 3 with *again false and then we can do the R_RISCV_ALIGN which does
not set *again itself so relaxation will stop.  If we do this, we maybe
don't need the sec_flg0 code anymore.  This can be a separate patch as it
is a separate bug report.

Also, fyi, I noticed the comment before the _bfd_riscv_relax_delete
function is wrong.  It is the same comment for the _bfd_riscv_relax_pc
function so it looks like it was copied and then someone forgot to edit
it.  This has nothing to do with your patch, or my R_RISCV_ALIGN suggestion
though.  So this can be a separate trivial fix.

Jim