public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] RISC-V: fix linker message when relaxation deletes bytes
@ 2022-10-06 11:46 Clément Chigot
  2022-10-13 12:05 ` Clément Chigot
  0 siblings, 1 reply; 12+ messages in thread
From: Clément Chigot @ 2022-10-06 11:46 UTC (permalink / raw)
  To: binutils; +Cc: palmer, Clément Chigot

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.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-06 11:46 [PATCH] RISC-V: fix linker message when relaxation deletes bytes Clément Chigot
@ 2022-10-13 12:05 ` Clément Chigot
  2022-10-14  5:08   ` Nelson Chu
  0 siblings, 1 reply; 12+ messages in thread
From: Clément Chigot @ 2022-10-13 12:05 UTC (permalink / raw)
  To: binutils; +Cc: palmer

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-13 12:05 ` Clément Chigot
@ 2022-10-14  5:08   ` Nelson Chu
  2022-10-14  7:24     ` Clément Chigot
  0 siblings, 1 reply; 12+ messages in thread
From: Nelson Chu @ 2022-10-14  5:08 UTC (permalink / raw)
  To: Clément Chigot; +Cc: binutils

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-14  5:08   ` Nelson Chu
@ 2022-10-14  7:24     ` Clément Chigot
  2022-10-15  0:42       ` Jeff Law
                         ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Clément Chigot @ 2022-10-14  7:24 UTC (permalink / raw)
  To: Nelson Chu; +Cc: binutils

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-14  7:24     ` Clément Chigot
@ 2022-10-15  0:42       ` Jeff Law
  2022-10-17  8:32         ` Clément Chigot
  2022-10-31  9:57       ` Clément Chigot
  2022-11-18  1:43       ` Nelson Chu
  2 siblings, 1 reply; 12+ messages in thread
From: Jeff Law @ 2022-10-15  0:42 UTC (permalink / raw)
  To: Clément Chigot, Nelson Chu; +Cc: binutils


On 10/14/22 01:24, Clément Chigot via Binutils 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.
>
>

The target has to adjust the local and global symbols defined in the 
relaxed section.      I haven't done relaxing in a long time, but the 
one I'm most familiar with is the H8.  If you look at 
elf32_h8_relax_delete_bytes you'll see suitable code to update the local 
and global symbols.

That code is very old and may not have seen many updates through the 
last 20 years, but it should give you a good idea of how ports have 
handled this.

Jeff


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-15  0:42       ` Jeff Law
@ 2022-10-17  8:32         ` Clément Chigot
  2022-10-19 17:38           ` Jeff Law
  0 siblings, 1 reply; 12+ messages in thread
From: Clément Chigot @ 2022-10-17  8:32 UTC (permalink / raw)
  To: Jeff Law; +Cc: Nelson Chu, binutils

Hi Jeff,

On Sat, Oct 15, 2022 at 2:42 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
> On 10/14/22 01:24, Clément Chigot via Binutils 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.
> >
> >
>
> The target has to adjust the local and global symbols defined in the
> relaxed section.      I haven't done relaxing in a long time, but the
> one I'm most familiar with is the H8.  If you look at
> elf32_h8_relax_delete_bytes you'll see suitable code to update the local
> and global symbols.

Yeah, that's what was already done in the Risc-V function:
riscv_relax_delete_bytes.
However, local and global symbols aren't enough for linker warnings
and errors. These ones are using the "outsymbols" BFD field which IIUC
is reading the object files directly and thus cannot know about the
deleted bytes.
So I guess H8 might have the same kind of errors. Are ld-undefined
tests succeeding ?

Thanks,
Clémebt

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-17  8:32         ` Clément Chigot
@ 2022-10-19 17:38           ` Jeff Law
  2022-10-20  7:20             ` Clément Chigot
  0 siblings, 1 reply; 12+ messages in thread
From: Jeff Law @ 2022-10-19 17:38 UTC (permalink / raw)
  To: Clément Chigot; +Cc: Nelson Chu, binutils


