avoiding undesirable undef[weak] syms after relocatable linking

avoiding undesirable undef[weak] syms after relocatable linking

[Alexandre Oliva]

The following, say t.s, is dystilled from libstdc++-v3's
floating_to_chars.o:

.section .text.foobar,"axG",@progbits,foo,comdat
.weak foo
.type foo,@function
foo:
# .dc.a undef
.size foo, . - foo
.weak bar
.set bar, foo
.text
.global baz
.type baz,@function
baz:
.dc.a foo
.size baz, . - baz

The weak definitions were originally implicit instantiations of
to_chars_i<unsigned int128_t>, and the aliases were originally meant for
abi compatibility because of changes in int128_t mangling.

The following, say m.s, is dystilled from a
libstdc++-v3/testsuite/std/time/clock/system/io.cc:

.section .text.foobar,"axG",@progbits,foo,comdat
.weak foo
.type foo,@function
foo:
.size foo, . - foo
.text
.global _start
.set _start,foo
.dc.a baz

And here's how to trigger the problem:

as t.s -o t.o && ar cr t.a t.o && ranlib t.a && as m.s -o m.o &&
ld m.o -o m -r && nm m | grep bar
                 w bar

This is probably expected behavior: the weakly-defined symbol in the
archive lives in a discarded section, so it decays to weak-undefined,
and it most likely goes unnoticed on systems that support undefweak.

If this were a final link, the bar symbol would be gone.  So would the
undef symbol, if the reference to it in t.s were uncommented.  But
because this is a relocatable link, both undefined symbols would remain.

Both of them are a problem for e.g. vxworks kernel modules, that never
undergo final linking, and whose loader rejects undefined symbols, even
weak ones, if they're not defined elsewhere.

I hoped --strip-discarded would get rid of undef, if not bar, but no
such luck.  Could we arrange for it to do so, even in relocatable
linking?  Or should we aim for another option to make -r (and -shared?)
linking behave more like final linking when it comes to discarding
unreferenced undefined symbols?

Thoughts?  Recommendations?

Thanks in advance,

-- 
Alexandre Oliva, happy hacker                    https://FSFLA.org/blogs/lxo/
   Free Software Activist                           GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: avoiding undesirable undef[weak] syms after relocatable linking

[Nick Clifton]

Hi Alex,

> And here's how to trigger the problem:
> 
> as t.s -o t.o && ar cr t.a t.o && ranlib t.a && as m.s -o m.o &&
> ld m.o -o m -r && nm m | grep bar
>                   w bar

Which version of the binutils are you using ?  And for which target ?

The reason I ask is that I tried the exact sequence you outlined but
for me the resulting 'm' file does not contain a symbol 'bar', weak
or otherwise:

   $ nm m
   0000000000000000 T _start
                    U baz
   0000000000000000 W foo

I ran these tests on an x86_64 host, running Fedora 39 with binutils
version 2.40 installed.  So probably one or more of these things is
a factor in the difference in behaviour.

Cheers
   Nick

Re: avoiding undesirable undef[weak] syms after relocatable linking

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

Hello, Nick,

On Apr 12, 2024, Nick Clifton <nickc@redhat.com> wrote:

> Hi Alex,
>> And here's how to trigger the problem:
>> as t.s -o t.o && ar cr t.a t.o && ranlib t.a && as m.s -o m.o &&
>> ld m.o -o m -r && nm m | grep bar
>> w bar

> Which version of the binutils are you using ?  And for which target ?

Originally, 2.42 snapshots on multiple vxworks targets, but I'd tried on
x86_64-linux-gnu as well.  Unfortunately, I see now that I made a
mistake in the command posted by email: somehow I dropped t.a from the
link command, presumably while editing it to remove local artifacts.
Please accept my apologies.  Here's the correct command, now (not)
adjusted, so that it runs from the top of a binutils build tree.  The
(unmodified) [tm].s files are attached.

$ gas/as-new t.s -o t.o && binutils/ar cr t.a t.o && binutils/ranlib t.a && gas/as-new m.s -o m.o && ld/ld-new m.o t.a -o m -r && binutils/nm-new m | grep bar
                 w bar
$ ld/ld-new --version
GNU ld (GNU Binutils) 2.42.50.20240413
[...]
$ git log
commit 4ad25f3bed6bc4c010962b11fde1c70ce8c22cae
[...]


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: t.s --]
[-- Type: text/x-assembly, Size: 213 bytes --]

