[PATCH v3] LTO: Restore the wrapper symbol check for standard function
H.J. Lu
hjl.tools@gmail.com
Sat Aug 3 02:53:43 GMT 2024
On Fri, Aug 2, 2024 at 7:41 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Aug 2, 2024 at 7:24 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Fri, Aug 2, 2024 at 7:01 PM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Fri, Aug 02, 2024 at 10:19:41AM -0700, H.J. Lu wrote:
> > > > Call unwrap_hash_lookup to restore the wrapper symbol check for standard
> > > > function since reference to standard function may not show up in LTO
> > > > symbol table:
> > > >
> > > > [hjl@gnu-tgl-3 pr31956-3]$ nm foo.o
> > > > 00000000 T main
> > > > U __real_malloc
> > > > 00000000 T __wrap_malloc
> > > > [hjl@gnu-tgl-3 pr31956-3]$ lto-dump -list foo.o
> > > > Type Visibility Size Name
> > > > function default 0 malloc
> > > > function default 0 __real_malloc
> > > > function default 3 main
> > > > function default 5 __wrap_malloc
> > > > [hjl@gnu-tgl-3 pr31956-3]$ make
> > > > gcc -O2 -flto -Wall -c -o foo.o foo.c
> > > > gcc -Wl,--wrap=malloc -O2 -flto -Wall -o x foo.o
> > > > /usr/local/bin/ld: /tmp/ccsPW0a9.ltrans0.ltrans.o: in function `main':
> > > > <artificial>:(.text.startup+0xa): undefined reference to `__wrap_malloc'
> > > > collect2: error: ld returned 1 exit status
> > > > make: *** [Makefile:22: x] Error 1
> > > > [hjl@gnu-tgl-3 pr31956-3]$
> > > >
> > > > PR ld/31956
> > > > * plugin.c (get_symbols): Restore the wrapper symbol check for
> > > > standard function.
> > > > * testsuite/ld-plugin/lto.exp: Run the malloc test.
> > > > * testsuite/ld-plugin/pr31956c.c: New file.
> > >
> > > Thanks for digging into this. Given that the addition of
> > > wrapper_symbol to bfd_link_hash_entry was ineffective, I'm inclined to
> > > revert your previous patch (except for the testcase), and then apply
> > > https://sourceware.org/pipermail/binutils/2024-July/135541.html plus
> > > your new testcase.
> > >
> > > As follows. Please check that my analysis of the problem is correct.
> > > I'm fairly certain it is, but as we know LTO is complicated.
> > >
> > > Nick, I think this should go on the 2.43 branch too, and is better
> > > than reverting eb7892c401 there, (which in turn is better than leaving
> > > eb7892c401 on the branch) but I'll leave that to you to decide.
> > > ----
> > > Subject: Re: LTO: Properly check wrapper symbol
> > >
> > > Prior to commit eb7892c401, when __wrap_foo was in the linker hash
> > > table (link_info.hash) but not foo, symbol resolution detemined by
> > > ld/plugin.c:get_symbols was wrong for __wrap_foo. Commit eb7892c401
> > > fixed this when there was a reference to __wrap_foo, but broke another
> > > case, when there was a definition for __wrap_foo but no reference to
> > > __wrap_foo. See https://bugzilla.redhat.com/show_bug.cgi?id=2301454
> > > and the testcase added here.
> > >
> > > In get_symbols when deciding "wrap_status" should be "wrapper" for a
> > > symbol __wrap_foo, I believe the proper test is that the symbol exists
> > > and foo appears on a command line --wrap=foo option (ie. is in
> > > link_info.wrap_hash). It is irrelevant whether foo appears anywhere
> > > in IR symbols. This can be accomplished by calling unwrap_hash_lookup
> > > and testing for any return different to the original symbol.
> > >
> > > PR 31956
> > > include/
> > > * bfdlink.h (struct bfd_link_hash_entry): Delete wrapper_symbol.
> > > bfd/
> > > * linker.c (bfd_wrapped_link_hash_lookup): Revert eb7892c401.
> > > ld/
> > > * plugin.c (get_symbols): Revert eb7892c401. Instead call
> > > unwrap_hash_lookup as before but don't check unwrap non-NULL.
> > > * testsuite/ld-plugin/lto.exp: Run the malloc test.
> > > * testsuite/ld-plugin/pr31956c.c: New file.
> > >
> > > Testcase courtesy of H.J. Lu.
> > >
> > > diff --git a/bfd/linker.c b/bfd/linker.c
> > > index 21009a838bc..111deecf55d 100644
> > > --- a/bfd/linker.c
> > > +++ b/bfd/linker.c
> > > @@ -573,8 +573,6 @@ bfd_wrapped_link_hash_lookup (bfd *abfd,
> > > strcat (n, WRAP);
> > > strcat (n, l);
> > > h = bfd_link_hash_lookup (info->hash, n, create, true, follow);
> > > - if (h != NULL)
> > > - h->wrapper_symbol = true;
> > > free (n);
> > > return h;
> > > }
> > > diff --git a/include/bfdlink.h b/include/bfdlink.h
> > > index f802ec627ef..015370d268f 100644
> > > --- a/include/bfdlink.h
> > > +++ b/include/bfdlink.h
> > > @@ -117,9 +117,6 @@ struct bfd_link_hash_entry
> > > /* The symbol, SYM, is referenced by __real_SYM in an object file. */
> > > unsigned int ref_real : 1;
> > >
> > > - /* The symbol is a wrapper symbol, __wrap_SYM. */
> > > - unsigned int wrapper_symbol : 1;
> > > -
> > > /* Symbol is a built-in define. These will be overridden by PROVIDE
> > > in a linker script. */
> > > unsigned int linker_def : 1;
> > > diff --git a/ld/plugin.c b/ld/plugin.c
> > > index 03ee9880d10..24ffb58741a 100644
> > > --- a/ld/plugin.c
> > > +++ b/ld/plugin.c
> > > @@ -778,8 +778,13 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms,
> > > {
> > > blhe = h;
> > > /* Check if a symbol is a wrapper symbol. */
> > > - if (blhe && blhe->wrapper_symbol)
> > > - wrap_status = wrapper;
> > > + if (blhe && link_info.wrap_hash != NULL)
> > > + {
> > > + struct bfd_link_hash_entry *unwrap
> > > + = unwrap_hash_lookup (&link_info, (bfd *) abfd, blhe);
> > > + if (unwrap != h)
> > > + wrap_status = wrapper;
> >
> > Will LTO remove __wrap_XXX when XXX isn't referenced? If not,
> > this will be a regression.
>
> With my patch, I got
>
> [hjl@gnu-tgl-3 pr31956-4]$ cat x.c
> void __wrap_parse_line(void) {};
>
> int
> main (void)
> {
> return 0;
> }
> [hjl@gnu-tgl-3 pr31956-4]$ make
> gcc -B./ -O2 -Wall -flto -c -o x.o x.c
> gcc -B./ -Wl,--wrap=parse_line -O2 -Wall -flto -o x x.o
> nm x | grep parse_line
> make: *** [Makefile:16: all] Error 1
> [hjl@gnu-tgl-3 pr31956-4]$
>
> I think your patch will keep __wrap_parse_line
I sent v4 with a testcase to verify that the unused wrapper is removed.
> >
> > > + }
> > > }
> > > else
> > > {
> > > diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
> > > index ad59e2abd61..a37a4e61bdc 100644
> > > --- a/ld/testsuite/ld-plugin/lto.exp
> > > +++ b/ld/testsuite/ld-plugin/lto.exp
> > > @@ -546,6 +546,14 @@ set lto_link_elf_tests [list \
> > > {} \
> > > "pr31956b" \
> > > ] \
> > > + [list \
> > > + "PR ld/31956 (malloc)" \
> > > + "-Wl,--wrap=malloc" \
> > > + "-O2 -flto" \
> > > + {pr31956c.c} \
> > > + {} \
> > > + "pr31956c" \
> > > + ] \
> > > [list \
> > > "Build pr30281.so" \
> > > "-shared -Wl,--version-script,pr30281.t \
> > > diff --git a/ld/testsuite/ld-plugin/pr31956c.c b/ld/testsuite/ld-plugin/pr31956c.c
> > > new file mode 100644
> > > index 00000000000..4a46b2b49a8
> > > --- /dev/null
> > > +++ b/ld/testsuite/ld-plugin/pr31956c.c
> > > @@ -0,0 +1,19 @@
> > > +#include <stdlib.h>
> > > +
> > > +extern void *__real_malloc (size_t);
> > > +
> > > +void *
> > > +__wrap_malloc (size_t n)
> > > +{
> > > + if (n == 0)
> > > + return NULL;
> > > + else
> > > + return __real_malloc (n);
> > > +};
> > > +
> > > +int
> > > +main (void)
> > > +{
> > > + void *ptr = malloc (30);
> > > + return ptr == NULL ? 1 : 0;
> > > +}
> > >
> > > --
> > > Alan Modra
> >
> >
> >
> > --
> > H.J.
>
>
>
> --
> H.J.
--
H.J.
More information about the Binutils
mailing list