[PATCH] bfd/RISC-V: Repeat a single target relax pass instead of 3

[Lewis Revill]

Hi Nelson,

Thanks for the review!

On Fri, 8 Oct 2021 at 09:52, Nelson Chu <nelson.chu@sifive.com> wrote:
>
> This looks interesting since you are removing the relax pass, which
> deletes the bytes that pass 1 made obsolete.  I'm not sure if the
> change will cause any side effects, so I would suggest keeping the
> delete relax pass separating, like what we always did before.

So this pass is no longer necessary since we only used R_RISCV_DELETE to
delay deleting bytes from within the relax_pc pass - because it was
dangerous to delete bytes while still doing the pcrel_hi/pcrel_lo
tracking. Now that this pass adds the capability to safely delete bytes
while still keeping track of pcrel_hi locations and symbols this is no
longer the case. In addition adding that second target pass would mean
that we would be back to needing to solve the restart_relax problem
since we couldn't use 'again' to start the whole process again.

> The returned boolean `updated' looks useless and redundant, so we
> probably can remove it.

Thanks, you're right.

> Likewise.  Besides, I suppose you also need to check if the `sym_sec'
> of riscv_pcgp_relocs is the same as the section in which we are
> deleting the bytes.  That is  - we can only modify the symbol value
> when the symbol section is the same as the deleting section.

Ah yes, good catch!

> In this place we make sure that only the symbols defined in the same
> `sec' will be modified.  But you only pass the symbol value into the
> riscv_update_pcgp_reloc_symvals, and use the value to decide if the
> pcgp table's symbols are the same one.  I think this is dangerous
> since you may modify the symbols which are actually defined in other
> sections.
>
> I think `riscv_update_pcgp_reloc_symvals' should be moved outside the
> if statement, and you only need to trace the whole pcgp tables once,
> with checking the `sym_sec' and `hi_addr', maybe something like this,
>
> if (sym_sec == sec && hi_addr > addr && hi_addr <= toaddr)

Hmm okay I'll see what I can do to update it like this. I think I
understand what you mean by 'only need to trace the whole pcgp tables
once'. So I would be able to just go through the entire table and check
if the deleted bytes occurred before the hi_addr right?

> The alignment relaxations are behind the pcgc relaxations, so we don't
> need to update the pcgp tables in fact.

Good point, I can probably pass null for this pass and check on the
other end.