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

[Nelson Chu]

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...


> 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.

Yeah, thanks for pointing this out and describing the behavior in
detail.  It's really helpful and important.


> On second thought, this looks like a pre-existing problem, it is just that your patch makes it very slightly worse.

Umm when we are in the relax pass 1 originally, we won't actually
delete the code for relax_pc, just use R_RISCV_DELETE to record that
the code should be deleted later in the relax pass 2.  Therefore, just
moving the relax_pc to the later relax pass should be fine, and should
get the same result as before.

>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.

It's a great idea, and we can use it to make `again` more effective.

> 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.

OK, I will fix that in another patch.


Thanks
Nelson