[v3] LTO: Restore the wrapper symbol check for standard function
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
Commit Message
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.
Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
ld/plugin.c | 14 ++++++++++++--
ld/testsuite/ld-plugin/lto.exp | 8 ++++++++
ld/testsuite/ld-plugin/pr31956c.c | 19 +++++++++++++++++++
3 files changed, 39 insertions(+), 2 deletions(-)
create mode 100644 ld/testsuite/ld-plugin/pr31956c.c
Comments
On Fri, Aug 2, 2024 at 10:19 AM H.J. Lu <hjl.tools@gmail.com> 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.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> ---
> ld/plugin.c | 14 ++++++++++++--
> ld/testsuite/ld-plugin/lto.exp | 8 ++++++++
> ld/testsuite/ld-plugin/pr31956c.c | 19 +++++++++++++++++++
> 3 files changed, 39 insertions(+), 2 deletions(-)
> create mode 100644 ld/testsuite/ld-plugin/pr31956c.c
>
> diff --git a/ld/plugin.c b/ld/plugin.c
> index 03ee9880d10..51c4765cc5b 100644
> --- a/ld/plugin.c
> +++ b/ld/plugin.c
> @@ -778,8 +778,18 @@ 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)
> + {
> + if (blhe->wrapper_symbol)
> + wrap_status = wrapper;
> + else if (link_info.wrap_hash != NULL)
> + {
> + struct bfd_link_hash_entry *unwrap
> + = unwrap_hash_lookup (&link_info, (bfd *) abfd, blhe);
> + if (unwrap != NULL && unwrap != h)
> + wrap_status = wrapper;
> + }
> + }
> }
> 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;
> +}
> --
> 2.45.2
>
Please commit it for me if it is correct.
Thanks.
"H.J. Lu" <hjl.tools@gmail.com> writes:
> 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:
I'll test it, thanks. I was planning to look at a testcase at the end of
the coming week as you were away but you got there first.
I can't review it though, I'm not comfortable enough with the linker
yet. Alan?
>
> [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.
>
> Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
> ---
> ld/plugin.c | 14 ++++++++++++--
> ld/testsuite/ld-plugin/lto.exp | 8 ++++++++
> ld/testsuite/ld-plugin/pr31956c.c | 19 +++++++++++++++++++
> 3 files changed, 39 insertions(+), 2 deletions(-)
> create mode 100644 ld/testsuite/ld-plugin/pr31956c.c
>
> diff --git a/ld/plugin.c b/ld/plugin.c
> index 03ee9880d10..51c4765cc5b 100644
> --- a/ld/plugin.c
> +++ b/ld/plugin.c
> @@ -778,8 +778,18 @@ 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)
> + {
> + if (blhe->wrapper_symbol)
> + wrap_status = wrapper;
> + else if (link_info.wrap_hash != NULL)
> + {
> + struct bfd_link_hash_entry *unwrap
> + = unwrap_hash_lookup (&link_info, (bfd *) abfd, blhe);
> + if (unwrap != NULL && unwrap != h)
> + wrap_status = wrapper;
> + }
> + }
> }
> 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;
> +}
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;
+ }
}
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;
+}
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.
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
>
> > + }
> > }
> > 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.
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.
On Fri, Aug 02, 2024 at 07:24:24PM -0700, H.J. Lu wrote:
> On Fri, Aug 2, 2024 at 7:01 PM Alan Modra <amodra@gmail.com> wrote:
> > - 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,
Likely not.
> this will be a regression.
I can't say I'm greatly concerned about a code size regression when
LTO and --wrap intersect, but yes, you have identified a potential
defect in my simpler pr3195c patch. Hmm. If I modify your pr31956c.c
testcase to make main just return 0, ie. not call malloc, and make
__wrap_malloc hidden then ld 2.42 leaves __wrap_malloc defined..
I'd like to find a testcase where there is a code size regression,
but really can't spend time today on that.
On Sat, Aug 3, 2024, 6:31 AM Alan Modra <amodra@gmail.com> wrote:
> On Fri, Aug 02, 2024 at 07:24:24PM -0700, H.J. Lu wrote:
> > On Fri, Aug 2, 2024 at 7:01 PM Alan Modra <amodra@gmail.com> wrote:
> > > - 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,
>
> Likely not.
>
> > this will be a regression.
>
> I can't say I'm greatly concerned about a code size regression when
> LTO and --wrap intersect, but yes, you have identified a potential
> defect in my simpler pr3195c patch. Hmm. If I modify your pr31956c.c
> testcase to make main just return 0, ie. not call malloc, and make
> __wrap_malloc hidden then ld 2.42 leaves __wrap_malloc defined..
> I'd like to find a testcase where there is a code size regression,
>
My v4 has such a test. I think your patch
may keep all unsed functions , not just
wrappers, when wrap us used.
but really can't spend time today on that.
>
> --
> Alan Modra
>
On Sat, Aug 3, 2024, 6:31 AM Alan Modra <amodra@gmail.com> wrote:
> On Fri, Aug 02, 2024 at 07:24:24PM -0700, H.J. Lu wrote:
> > On Fri, Aug 2, 2024 at 7:01 PM Alan Modra <amodra@gmail.com> wrote:
> > > - 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,
>
> Likely not.
>
> > this will be a regression.
>
> I can't say I'm greatly concerned about a code size regression when
> LTO and --wrap intersect, but yes, you have identified a potential
> defect in my simpler pr3195c patch. Hmm. If I modify your pr31956c.c
> testcase to make main just return 0, ie. not call malloc, and make
> __wrap_malloc hidden then ld 2.42 leaves __wrap_malloc defined..
>
It is because malloc is in libc.so.
I'd like to find a testcase where there is a code size regression,
> but really can't spend time today on that.
>
> --
> Alan Modra
>
>
On Fri, Aug 02, 2024 at 07:53:43PM -0700, H.J. Lu wrote:
> > I think your patch will keep __wrap_parse_line
It does.
> I sent v4 with a testcase to verify that the unused wrapper is removed.
OK, you convinced me. Please apply mainline, and leave it to Nick to
apply to the branch.
On Sat, Aug 3, 2024, 6:38 AM Alan Modra <amodra@gmail.com> wrote:
> On Fri, Aug 02, 2024 at 07:53:43PM -0700, H.J. Lu wrote:
> > > I think your patch will keep __wrap_parse_line
>
> It does.
>
> > I sent v4 with a testcase to verify that the unused wrapper is removed.
>
> OK, you convinced me. Please apply mainline, and leave it to Nick to
>
Can you apply the v4 patch for me? Thanks.
apply to the branch.
>
> --
> Alan Modra
>
>
@@ -778,8 +778,18 @@ 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)
+ {
+ if (blhe->wrapper_symbol)
+ wrap_status = wrapper;
+ else if (link_info.wrap_hash != NULL)
+ {
+ struct bfd_link_hash_entry *unwrap
+ = unwrap_hash_lookup (&link_info, (bfd *) abfd, blhe);
+ if (unwrap != NULL && unwrap != h)
+ wrap_status = wrapper;
+ }
+ }
}
else
{
@@ -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 \
new file mode 100644
@@ -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;
+}