[PATCH v3] LTO: Restore the wrapper symbol check for standard function
H.J. Lu
hjl.tools@gmail.com
Sat Aug 3 02:24:24 GMT 2024
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.
> + }
> }
> 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.
More information about the Binutils
mailing list