.section .text.foobar,"axG",@progbits,foo,comdat
.weak foo
.type foo,@function
foo:
# .dc.a undef
.size foo, . - foo
.weak bar
.set bar, foo
.text
.global baz
.type baz,@function
baz:
.dc.a foo
.size baz, . - baz

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: m.s --]
[-- Type: text/x-assembly, Size: 150 bytes --]

.section .text.foobar,"axG",@progbits,foo,comdat
.weak foo
.type foo,@function
foo:
.size foo, . - foo
.text
.global _start
.set _start,foo
.dc.a baz

[-- Attachment #4: Type: text/plain, Size: 283 bytes --]



-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: avoiding undesirable undef[weak] syms after relocatable linking

[H.J. Lu]

On Sat, Apr 13, 2024 at 3:15 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> Hello, Nick,
>
> On Apr 12, 2024, Nick Clifton <nickc@redhat.com> wrote:
>
> > Hi Alex,
> >> And here's how to trigger the problem:
> >> as t.s -o t.o && ar cr t.a t.o && ranlib t.a && as m.s -o m.o &&
> >> ld m.o -o m -r && nm m | grep bar
> >> w bar
>
> > Which version of the binutils are you using ?  And for which target ?
>
> Originally, 2.42 snapshots on multiple vxworks targets, but I'd tried on
> x86_64-linux-gnu as well.  Unfortunately, I see now that I made a
> mistake in the command posted by email: somehow I dropped t.a from the
> link command, presumably while editing it to remove local artifacts.
> Please accept my apologies.  Here's the correct command, now (not)
> adjusted, so that it runs from the top of a binutils build tree.  The
> (unmodified) [tm].s files are attached.
>
> $ gas/as-new t.s -o t.o && binutils/ar cr t.a t.o && binutils/ranlib t.a && gas/as-new m.s -o m.o && ld/ld-new m.o t.a -o m -r && binutils/nm-new m | grep bar
>                  w bar
> $ ld/ld-new --version
> GNU ld (GNU Binutils) 2.42.50.20240413
> [...]
> $ git log
> commit 4ad25f3bed6bc4c010962b11fde1c70ce8c22cae
> [...]
>
>

Please open a bug report.

Thanks.

-- 
H.J.

Re: avoiding undesirable undef[weak] syms after relocatable linking

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]

On Sat, Apr 13, 2024 at 6:52 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Sat, Apr 13, 2024 at 3:15 AM Alexandre Oliva <oliva@adacore.com> wrote:
> >
> > Hello, Nick,
> >
> > On Apr 12, 2024, Nick Clifton <nickc@redhat.com> wrote:
> >
> > > Hi Alex,
> > >> And here's how to trigger the problem:
> > >> as t.s -o t.o && ar cr t.a t.o && ranlib t.a && as m.s -o m.o &&
> > >> ld m.o -o m -r && nm m | grep bar
> > >> w bar
> >
> > > Which version of the binutils are you using ?  And for which target ?
> >
> > Originally, 2.42 snapshots on multiple vxworks targets, but I'd tried on
> > x86_64-linux-gnu as well.  Unfortunately, I see now that I made a
> > mistake in the command posted by email: somehow I dropped t.a from the
> > link command, presumably while editing it to remove local artifacts.
> > Please accept my apologies.  Here's the correct command, now (not)
> > adjusted, so that it runs from the top of a binutils build tree.  The
> > (unmodified) [tm].s files are attached.
> >
> > $ gas/as-new t.s -o t.o && binutils/ar cr t.a t.o && binutils/ranlib t.a && gas/as-new m.s -o m.o && ld/ld-new m.o t.a -o m -r && binutils/nm-new m | grep bar
> >                  w bar
> > $ ld/ld-new --version
> > GNU ld (GNU Binutils) 2.42.50.20240413
> > [...]
> > $ git log
> > commit 4ad25f3bed6bc4c010962b11fde1c70ce8c22cae
> > [...]
> >
> >
>
> Please open a bug report.
>
> Thanks.
>

This should fix it.  I will submit a complete patch after the bug
report is opened.

-- 
H.J.

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 4904 bytes --]

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index ef5dcb55e72..92a0287d40e 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -232,6 +232,8 @@ struct elf_link_hash_entry
      a strong defined symbol alias.  U.ALIAS points to a list of aliases,
      the definition having is_weakalias clear.  */
   unsigned int is_weakalias : 1;