On 10/17/22 02:32, Clément Chigot wrote:
>
> Yeah, that's what was already done in the Risc-V function:
> riscv_relax_delete_bytes.
> However, local and global symbols aren't enough for linker warnings
> and errors. These ones are using the "outsymbols" BFD field which IIUC
> is reading the object files directly and thus cannot know about the
> deleted bytes.

But if you're getting the outsymbols for the output bfd, then they 
should have the right address.  If they don't, then the symbol table in 
the output file is going to be wrong.  And changing the local/global 
symbols when relaxing should directly influence the outsymbols for the 
output bfd.



> So I guess H8 might have the same kind of errors. Are ld-undefined
> tests succeeding ?

Which test specifically?  None are failing, but some are 
UNTESTED/UNSUPPORTED:


UNSUPPORTED: -shared --entry foo archive
UNSUPPORTED: -shared --entry foo -u foo archive
UNTESTED: undefined
UNTESTED: undefined function
UNTESTED: undefined line
UNSUPPORTED: undefined symbols in shared lib
UNSUPPORTED: weak undefined function symbols in shared lib


Do I need to turn on relaxing by default to see this issue (unlike 
riscv, h8 does not enable relaxing by default).

jeff


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-19 17:38           ` Jeff Law
@ 2022-10-20  7:20             ` Clément Chigot
  0 siblings, 0 replies; 12+ messages in thread
From: Clément Chigot @ 2022-10-20  7:20 UTC (permalink / raw)
  To: Jeff Law; +Cc: Nelson Chu, binutils

Hi Jeff

On Wed, Oct 19, 2022 at 7:38 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
> On 10/17/22 02:32, Clément Chigot wrote:
> >
> > Yeah, that's what was already done in the Risc-V function:
> > riscv_relax_delete_bytes.
> > However, local and global symbols aren't enough for linker warnings
> > and errors. These ones are using the "outsymbols" BFD field which IIUC
> > is reading the object files directly and thus cannot know about the
> > deleted bytes.
>
> But if you're getting the outsymbols for the output bfd, then they
> should have the right address.  If they don't, then the symbol table in
> the output file is going to be wrong.  And changing the local/global
> symbols when relaxing should directly influence the outsymbols for the
> output bfd.

For Risc-V, it seems that outsymbols aren't needed for the output file
symbol table. I've tried with different programs, bfd_get_outsymbols
seems to be reached only when an error has to be displayed.

> > So I guess H8 might have the same kind of errors. Are ld-undefined
> > tests succeeding ?
>
> Which test specifically?  None are failing, but some are
> UNTESTED/UNSUPPORTED:
>
>
> UNSUPPORTED: -shared --entry foo archive
> UNSUPPORTED: -shared --entry foo -u foo archive
> UNTESTED: undefined
> UNTESTED: undefined function
> UNTESTED: undefined line
> UNSUPPORTED: undefined symbols in shared lib
> UNSUPPORTED: weak undefined function symbols in shared lib
>
>
> Do I need to turn on relaxing by default to see this issue (unlike
> riscv, h8 does not enable relaxing by default).

Hum, I guess they are probably disabled because you haven't a compiler
available for H8 or at least it's not properly set up for the
testsuites.
The way I'm enabling them is by passing a "board" to DejaGNU (see
[1]). This board will have the correct CFLAGS and LDFLAGS.
  | set_board_info ldflags  "${CFLAGS}"
  | set_board_info ldflags  "${LDFLAGS}"
  | set_currtarget_info ldflags  "${CFLAGS}"
  | set_currtarget_info ldflags  "${LDFLAGS}"

Note that I stated in a recent patch improving the support for
LDFLAGS, I'm not sure why both set_board and set_currtarget are
needed. But I do need both to make it work.

[1] https://www.gnu.org/software/dejagnu/manual/Adding-a-new-board.html
[2] https://sourceware.org/pipermail/binutils/2022-October/123314.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-14  7:24     ` Clément Chigot
  2022-10-15  0:42       ` Jeff Law
@ 2022-10-31  9:57       ` Clément Chigot
  2022-11-17  9:36         ` Clément Chigot
  2022-11-18  1:43       ` Nelson Chu
  2 siblings, 1 reply; 12+ messages in thread
