[PATCH v3] LTO: Restore the wrapper symbol check for standard function

[Alan Modra]

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;
+}

-- 
Alan Modra