+  /* Symbol has a relocation.  */
+  unsigned int has_reloc : 1;
 
   /* String table index in .dynstr if this is a dynamic symbol.  */
   unsigned long dynstr_index;
diff --git a/bfd/elf-vxworks.c b/bfd/elf-vxworks.c
index 4c172cd4115..36e5540f9c0 100644
--- a/bfd/elf-vxworks.c
+++ b/bfd/elf-vxworks.c
@@ -172,35 +172,39 @@ elf_vxworks_emit_relocs (bfd *output_bfd,
 	   irela += bed->s->int_rels_per_ext_rel,
 	     hash_ptr++)
 	{
-	  if (*hash_ptr
-	      && (*hash_ptr)->def_dynamic
-	      && !(*hash_ptr)->def_regular
-	      && ((*hash_ptr)->root.type == bfd_link_hash_defined
-		  || (*hash_ptr)->root.type == bfd_link_hash_defweak)
-	      && (*hash_ptr)->root.u.def.section->output_section != NULL)
+	  if (*hash_ptr)
 	    {
-	      /* This is a relocation from an executable or shared
-		 library against a symbol in a different shared
-		 library.  We are creating a definition in the output
-		 file but it does not come from any of our normal (.o)
-		 files. ie. a PLT stub.  Normally this would be a
-		 relocation against against SHN_UNDEF with the VMA of
-		 the PLT stub.  This upsets the VxWorks loader.
-		 Convert it to a section-relative relocation.  This
-		 gets some other symbols (for instance .dynbss), but
-		 is conservatively correct.  */
-	      for (j = 0; j < bed->s->int_rels_per_ext_rel; j++)
+	      (*hash_ptr)->has_reloc = 1;
+	      if ((*hash_ptr)->def_dynamic
+		  && !(*hash_ptr)->def_regular
+		  && ((*hash_ptr)->root.type == bfd_link_hash_defined
+		      || (*hash_ptr)->root.type == bfd_link_hash_defweak)
+		  && (*hash_ptr)->root.u.def.section->output_section != NULL)
 		{
-		  asection *sec = (*hash_ptr)->root.u.def.section;
-		  int this_idx = sec->output_section->target_index;
-
-		  irela[j].r_info
-		    = ELF32_R_INFO (this_idx, ELF32_R_TYPE (irela[j].r_info));
-		  irela[j].r_addend += (*hash_ptr)->root.u.def.value;
-		  irela[j].r_addend += sec->output_offset;
+		  /* This is a relocation from an executable or shared
+		     library against a symbol in a different shared
+		     library.  We are creating a definition in the output
+		     file but it does not come from any of our normal (.o)
+		     files. ie. a PLT stub.  Normally this would be a
+		     relocation against against SHN_UNDEF with the VMA of
+		     the PLT stub.  This upsets the VxWorks loader.
+		     Convert it to a section-relative relocation.  This
+		     gets some other symbols (for instance .dynbss), but
+		     is conservatively correct.  */
+		  for (j = 0; j < bed->s->int_rels_per_ext_rel; j++)
+		    {
+		      asection *sec = (*hash_ptr)->root.u.def.section;
+		      int this_idx = sec->output_section->target_index;
+
+		      irela[j].r_info
+			= ELF32_R_INFO (this_idx,
+					ELF32_R_TYPE (irela[j].r_info));
+		      irela[j].r_addend += (*hash_ptr)->root.u.def.value;
+		      irela[j].r_addend += sec->output_offset;
+		    }
+		  /* Stop the generic routine adjusting this entry.  */
+		  *hash_ptr = NULL;
 		}
-	      /* Stop the generic routine adjusting this entry.  */
-	      *hash_ptr = NULL;
 	    }
 	}
     }
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 321e3d5e2ff..6558de82fa2 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -2942,8 +2942,7 @@ _bfd_elf_link_output_relocs (bfd *output_bfd,
 			     asection *input_section,
 			     Elf_Internal_Shdr *input_rel_hdr,
 			     Elf_Internal_Rela *internal_relocs,
-			     struct elf_link_hash_entry **rel_hash
-			       ATTRIBUTE_UNUSED)
+			     struct elf_link_hash_entry **rel_hash)
 {
   Elf_Internal_Rela *irela;
   Elf_Internal_Rela *irelaend;
@@ -2986,9 +2985,12 @@ _bfd_elf_link_output_relocs (bfd *output_bfd,
 		      * bed->s->int_rels_per_ext_rel);
   while (irela < irelaend)
     {
+      if (*rel_hash)
+	(*rel_hash)->has_reloc = 1;
       (*swap_out) (output_bfd, irela, erel);
       irela += bed->s->int_rels_per_ext_rel;
       erel += input_rel_hdr->sh_entsize;
+      rel_hash++;
     }
 
   /* Bump the counter, so that we know where to add the next set of
@@ -10737,8 +10739,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   /* We don't want to output symbols that have never been mentioned by
      a regular file, or that we have been told to strip.  However, if
      h->indx is set to -2, the symbol is used by a reloc and we must
-     output it.  */
-  strip = false;
+     output it.  Always strip undefined weak symbols if they don't
+     have relocation.  */
+  strip = h->root.type == bfd_link_hash_undefweak && !h->has_reloc;
   if (h->indx == -2)
     ;
   else if ((h->def_dynamic

Re: avoiding undesirable undef[weak] syms after relocatable linking

[Alexandre Oliva]

Hello, H.J.,

On Apr 13, 2024, "H.J. Lu" <hjl.tools@gmail.com> wrote:

>> $ gas/as-new t.s -o t.o && binutils/ar cr t.a t.o && binutils/ranlib
>> t.a && gas/as-new m.s -o m.o && ld/ld-new m.o t.a -o m -r &&
>> binutils/nm-new m | grep bar
>> w bar
>> $ ld/ld-new --version
>> GNU ld (GNU Binutils) 2.42.50.20240413
>> [...]
>> $ git log
>> commit 4ad25f3bed6bc4c010962b11fde1c70ce8c22cae
>> [...]

> Please open a bug report.

Can do (and thanks for the patch), but...  Are you positive that that's
a bug to begin with?

I am concerned that changing it might break something, though I can't
quite put my finger on what it is that might break with this change.

Say, given another object file, say x.o, to be final-linked with the
result of the relocatable link that this patch would change, couldn't a
symbol that resolved to the undefweak become a final link failure after
the change?  Or if the relocatable link output was made part of a
dynamic library, couldn't something break in case the undefweak symbol
was meant to be dynamically exported, but would now be entirely gone?

That's why I was thinking of controlling this behavior change with an
option.

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: avoiding undesirable undef[weak] syms after relocatable linking

[H.J. Lu]

On Mon, Apr 15, 2024 at 11:00 PM Alexandre Oliva <oliva@adacore.com> wrote:
>
> Hello, H.J.,
>
> On Apr 13, 2024, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>
> >> $ gas/as-new t.s -o t.o && binutils/ar cr t.a t.o && binutils/ranlib
> >> t.a && gas/as-new m.s -o m.o && ld/ld-new m.o t.a -o m -r &&
> >> binutils/nm-new m | grep bar
> >> w bar
> >> $ ld/ld-new --version
> >> GNU ld (GNU Binutils) 2.42.50.20240413
> >> [...]
> >> $ git log
> >> commit 4ad25f3bed6bc4c010962b11fde1c70ce8c22cae
> >> [...]
>
> > Please open a bug report.
>
> Can do (and thanks for the patch), but...  Are you positive that that's
> a bug to begin with?

Since there are no relocations against the undefined weak symbol,
it is not necessary to have it in the symbol table.

> I am concerned that changing it might break something, though I can't
> quite put my finger on what it is that might break with this change.
>
> Say, given another object file, say x.o, to be final-linked with the
> result of the relocatable link that this patch would change, couldn't a
> symbol that resolved to the undefweak become a final link failure after
> the change?  Or if the relocatable link output was made part of a
> dynamic library, couldn't something break in case the undefweak symbol
> was meant to be dynamically exported, but would now be entirely gone?

Since there is no relocation against it, it should be OK.

> That's why I was thinking of controlling this behavior change with an
> option.
>
> --
> Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
>    Free Software Activist                   GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive



-- 
H.J.

Re: avoiding undesirable undef[weak] syms after relocatable linking

[Alexandre Oliva]

On Apr 16, 2024, "H.J. Lu" <hjl.tools@gmail.com> wrote:

> Since there are no relocations against the undefined weak symbol,
> it is not necessary to have it in the symbol table.

> Since there is no relocation against it, it should be OK.

Thanks for the reassurance.

Filed: https://sourceware.org/bugzilla/show_bug.cgi?id=31652

-- 
Alexandre Oliva, happy hacker                    https://FSFLA.org/blogs/lxo/
   Free Software Activist                           GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice but
very few check the facts.  Think Assange & Stallman.  The empires strike back