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

[Nelson Chu]

On Mon, Oct 11, 2021 at 4:43 PM Lewis Revill <lewis.revill@embecosm.com> wrote:
>
> On Fri, 8 Oct 2021 at 17:24, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> > I haven't looked at the patch (and must have missed the earlier
> > discussion), but the specific case that I was trying to solve with the
> > R_RISCV_DELETE split was something like
> >
> >
> >     r0: auipc t0, symbol0
> >     r1: auipc t1, symbol1
> >         ld    t0, symbol0, r0 # reloc points to r0
> >         ld    t1, symbol1, r1 # reloc points to r1
> >
> > in this case, if the auipc at r0 gets deleted then we end up with an
> > aliasing issue and need some way to disambiguate which of the two
> > symbols we're targeting.  I don't remember if we ever put a test case in
> > there for this, but it did manifest in real code (IIRC it was a Linux
> > build, but you need to twiddle all the compiler flags so it'll schedule
> > the auipc-based pairs).

Umm I think we don't have the one in the current ld testcases, but
maybe it is worth having.

> > The idea behing the extra pass here was to split this into two phases,
> > so we could resolve the addresses before creating the ambiguities.
>
> Thanks Palmer, I've checked the behaviour of my patch versus before my
> patch and it appears the same. But just to be sure do you maybe have a
> full testcase with more context, EG location of symbol0 and symbol1? I
> will also have a look through and try to reason why my patch would still
> be able to handle this.

Consider the following testcase, and compile with the patch,

nelson@LAPTOP-QFSGI1F2:~/test$ cat tmp.s
        .text
        .globl _start
_start:
.L1:    auipc   a0, %pcrel_hi(data_a)
.L2:    auipc   a1, %pcrel_hi(data_b)
        addi    a0, a0, %pcrel_lo(.L1)
        addi    a1, a1, %pcrel_lo(.L2)

        .data
        .word 0x0
        .globl data_a
data_a:
        .word 0x1

        .section .rodata
        .globl data_b
data_b:
        .word 0x2
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-as
tmp.s -o tmp.o
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-ld
tmp.o
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-objdump
-d -Mno-aliases a.out

a.out:     file format elf64-littleriscv


Disassembly of section .text:

00000000000100e8 <_start>:
   100e8:       00000597                auipc   a1,0x0
   100ec:       80418513                addi    a0,gp,-2044 # 110fc <data_a>
   100f0:       80418593                addi    a1,gp,-2044 # 110fc <data_a>

The second addi looks wrong, since the symbol should be data_b in the
rodata, which cannot be relaxed generally.  What Palmer is concerned
about is that - it is hard to connect the high and low parts of pcrel
relocations correctly when we are also deleting instructions at the
same time.  The expected output should be like that,

nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-ld
tmp.o
nelson@LAPTOP-QFSGI1F2:~/test$
~/binutils-dev/build-elf64-upstream/build-install/bin/riscv64-unknown-elf-objdump
-d -Mno-aliases a.out

a.out:     file format elf64-littleriscv


Disassembly of section .text:

00000000000100e8 <_start>:
   100e8:       00000597                auipc   a1,0x0
   100ec:       80418513                addi    a0,gp,-2044 # 110fc <data_a>
   100f0:       00c58593                addi    a1,a1,12 # 100f4 <data_b>


However, to make sure we are now aligned, my imagination is that,

* relax pass 0 (lui, call, tprel, update the pcgp table when deleting
the code; pcrel but don't delete auipc at this pass, just marked as
R_RISCV_DELETE)
* relax pass 1 (delete auipc for pcrel)
* relax pass 2 (alignment) a

I understand that using another relax pass to delete the auipc should
also reduce the chance of all relaxations, but the correctness should
be the first priority.  I would suggest that we still delete the auipc
in another relax pass like what we did as usual.  The advantage is
that we don’t have to spend too much time to bear the risk of the
large changes, and can also resolve the small region problem and the
issue of commit abd20cb.  On the issue of the reduction of relaxed
chances, we could try to improve this in the future patches, it isn't
very urgent for me at this stage.

Oh, another thing is that, I cannot apply the patch directly since the
updated patch looks corrupt with many newlines?  Anyway, we don't need
to update the ChangeLogs files when sending the patch, just writing
the ChangeLogs in the commit comments should be enough.  Otherwise we
always need to resolve the conflicts since the ChangeLogs files are
changed frequently.  However, maybe using the git send-mail to send
the patch, or attach the format patch in the mail directly should be
more convenient.

Thanks
Nelson