Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes

[Nelson Chu <nelson@rivosinc.com>]

Hi,

On Thu, Oct 13, 2022 at 8:06 PM Clément Chigot via Binutils
<binutils@sourceware.org> wrote:
>
> Gentle ping
>
> Thanks,
> Clément
>
> On Thu, Oct 6, 2022 at 1:46 PM Clément Chigot <chigot@adacore.com> wrote:
> >
> > The section relaxation can delete some bytes resulting in the symbols'
> > value being modified.
> > As the linker messages retrieve a symbol information using the
> > outsymbols field of abfd, it must be updated as well.

Interesting, I never noticed this.  If this is necessary, then other
targets should also update the output symbols when relaxing (nds32 and
sh), but it seems like they don't do that as well.

> > bfd/ChangeLog:
> >
> >         * elfnn-riscv.c (riscv_relax_delete_bytes): Update
> >         abfd->outsymbols.
> >
> > ld/ChangeLog:
> >
> >         * testsuite/ld-riscv-elf/ld-riscv-elf.exp: New test.
> >         * testsuite/ld-riscv-elf/undefined-align.d: New test.
> >         * testsuite/ld-riscv-elf/undefined-align.s: New test.
> > ---
> >  bfd/elfnn-riscv.c                           | 23 +++++++++++++++++++++
> >  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp  |  1 +
> >  ld/testsuite/ld-riscv-elf/undefined-align.d |  5 +++++
> >  ld/testsuite/ld-riscv-elf/undefined-align.s | 10 +++++++++
> >  4 files changed, 39 insertions(+)
> >  create mode 100644 ld/testsuite/ld-riscv-elf/undefined-align.d
> >  create mode 100644 ld/testsuite/ld-riscv-elf/undefined-align.s
> >
> > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> > index 3d2ddf4e651..7a6b66dcd8a 100644
> > --- a/bfd/elfnn-riscv.c
> > +++ b/bfd/elfnn-riscv.c
> > @@ -4060,6 +4060,7 @@ riscv_relax_delete_bytes (bfd *abfd,
> >    unsigned int sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
> >    struct bfd_elf_section_data *data = elf_section_data (sec);
> >    bfd_byte *contents = data->this_hdr.contents;
> > +  asymbol **outsyms = bfd_get_outsymbols (abfd);
> >
> >    /* Actually delete the bytes.  */
> >    sec->size -= count;
> > @@ -4158,6 +4159,28 @@ riscv_relax_delete_bytes (bfd *abfd,
> >         }
> >      }
> >
> > +  /* As linker messages are getting symbols through outsymbols field of abfd,
> > +     it must be adjusted too.  */
> > +  if (outsyms == NULL)
> > +    {
> > +      if (!bfd_generic_link_read_symbols (abfd))
> > +       link_info->callbacks->einfo (_("%F%P: %pB: could not read symbols: %E\n"), abfd);
> > +      outsyms = bfd_get_outsymbols (abfd);
> > +    }
> > +
> > +  for (i = 0; i < bfd_get_symcount (abfd); i++)
> > +    {
> > +      asymbol *sym = outsyms[i];
> > +
> > +      if (sym->section != sec)
> > +       continue;
> > +
> > +      /* If the symbol is in the range of memory we just moved, we
> > +        have to adjust its value.  */
> > +      if (sym->value > addr && sym->value <= toaddr)
> > +       sym->value -= count;
> > +    }
> > +
> >    return true;
> >  }
> >
> > diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> > index df89e0ee68b..86f1d05bc08 100644
> > --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> > +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> > @@ -176,6 +176,7 @@ if [istarget "riscv*-*-*"] {
> >         [list "Weak reference 64" "-T weakref.ld -m[riscv_choose_lp64_emul]" "" \
> >             "-march=rv64i -mabi=lp64" {weakref64.s} \
> >             {{objdump -d weakref64.d}} "weakref64"]]
> > +    run_dump_test "undefined-align"
> >
> >      # The following tests require shared library support.
> >      if ![check_shared_lib_support] {
> > diff --git a/ld/testsuite/ld-riscv-elf/undefined-align.d b/ld/testsuite/ld-riscv-elf/undefined-align.d
> > new file mode 100644
> > index 00000000000..c8cbb84ac7c
> > --- /dev/null
> > +++ b/ld/testsuite/ld-riscv-elf/undefined-align.d
> > @@ -0,0 +1,5 @@
> > +#name: undefined with alignment
> > +#source: undefined-align.s
> > +#as:
> > +#ld: --relax
> > +#error: \A[^\n]*\.o: in function `_start':\n\(\.text\+0x0\): undefined reference to `foo'\Z
> > diff --git a/ld/testsuite/ld-riscv-elf/undefined-align.s b/ld/testsuite/ld-riscv-elf/undefined-align.s
> > new file mode 100644
> > index 00000000000..577557c663a
> > --- /dev/null
> > +++ b/ld/testsuite/ld-riscv-elf/undefined-align.s
> > @@ -0,0 +1,10 @@
> > +# Make sure that the linker messages take into account the modification
> > +# of a symbol's value made during the section relaxation.
> > +# Here, "_start" will have an offset made by ".align 4" which will be
> > +# removed during the relaxation of R_RISCV_ALIGN.
> > +       .text
> > +       .align  4
> > +       .globl  _start
> > +       .type   _start, @function
> > +_start:
> > +       call    foo

Tested with undefined-align.s, I get the following error message
before applying this patch,

% riscv64-unknown-elf-ld tmp.o

riscv64-unknown-elf-ld: tmp.o:(.text+0x0): undefined reference to `foo'

And get the error after applying this patch,

% riscv64-unknown-elf-ld tmp.o

riscv64-unknown-elf-ld: tmp.o: in function `_start':

(.text+0x0): undefined reference to `foo'

It reports more detail, but I cannot see if the removed offset may
cause the wrong message?

Thanks
Nelson

> > --
> > 2.25.1
> >