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

["Clément Chigot" <chigot@adacore.com>]

Hi,

Gentle ping

On Fri, Oct 14, 2022 at 9:24 AM Clément Chigot <chigot@adacore.com> wrote:
>
> On Fri, Oct 14, 2022 at 7:08 AM Nelson Chu <nelson@rivosinc.com> wrote:
> >
> > 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.
>
> Yeah, I agree there are probably other targets having a similar issue.
> I don't know at what extend outsymbols is being used for. But the fact
> that is being filled with "untouched" symbols in object files looks
> weird to me. Maybe there is something to look into this direction.
> For now, this patch aims just to fix ld-undefined on Risc-V targets.
>
> > > > 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?
>
> I'm not sure I understand what you mean.
> As in the object file, _start starts at 0xc, you mean that the error
> message should be something like:
>   |  riscv64-unknown-elf-ld: tmp.o: in function `_start':
>   |  (.text+0xc): undefined reference to `foo'
> ?
>
> Thanks,
> Clément