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

[Jim Wilson]

On Thu, Nov 19, 2020 at 5:45 PM Nelson Chu <nelson.chu@sifive.com> wrote:

> On Fri, Nov 20, 2020 at 7:13 AM Jim Wilson <jimw@sifive.com> wrote:
> > 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.
>
> Agree with this, but I think the backward calls also can not work as
> expected right now.  The relax_pc only changes the R_RISCV_PCREL_HI20
> relocs to R_RISCV_DELETE, but waits for the next relax pass to
> actually delete the code.  That means we will get the same symbols for
> relax_call, no matter if the patterns are relaxed by the relax_pc or
> not.  Therefore, we should set `again` to TRUE, and try to run the
> relaxation round many times (I think one round means run relax pass 0
> to 2 one time), to let the call case you mentioned above have a chance
> to be relaxed.  However, the `again` might fail because of the
> relax_align that you mentioned as follows...
>

The R_RISCV_DELETE serves a special purpose.  We normally relax the auipc
before the add/ld/jalr/whatever instruction that it is paired with.  The
%lo relocation requires access to the address of the auipc to relax it.
But if we delete the auipc instruction before we relax the %lo, then it no
longer has an address, and the relaxation of the %lo can fail and produce
the wrong result.  So instead of deleting the auipc instruction, we give it
a R_RISCV_DELETE reloc.  Then after we have done the %lo relaxations we go
back through and process the R_RISCV_DELETE relocs to actually delete the
auipc instruction.

I believe the code already sets again to TRUE every place where we delete
or shorten an instruction during relaxation.  We just aren't taking good
advantage of this as any R_RISCV_ALIGN reloc processing prevents the next
call to _bfd_riscv_relax_section from doing anything.

In case I wasn't clear, I think your patch is fine.  I just think we can
make more improvements with a follow on patch.

Jim