From: Clément Chigot @ 2022-10-31  9:57 UTC (permalink / raw)
  To: Nelson Chu; +Cc: binutils

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-31  9:57       ` Clément Chigot
@ 2022-11-17  9:36         ` Clément Chigot
  0 siblings, 0 replies; 12+ messages in thread
From: Clément Chigot @ 2022-11-17  9:36 UTC (permalink / raw)
  To: binutils; +Cc: Nelson Chu

Hi

Gentle ping

On Mon, Oct 31, 2022 at 10:57 AM Clément Chigot <chigot@adacore.com> wrote:
>
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-10-14  7:24     ` Clément Chigot
  2022-10-15  0:42       ` Jeff Law
  2022-10-31  9:57       ` Clément Chigot
@ 2022-11-18  1:43       ` Nelson Chu
  2022-11-18  8:17         ` Clément Chigot
  2 siblings, 1 reply; 12+ messages in thread
From: Nelson Chu @ 2022-11-18  1:43 UTC (permalink / raw)
  To: Clément Chigot; +Cc: binutils

On Fri, Oct 14, 2022 at 3:24 PM 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'
> ?

The error message doesn't seem wrong without your patch, just don't
report "in function `_start':".  The comment in your testcase said the
offset should be removed after handling alignment, but I get the same
zero offset with or without your patch.  It would be great if you can
provide other cases to show how the offset reported by error message
will be wrong.

Thanks
Nelson

> Thanks,
> Clément

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] RISC-V: fix linker message when relaxation deletes bytes
  2022-11-18  1:43       ` Nelson Chu
@ 2022-11-18  8:17         ` Clément Chigot
  0 siblings, 0 replies; 12+ messages in thread
From: Clément Chigot @ 2022-11-18  8:17 UTC (permalink / raw)
  To: Nelson Chu; +Cc: binutils

On Fri, Nov 18, 2022 at 2:43 AM Nelson Chu <nelson@rivosinc.com> wrote:
>
> On Fri, Oct 14, 2022 at 3:24 PM 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'
> > ?
>
> The error message doesn't seem wrong without your patch, just don't
> report "in function `_start':".  The comment in your testcase said the
> offset should be removed after handling alignment, but I get the same
> zero offset with or without your patch.  It would be great if you can
> provide other cases to show how the offset reported by error message
> will be wrong.

AFAICT, the offset will never be wrong because it's correctly updated
during the relax in riscv_relax_delete_bytes:
 |       data->relocs[i].r_offset -= count;

However, the outsymbols, which are used inside ldmisc.c to find the
function, aren't.
Maybe my message is misleading, would something like this be clearer ?
  | # Make sure that the linker messages take into account the modification
  | # of a symbol's value made during the section relaxation when looking
  | # for the function name.
  | # Here, "_start" will have an offset made by ".align 4" which will be
  | # removed during the relaxation of R_RISCV_ALIGN. Ensure that
  | # that the error has both the new offset (0x0 instead of 0xc) and it's
  | # correctly showing "in function `_start`".

Thanks,
Clément

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-11-18  8:18 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 11:46 [PATCH] RISC-V: fix linker message when relaxation deletes bytes Clément Chigot
2022-10-13 12:05 ` Clément Chigot
2022-10-14  5:08   ` Nelson Chu
2022-10-14  7:24     ` Clément Chigot
2022-10-15  0:42       ` Jeff Law
2022-10-17  8:32         ` Clément Chigot
2022-10-19 17:38           ` Jeff Law
2022-10-20  7:20             ` Clément Chigot
2022-10-31  9:57       ` Clément Chigot
2022-11-17  9:36         ` Clément Chigot
2022-11-18  1:43       ` Nelson Chu
2022-11-18  8:17         ` Clément Chigot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).