[PATCHx3] LD plugins: matters outstanding.

[PATCHx3] LD plugins: matters outstanding.

[Dave Korn]

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


    Hello binutils,

  The attached three patches fix two small bugs in the LD plugin interface,
and add support for MinGW.

[PATCH] Avoid testsuite errors when no cross compiler is available.

ld/testsuite/ChangeLog:

	* ld-plugin/plugin.exp: Don't error out if there is no target compiler
	available, make tests UNSUPPORTED instead.

  This is just a repost of the patch I sent earlier to prevent noise in
cross-target testsuite runs.

[PATCH] Fix potential use-after-free bugs.

ld/ChangeLog:

	* plugin.c (add_input_file): Take copy of input string.
	(add_input_library): Likewise.
	(set_extra_library_path): Likewise.

  This patch fixes a bug I found while porting the GCC lto-plugin.  The
strings passed to these callbacks belong to the plugin, and it is allowed to
free them after the callback returns.  So ld can't hang on to a copy of a
pointer to the string (embedded in a lang input statement); it has to make a
dup.  Without this fix, plugin pass-through filenames were getting mangled.

[PATCH] Provide win32-based dlapi replacements on windows platforms without
dlfcn.h.

ld/ChangeLog:

	* configure.in: If <dlfcn.h> can't be found, try for <Windows.h>
	* configure: Regenerate.
	* config.in: Likewise.
	* plugin.c [!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlopen): Provide
	trival LoadLibrary-based replacement for Windows systems.
	[!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlsym): Likewise trivial
	replacement based on GetProcAddress.
	[!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlsym): Likewise FreeLibrary.
	* sysdep.h: Don't infer presence of <dlfcn.h> from ENABLE_PLUGINS
	anymore, use its own guard.

  I took a look at libltdl, and decided it was too much of a top-heavy and
intrusive solution.  Given that so far it's looking like Windows is going to
be the only non-ELF platform fully supporting LTO, I decided just to slap a
trivial shim layer in for MinGW's sake.

  Built and tested on i686-pc-cygwin.  Built on i686-pc-mingw32, but there's
no dejagnu on that platform, so I just manually ran the equivalent of the most
basic plugin load test.  (It worked.)

  These patches complete the LD plugin interface work.  (Tristan, the patch I
thought I might need to do about rescanning libs turns out not to be needed,
so this is everything.)

  OK for trunk?

    cheers,
      DaveK


[-- Attachment #2: ld-plugin-fixes-1-tests-fix.diff --]
[-- Type: text/x-c, Size: 2090 bytes --]

From babff63d4d6d96ab9fae22b67a729445d767d459 Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Fri, 15 Oct 2010 16:05:51 +0100
Subject: [PATCH] Avoid testsuite errors when no cross compiler is available.

ld/testsuite/ChangeLog:

	* ld-plugin/plugin.exp: Don't error out if there is no target compiler
	available, make tests UNSUPPORTED instead.
---
 ld/testsuite/ld-plugin/plugin.exp |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/ld/testsuite/ld-plugin/plugin.exp b/ld/testsuite/ld-plugin/plugin.exp
index 796cb0e..416159a 100644
--- a/ld/testsuite/ld-plugin/plugin.exp
+++ b/ld/testsuite/ld-plugin/plugin.exp
@@ -24,6 +24,13 @@ if ![check_plugin_api_available] {
     return
 }
 
+# And a compiler to be available.
+set can_compile 1
+if { [which $CC] == 0 } {
+  # Don't fail immediately, 
+  set can_compile 0
+}
+
 pass "plugin API enabled"
 
 global base_dir
@@ -62,12 +69,15 @@ set regcln "-plugin-opt registercleanup"
 set failed_compile 0
 set _ ""
 set plugin_nm_output ""
-if { ![ld_compile "$CC $CFLAGS" $srcdir/$subdir/main.c tmpdir/main.o]
-	|| ![ld_compile "$CC $CFLAGS" $srcdir/$subdir/func.c tmpdir/func.o]
-	|| ![ld_compile "$CC $CFLAGS" $srcdir/$subdir/text.c tmpdir/text.o] } {
+if { $can_compile && \
+	(![ld_compile "$CC $CFLAGS" $srcdir/$subdir/main.c tmpdir/main.o] \
+	|| ![ld_compile "$CC $CFLAGS" $srcdir/$subdir/func.c tmpdir/func.o] \
+	|| ![ld_compile "$CC $CFLAGS" $srcdir/$subdir/text.c tmpdir/text.o]) } {
     # Defer fail until we have list of tests set.
     set failed_compile 1
-} else {
+}
+
+if { $can_compile && !$failed_compile } {
     # Find out if symbols have prefix on this platform before setting tests.
     catch "exec $NM tmpdir/func.o" plugin_nm_output
     if { [regexp "_func" "$plugin_nm_output"] } {
@@ -142,7 +152,7 @@ set plugin_extra_elf_tests [list \
 				{readelf -s plugin-vis-1.d}} "main.x" ] \
 ]
 
-if { $failed_compile != 0 } {
+if { !$can_compile || $failed_compile } {
     foreach testitem $plugin_tests {
 	unresolved [lindex $testitem 0]
     }

[-- Attachment #3: ld-plugin-fixes-2-strdup-fix.diff --]
[-- Type: text/x-c, Size: 1426 bytes --]

From 1478b11d6ed70bde869ad90d52a6194abd92d88d Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Fri, 15 Oct 2010 16:09:56 +0100
Subject: [PATCH] Fix potential use-after-free bugs.

ld/ChangeLog:

	* plugin.c (add_input_file): Take copy of input string.
	(add_input_library): Likewise.
	(set_extra_library_path): Likewise.
---
 ld/plugin.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/ld/plugin.c b/ld/plugin.c
index b484bd0..0c88ef8 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -433,7 +433,8 @@ static enum ld_plugin_status
 add_input_file (const char *pathname)
 {
   ASSERT (called_plugin);
-  if (!lang_add_input_file (pathname, lang_input_file_is_file_enum, NULL))
+  if (!lang_add_input_file (xstrdup (pathname), lang_input_file_is_file_enum,
+	NULL))
     return LDPS_ERR;
   return LDPS_OK;
 }
@@ -443,7 +444,8 @@ static enum ld_plugin_status
 add_input_library (const char *pathname)
 {
   ASSERT (called_plugin);
-  if (!lang_add_input_file (pathname, lang_input_file_is_l_enum, NULL))
+  if (!lang_add_input_file (xstrdup (pathname), lang_input_file_is_l_enum,
+	NULL))
     return LDPS_ERR;
   return LDPS_OK;
 }
@@ -454,7 +456,7 @@ static enum ld_plugin_status
 set_extra_library_path (const char *path)
 {
   ASSERT (called_plugin);
-  ldfile_add_library_path (path, FALSE);
+  ldfile_add_library_path (xstrdup (path), FALSE);
   return LDPS_OK;
 }
 

[-- Attachment #4: ld-plugin-fixes-3-mingw-loadlib.diff --]
[-- Type: text/x-c, Size: 4124 bytes --]

From 9b6758820e42a6b6ea97226f860a22dfb4b653e6 Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Fri, 15 Oct 2010 16:34:37 +0100
Subject: [PATCH] Provide win32-based dlapi replacements on windows platforms without dlfcn.h.

ld/ChangeLog:

	* configure.in: If <dlfcn.h> can't be found, try for <Windows.h>
	* configure: Regenerate.
	* config.in: Likewise.
	* plugin.c [!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlopen): Provide
	trival LoadLibrary-based replacement for Windows systems.
	[!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlsym): Likewise trivial
	replacement based on GetProcAddress.
	[!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlsym): Likewise FreeLibrary.
	* sysdep.h: Don't infer presence of <dlfcn.h> from ENABLE_PLUGINS
	anymore, use its own guard.
---
 ld/config.in    |    3 +++
 ld/configure    |   16 ++++++++++++++++
 ld/configure.in |    4 ++++
 ld/plugin.c     |   28 ++++++++++++++++++++++++++++
 ld/sysdep.h     |    3 +--
 5 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/ld/config.in b/ld/config.in
index 9e6e97b..f49327c 100644
--- a/ld/config.in
+++ b/ld/config.in
@@ -129,6 +129,9 @@
 /* Define to 1 if you have the `waitpid' function. */
 #undef HAVE_WAITPID
 
+/* Define to 1 if you have the <Windows.h> header file. */
+#undef HAVE_WINDOWS_H
+
 /* Define to 1 if you have the <zlib.h> header file. */
 #undef HAVE_ZLIB_H
 
diff --git a/ld/configure b/ld/configure
index 9ff8529..3367a88 100755
--- a/ld/configure
+++ b/ld/configure
@@ -12919,6 +12919,22 @@ else
 fi
 done
 
+# We also support plugins on Windows (MinGW).
+if test x$enable_plugins = xno ; then
+  for ac_header in Windows.h
+do :
+  ac_fn_c_check_header_compile "$LINENO" "Windows.h" "ac_cv_header_Windows_h" "$ac_includes_default
+"
+if test "x$ac_cv_header_Windows_h" = x""yes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_WINDOWS_H 1
+_ACEOF
+ enable_plugins=yes
+fi
+
+done
+
+fi
  if test x$enable_plugins = xyes; then
   ENABLE_PLUGINS_TRUE=
   ENABLE_PLUGINS_FALSE='#'
diff --git a/ld/configure.in b/ld/configure.in
index 29d01cc..122f65e 100644
--- a/ld/configure.in
+++ b/ld/configure.in
@@ -169,6 +169,10 @@ enable_plugins=yes
 AC_CHECK_HEADER([dlfcn.h],[],[enable_plugins=no],[AC_INCLUDES_DEFAULT])
 AC_SEARCH_LIBS([dlopen],[dl],[],[enable_plugins=no],[])
 AC_CHECK_FUNCS([dlopen dlsym dlclose],[],[enable_plugins=no])
+# We also support plugins on Windows (MinGW).
+if test x$enable_plugins = xno ; then
+  AC_CHECK_HEADERS([Windows.h],[enable_plugins=yes],[],[AC_INCLUDES_DEFAULT])
+fi
 AM_CONDITIONAL([ENABLE_PLUGINS], [test x$enable_plugins = xyes])
 
 AC_MSG_CHECKING(for a known getopt prototype in unistd.h)
diff --git a/ld/plugin.c b/ld/plugin.c
index 0c88ef8..caf8a34 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -32,6 +32,9 @@
 #include "plugin.h"
 #include "plugin-api.h"
 #include "elf-bfd.h"
+#if defined (HAVE_WINDOWS_H)
+#include <Windows.h>
+#endif
 
 /* The suffix to append to the name of the real (claimed) object file
    when generating a dummy BFD to hold the IR symbols sent from the
@@ -128,6 +131,31 @@ static const enum ld_plugin_tag tv_header_tags[] =
 /* How many entries in the constant leading part of the tv array.  */
 static const size_t tv_header_size = ARRAY_SIZE (tv_header_tags);
 
+#if !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)
+
+#define RTLD_NOW 0	/* Dummy value.  */
+
+static void *
+dlopen (const char *file, int mode ATTRIBUTE_UNUSED)
+{
+  return LoadLibrary (file);
+}
+
+static void *
+dlsym (void *handle, const char *name)
+{
+  return GetProcAddress (handle, name);
+}
+
+static int
+dlclose (void *handle)
+{
+  FreeLibrary (handle);
+  return 0;
+}
+
+#endif /* !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)  */
+
 /* Helper function for exiting with error status.  */
 static int
 set_plugin_error (const char *plugin)
diff --git a/ld/sysdep.h b/ld/sysdep.h
index 9dfae10..b7d5b88 100644
--- a/ld/sysdep.h
+++ b/ld/sysdep.h
@@ -90,8 +90,7 @@ extern char *strrchr ();
 #endif
 #endif
 
-/* This is both more precise than and includes HAVE_DLFCN_H.  */
-#ifdef ENABLE_PLUGINS
+#ifdef HAVE_DLFCN_H
 #include <dlfcn.h>
 #endif
 

Re: [PATCHx3] LD plugins: matters outstanding.

[Richard Henderson]

On 10/15/2010 08:56 AM, Dave Korn wrote:
> 	* ld-plugin/plugin.exp: Don't error out if there is no target compiler
> 	available, make tests UNSUPPORTED instead.

Ok.

> 	* plugin.c (add_input_file): Take copy of input string.
> 	(add_input_library): Likewise.
> 	(set_extra_library_path): Likewise.

Ok.

> 	* configure.in: If <dlfcn.h> can't be found, try for <Windows.h>
> 	* configure: Regenerate.
> 	* config.in: Likewise.
> 	* plugin.c [!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlopen): Provide
> 	trival LoadLibrary-based replacement for Windows systems.
> 	[!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlsym): Likewise trivial
> 	replacement based on GetProcAddress.
> 	[!HAVE_DLFCN_H && HAVE_WINDOWS_H] (dlsym): Likewise FreeLibrary.
> 	* sysdep.h: Don't infer presence of <dlfcn.h> from ENABLE_PLUGINS
> 	anymore, use its own guard.

I think it's slightly confusing that HAVE_WINDOWS_H is defined only if
HAVE_DLFCN_H isn't.  I think it might be a tad better to also test
HAVE_DLFCN_H in the ifdef around the include.

Ok with that change.


r~

Re: [PATCHx3] LD plugins: matters outstanding.

[Dave Korn]

On 15/10/2010 16:52, Richard Henderson wrote:

> I think it's slightly confusing that HAVE_WINDOWS_H is defined only if
> HAVE_DLFCN_H isn't.  I think it might be a tad better to also test
> HAVE_DLFCN_H in the ifdef around the include.

    Just checking: you mean like so, right?

> #if !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)
> #include <Windows.h>
> #endif

    cheers,
      DaveK

Re: [PATCHx3] LD plugins: matters outstanding.

[Richard Henderson]

On 10/15/2010 09:23 AM, Dave Korn wrote:
> On 15/10/2010 16:52, Richard Henderson wrote:
> 
>> I think it's slightly confusing that HAVE_WINDOWS_H is defined only if
>> HAVE_DLFCN_H isn't.  I think it might be a tad better to also test
>> HAVE_DLFCN_H in the ifdef around the include.
> 
>     Just checking: you mean like so, right?
> 
>> #if !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)
>> #include <Windows.h>
>> #endif

Yep, thanks.


r~

Re: [PATCHx3] LD plugins: matters outstanding.

[Dave Korn]

On 15/10/2010 17:03, Richard Henderson wrote:
> On 10/15/2010 09:23 AM, Dave Korn wrote:
>> On 15/10/2010 16:52, Richard Henderson wrote:
>>
>>> I think it's slightly confusing that HAVE_WINDOWS_H is defined only if
>>> HAVE_DLFCN_H isn't.  I think it might be a tad better to also test
>>> HAVE_DLFCN_H in the ifdef around the include.
>>     Just checking: you mean like so, right?
>>
>>> #if !defined (HAVE_DLFCN_H) && defined (HAVE_WINDOWS_H)
>>> #include <Windows.h>
>>> #endif
> 
> Yep, thanks.
> 
> 
> r~

  All committed, now for the gcc side of things!

    cheers,
      DaveK

Re: [PATCHx3] LD plugins: matters outstanding.

[Tristan Gingold]

On Oct 15, 2010, at 6:46 PM, Dave Korn wrote:
> 
>  All committed, now for the gcc side of things!

Does it also mean that you're OK if I create the release branch this week ?

Tristan.

Re: [PATCHx3] LD plugins: matters outstanding.

[Dave Korn]

On 18/10/2010 14:31, Tristan Gingold wrote:
> On Oct 15, 2010, at 6:46 PM, Dave Korn wrote:
>>  All committed, now for the gcc side of things!
> 
> Does it also mean that you're OK if I create the release branch this week ?
> 
> Tristan.
> 

  Yep.  I'm still waiting for someone to explicitly OK the one real bug fix at
http://sourceware.org/ml/binutils/2010-10/msg00261.html, but maybe I should
just say "I'll commit it this evening unless anyone actually objects"...

    cheers,
      DaveK

Re: [PATCHx3] LD plugins: matters outstanding.

[Richard Sandiford]

Dave Korn <dave.korn.cygwin@gmail.com> writes:
> On 18/10/2010 14:31, Tristan Gingold wrote:
>> On Oct 15, 2010, at 6:46 PM, Dave Korn wrote:
>>>  All committed, now for the gcc side of things!
>> 
>> Does it also mean that you're OK if I create the release branch this week ?
>> 
>> Tristan.
>> 
>
>   Yep.  I'm still waiting for someone to explicitly OK the one real bug fix at
> http://sourceware.org/ml/binutils/2010-10/msg00261.html, but maybe I should
> just say "I'll commit it this evening unless anyone actually objects"...

Patch is OK, thanks.

Richard

mips and frv testsuite failures after plugin patch

[Alan Modra]

On Wed, Oct 20, 2010 at 04:08:23PM +0100, Dave Korn wrote:
> On 18/10/2010 14:31, Tristan Gingold wrote:
> > Does it also mean that you're OK if I create the release branch this week ?
> 
>   Yep.  I'm still waiting for someone to explicitly OK the one real bug fix at

Dave, your patch introduced a bunch of testsuite failures, some shown
below.

frv-elf  +FAIL: FRV TLS relocs, static linking
frv-elf  +FAIL: FRV TLS relocs, dynamic linking
frv-elf  +FAIL: FRV TLS relocs, pie linking
frv-elf  +FAIL: FRV TLS relocs, shared linking with local binding
frv-elf  +FAIL: FRV TLS relocs, shared linking with relaxation
frv-elf  +FAIL: FRV TLS relocs with addends, dynamic linking
frv-elf  +FAIL: FRV TLS relocs with addends, shared linking
frv-elf  +FAIL: FRV TLS relocs with addends, shared linking with static TLS
frv-elf  +FAIL: FRV TLS relocs with addends, dynamic linking, relaxing
frv-elf  +FAIL: FRV TLS relocs with addends, shared linking, relaxing
frv-elf  +FAIL: FRV TLS relocs with addends, shared linking with static TLS, relaxing
mips-linux  +FAIL: Dynamic executable with TLS
mips-linux  +FAIL: Shared library with multiple GOTs and TLS
mips-linux  +FAIL: Dynamic executable with TLS and versioning
mips-linux  +FAIL: Dynamic executable with TLS and versioning (order 2)
mips-linux  +FAIL: Dynamic executable with TLS and versioning (order 3)
mips-linux  +FAIL: Shared library with TLS and hidden symbols (3)

From a quick look at the symptoms and at the changes your patch
introduced, I'd say it was simply due to opening the plugin bfd in
plugin_get_ir_dummy_bfd via ldfile_try_open_bfd.  A number of the hash
functions in elf32-frv.c and elfxx-mips.x use abfd->id, resulting in
various bits and pieces coming out of a hash traversal in a different
order after your patch.

-- 
Alan Modra
Australia Development Lab, IBM

Re: mips and frv testsuite failures after plugin patch

[Dave Korn]

On 25/10/2010 04:34, Alan Modra wrote:
> On Wed, Oct 20, 2010 at 04:08:23PM +0100, Dave Korn wrote:
>> On 18/10/2010 14:31, Tristan Gingold wrote:
>>> Does it also mean that you're OK if I create the release branch this week ?
>>   Yep.  I'm still waiting for someone to explicitly OK the one real bug fix at
> 
> Dave, your patch introduced a bunch of testsuite failures, some shown
> below.
> 
> frv-elf  +FAIL: FRV TLS relocs, static linking

> mips-linux  +FAIL: Dynamic executable with TLS

  Urgh.  Any other targets?  A quick grep suggests bfin and m68k might be
affected too.

> From a quick look at the symptoms and at the changes your patch
> introduced, I'd say it was simply due to opening the plugin bfd in
> plugin_get_ir_dummy_bfd via ldfile_try_open_bfd.  A number of the hash
> functions in elf32-frv.c and elfxx-mips.x use abfd->id, resulting in
> various bits and pieces coming out of a hash traversal in a different
> order after your patch.

  Groan.  Would it be suitable to update the testcases so they match the new
order of outputs?


  ... oooooor, I could make _bfd_delete_bfd decrement the _bfd_id_counter when
it notices we're deleting the last-created BFD?  Hmm, I like that idea.  It
seems a lot simpler.  There shouldn't be any problem with the notion of
speculatively allocating an ID and then recycling it immediately (at any rate,
before any other IDs are allocated).

    cheers,
      DaveK

Re: mips and frv testsuite failures after plugin patch

[Alan Modra]

On Mon, Oct 25, 2010 at 07:20:37AM +0100, Dave Korn wrote:
>   Urgh.  Any other targets?  A quick grep suggests bfin and m68k might be
> affected too.

Yep.

m68k-linux  +FAIL: ld-m68k/got-multigot-12-13-14-34-35-ok
m68k-linux  +FAIL: ld-m68k/got-xgot-12-13-14-15-34-35-ok

>   Groan.  Would it be suitable to update the testcases so they match the new
> order of outputs?

No, that won't work because you made the plugin support conditional.

>   ... oooooor, I could make _bfd_delete_bfd decrement the _bfd_id_counter when
> it notices we're deleting the last-created BFD?  Hmm, I like that idea.  It
> seems a lot simpler.  There shouldn't be any problem with the notion of
> speculatively allocating an ID and then recycling it immediately (at any rate,
> before any other IDs are allocated).

Or this.

bfd/
	* opncls.c (_bfd_id_counter): Rename to bfd_id_counter.
	(bfd_reserved_id_counter, bfd_use_reserved_id): New vars.
	(_bfd_new_bfd): Use negative id when bfd_use_reserved_id.
	(bfd_create): Doc fix.
	* bfd-in2.h: Regenerate.
ld/
	* plugin.c (plugin_get_ir_dummy_bfd): Set bfd_use_reserved_id.
	Formatting.

Index: bfd/opncls.c
===================================================================
RCS file: /cvs/src/src/bfd/opncls.c,v
retrieving revision 1.64
diff -u -p -r1.64 opncls.c
--- bfd/opncls.c	26 May 2010 07:37:36 -0000	1.64
+++ bfd/opncls.c	25 Oct 2010 04:56:54 -0000
@@ -38,9 +38,17 @@
 #define S_IXOTH 0001	/* Execute by others.  */
 #endif
 
-/* Counter used to initialize the bfd identifier.  */
+/* Counters used to initialize the bfd identifier.  */
 
-static unsigned int _bfd_id_counter = 0;
+static unsigned int bfd_id_counter = 0;
+static unsigned int bfd_reserved_id_counter = 0;
+
+/*
+CODE_FRAGMENT
+.{* Set to N to open the next N BFDs using an alternate id space.  *}
+.extern unsigned int bfd_use_reserved_id;
+*/
+unsigned int bfd_use_reserved_id = 0;
 
 /* fdopen is a loser -- we should use stdio exclusively.  Unfortunately
    if we do that we can't use fcntl.  */
@@ -56,7 +64,13 @@ _bfd_new_bfd (void)
   if (nbfd == NULL)
     return NULL;
 
-  nbfd->id = _bfd_id_counter++;
+  if (bfd_use_reserved_id)
+    {
+      nbfd->id = --bfd_reserved_id_counter;
+      --bfd_use_reserved_id;
+    }
+  else
+    nbfd->id = bfd_id_counter++;
 
   nbfd->memory = objalloc_create ();
   if (nbfd->memory == NULL)
@@ -753,7 +767,7 @@ SYNOPSIS
 DESCRIPTION
 	Create a new BFD in the manner of <<bfd_openw>>, but without
 	opening a file. The new BFD takes the target from the target
-	used by @var{template}. The format is always set to <<bfd_object>>.
+	used by @var{templ}. The format is always set to <<bfd_object>>.
 */
 
 bfd *
Index: ld/plugin.c
===================================================================
RCS file: /cvs/src/src/ld/plugin.c,v
retrieving revision 1.4
diff -u -p -r1.4 plugin.c
--- ld/plugin.c	20 Oct 2010 17:01:05 -0000	1.4
+++ ld/plugin.c	25 Oct 2010 04:57:23 -0000
@@ -224,16 +224,18 @@ bfd *
 plugin_get_ir_dummy_bfd (const char *name, bfd *srctemplate)
 {
   asection *sec;
-  bfd *abfd = bfd_create (
-		concat (name, IRONLY_SUFFIX, (const char *)NULL),
-		srctemplate);
+  bfd *abfd;
+
+  bfd_use_reserved_id = 1;
+  abfd = bfd_create (concat (name, IRONLY_SUFFIX, (const char *)NULL),
+		     srctemplate);
   bfd_set_arch_info (abfd, bfd_get_arch_info (srctemplate));
   bfd_make_writable (abfd);
   /* Create a minimal set of sections to own the symbols.  */
   sec = bfd_make_section_old_way (abfd, ".text");
   bfd_set_section_flags (abfd, sec,
-	SEC_CODE | SEC_HAS_CONTENTS | SEC_READONLY
-	| SEC_ALLOC | SEC_LOAD | SEC_KEEP);
+			 (SEC_CODE | SEC_HAS_CONTENTS | SEC_READONLY
+			  | SEC_ALLOC | SEC_LOAD | SEC_KEEP));
   sec->output_section = sec;
   sec->output_offset = 0;
   return abfd;

-- 
Alan Modra
Australia Development Lab, IBM

Re: mips and frv testsuite failures after plugin patch

[Dave Korn]

On 25/10/2010 07:22, Alan Modra wrote:

>>   ... oooooor, I could make _bfd_delete_bfd decrement the _bfd_id_counter when
>> it notices we're deleting the last-created BFD?  Hmm, I like that idea.  It
>> seems a lot simpler.  There shouldn't be any problem with the notion of
>> speculatively allocating an ID and then recycling it immediately (at any rate,
>> before any other IDs are allocated).
>
> Or this.
>
> bfd/
> 	* opncls.c (_bfd_id_counter): Rename to bfd_id_counter.
> 	(bfd_reserved_id_counter, bfd_use_reserved_id): New vars.
> 	(_bfd_new_bfd): Use negative id when bfd_use_reserved_id.
> 	(bfd_create): Doc fix.
> 	* bfd-in2.h: Regenerate.
> ld/
> 	* plugin.c (plugin_get_ir_dummy_bfd): Set bfd_use_reserved_id.
> 	Formatting.

  Counting down from the other end, yeah, I considered that too; it's probably
even more reliable.  It looks like it ought to work.

  On the other hand, your patch didn't make any difference when I tried it
against cross-frv-elf on HEAD: no change in the test results at all.

  On the third hand, it appears to me that those "FRV TLS relocs" failures
were all pre-existing before my patch.  I wound back a repository to

> commit 88a19d49326ad9b004d2d653fe5cc4f225b266e5
> Author: Alan Modra <amodra
> Date:   Wed Oct 13 23:00:05 2010 +0000
>
>     daily update

... here, and it still gives FAILs for all those tests.  So that explains why
patching bfd doesn't fix them for me, but leaves open the mystery of why
you've only just started seeing them.

    cheers,
      DaveK

Re: mips and frv testsuite failures after plugin patch

[Alan Modra]

On Mon, Oct 25, 2010 at 10:11:06AM +0100, Dave Korn wrote:
>   On the other hand, your patch didn't make any difference when I tried it
> against cross-frv-elf on HEAD: no change in the test results at all.

I did commit the ld/ part a bit later than the bfd/ part.  Perhaps you
didn't get the ld/plugin.c change?

>   On the third hand, it appears to me that those "FRV TLS relocs" failures
> were all pre-existing before my patch.

It doesn't appear that way to me.

-- 
Alan Modra
Australia Development Lab, IBM

[PATCH] fix testsuite ldscripts problem [was Re: mips and frv testsuite failures after plugin patch]

[Dave Korn]

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

On 25/10/2010 12:06, Alan Modra wrote:
> On Mon, Oct 25, 2010 at 10:11:06AM +0100, Dave Korn wrote:
>>   On the other hand, your patch didn't make any difference when I tried it
>> against cross-frv-elf on HEAD: no change in the test results at all.
> 
> I did commit the ld/ part a bit later than the bfd/ part.  Perhaps you
> didn't get the ld/plugin.c change?

  I applied it manually to one of my sandboxes from your email, so no worry
there.  Thanks for taking care of it.

>>   On the third hand, it appears to me that those "FRV TLS relocs" failures
>> were all pre-existing before my patch.
> 
> It doesn't appear that way to me.

  It turns out there was a confounding factor causing them to fail anyway;
lots of the ld tests were failing with errors like this:

> Executing on host: sh -c {./ld-new -melf32frvfd
> -L/gnu/binutils/git.repo/binutils/ld/testsuite/ld-frv -static
> tmpdir/tls-1-dep.o -o tmpdir/dump tmpdir/dump0.o 2>&1} /dev/null ld.tmp
> (timeout = 300)
> spawn [open ...]
> /gnu/binutils/frv-elf.clean/ld/./.libs/ld-new: cannot open linker script
> file ldscripts/elf32frvfd.xc: No such file or directory
> failed with: </gnu/binutils/frv-elf.clean/ld/./.libs/ld-new: cannot open
> linker script file ldscripts/elf32frvfd.xc: No such file or directory>,
> expected: <>
> /gnu/binutils/frv-elf.clean/ld/./.libs/ld-new: cannot open linker script
> file ldscripts/elf32frvfd.xc: No such file or directory
> failed with: </gnu/binutils/frv-elf.clean/ld/./.libs/ld-new: cannot open
> linker script file ldscripts/elf32frvfd.xc: No such file or directory>,
> expected: <>
> FAIL: FRV TLS relocs, static linking

  As it happens, there's a bug w.r.t. using linker scripts in the testsuite on
hosts that require a libtool wrapper.  We rely on find_scripts_dir() in
ldfile.c to locate $objdir/ld/ldscripts/ by calling

>   dir = make_relative_prefix (program_name, ".", ".");

which works fine on (e.g.) linux hosts where program_name is
$objdir/ld/ld-new, but fails on hosts that need a libtool wrapper, where the
actual program is at ( $objdir/ld/.libs/ld-new.

  A quick "ln -s ../ldscripts .libs/ldscripts" fixed it, so attached is a
patch that does pretty much the same at "make check" time, what do you think?

ld/ChangeLog:

	* Makefile.am (ldscripts-link): New phony target.
	(check-DEJAGNU): Depend on it.
	* Makefile.in: Regenerate.

  [ A bigger problem ISTM is that find_scripts_dir() will always prefer an
existing installed ldscripts directory if there is one, but I haven't thought
what to do about that, and I'm going to punt on it, I have too much else going
on right now. ]

    cheers,
      DaveK


[-- Attachment #2: testsuite-ldscripts-fix.diff --]
[-- Type: text/x-c, Size: 856 bytes --]

Index: ld/Makefile.am
===================================================================
RCS file: /cvs/src/src/ld/Makefile.am,v
retrieving revision 1.292
diff -p -u -r1.292 Makefile.am
--- ld/Makefile.am	14 Oct 2010 01:31:29 -0000	1.292
+++ ld/Makefile.am	25 Oct 2010 19:45:45 -0000
@@ -1916,7 +1916,16 @@ EXTRA_ld_new_SOURCES += $(ALL_EMULATION_
 # This is the real libbfd.a created by libtool.
 TESTBFDLIB = @TESTBFDLIB@
 
-check-DEJAGNU: site.exp
+ldscripts-link:
+	-eval "x`$(LIBTOOL) --config | $(GREP) ^objdir=`" && \
+	if test -d $$xobjdir; then \
+	    test ! -e $$xobjdir/ldscripts \
+		&& $(LN_S) ../ldscripts $$xobjdir/ldscripts; \
+	fi
+
+.PHONY: ldscripts-link
+
+check-DEJAGNU: site.exp ldscripts-link
 	srcroot=`cd $(srcdir) && pwd`; export srcroot; \
 	r=`pwd`; export r; \
 	LC_COLLATE=; LC_ALL=; LANG=; export LC_COLLATE LC_ALL LANG; \

Re: [PATCH] fix testsuite ldscripts problem [was Re: mips and frv testsuite failures after plugin patch]

[Alan Modra]

On Mon, Oct 25, 2010 at 09:23:18PM +0100, Dave Korn wrote:
>   A quick "ln -s ../ldscripts .libs/ldscripts" fixed it, so attached is a
> patch that does pretty much the same at "make check" time, what do you think?

Seems a reasonable fix.  Should be OK even without running libtool, ie.
always make a link in $xobjdir.

>   [ A bigger problem ISTM is that find_scripts_dir() will always prefer an
> existing installed ldscripts directory if there is one

Err, yeah.  For a typical setup,
-DSCRIPTDIR='"/usr/local/powerpc-linux/lib"'
-DBINDIR='"/usr/local/bin"'
-DTOOLBINDIR='"/usr/local/powerpc-linux/bin"'
the first call to check_for_scripts_dir in find_scripts_dir will look
for dir_path_to_ld/../powerpc-linux/lib/ldscripts, the second for
dir_path_to_ld/../lib/ldscripts, and the third for
/usr/local/powerpc-linux/lib/ldscripts.  The comment on the third call
says "We've been installed normally", however, that actually handles
the case where ld *hasn't* been installed normally.  The normal ld
install will be in /usr/local/bin, so the first call to
check_for_scripts_dir will find it, even when ld is a symbolic link to
/usr/local/bin/ld.

I think that we could probably remove the third call.  If ld is moved
somewhere without its accompanying ldscripts dir (assuming ld is built
to use ldscripts), I'd say that is an error on the part of whoever
moved ld.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] fix testsuite ldscripts problem [was Re: mips and frv testsuite failures after plugin patch]

[Dave Korn]

On 26/10/2010 08:40, Alan Modra wrote:
> On Mon, Oct 25, 2010 at 09:23:18PM +0100, Dave Korn wrote:
>>   A quick "ln -s ../ldscripts .libs/ldscripts" fixed it, so attached is a
>> patch that does pretty much the same at "make check" time, what do you think?
> 
> Seems a reasonable fix.  Should be OK even without running libtool, ie.
> always make a link in $xobjdir.

  I take it you mean "always make a link in .libs/"?

    cheers,
      DaveK

Re: [PATCH] fix testsuite ldscripts problem [was Re: mips and frv testsuite failures after plugin patch]

[Ralf Wildenhues]

Hi Dave,

* Dave Korn wrote on Mon, Oct 25, 2010 at 10:23:18PM CEST:
> --- ld/Makefile.am	14 Oct 2010 01:31:29 -0000	1.292
> +++ ld/Makefile.am	25 Oct 2010 19:45:45 -0000
> @@ -1916,7 +1916,16 @@ EXTRA_ld_new_SOURCES += $(ALL_EMULATION_
>  # This is the real libbfd.a created by libtool.
>  TESTBFDLIB = @TESTBFDLIB@
>  
> -check-DEJAGNU: site.exp
> +ldscripts-link:
> +	-eval "x`$(LIBTOOL) --config | $(GREP) ^objdir=`" && \
> +	if test -d $$xobjdir; then \
> +	    test ! -e $$xobjdir/ldscripts \
> +		&& $(LN_S) ../ldscripts $$xobjdir/ldscripts; \

This won't work when LN_S is 'cp -p'; instead, please do
  cd $$xobjdir && $(LN_S) ../ldscripts ldscripts

as documented in 'info Autoconf --index LN_S'.

Besides, Solaris /bin/sh test does not have -e, so you might want to
prefer -f or -r if you're still going to use the test.

> +	fi
> +
> +.PHONY: ldscripts-link
> +
> +check-DEJAGNU: site.exp ldscripts-link
>  	srcroot=`cd $(srcdir) && pwd`; export srcroot; \
>  	r=`pwd`; export r; \
>  	LC_COLLATE=; LC_ALL=; LANG=; export LC_COLLATE LC_ALL LANG; \

Cheers,
Ralf

Re: [PATCH] fix testsuite ldscripts problem [was Re: mips and frv testsuite failures after plugin patch]

[Dave Korn]

On 26/10/2010 21:22, Ralf Wildenhues wrote:
> Hi Dave,

  Hi Ralf :)

> * Dave Korn wrote on Mon, Oct 25, 2010 at 10:23:18PM CEST:
>> --- ld/Makefile.am	14 Oct 2010 01:31:29 -0000	1.292
>> +++ ld/Makefile.am	25 Oct 2010 19:45:45 -0000
>> @@ -1916,7 +1916,16 @@ EXTRA_ld_new_SOURCES += $(ALL_EMULATION_
>>  # This is the real libbfd.a created by libtool.
>>  TESTBFDLIB = @TESTBFDLIB@
>>  
>> -check-DEJAGNU: site.exp
>> +ldscripts-link:
>> +	-eval "x`$(LIBTOOL) --config | $(GREP) ^objdir=`" && \
>> +	if test -d $$xobjdir; then \
>> +	    test ! -e $$xobjdir/ldscripts \
>> +		&& $(LN_S) ../ldscripts $$xobjdir/ldscripts; \
> 
> This won't work when LN_S is 'cp -p'; instead, please do
>   cd $$xobjdir && $(LN_S) ../ldscripts ldscripts
> 
> as documented in 'info Autoconf --index LN_S'.

  Thank you; I looked up eval but forgot to look up linking.  Will change as
you suggest.

> Besides, Solaris /bin/sh test does not have -e, so you might want to
> prefer -f or -r if you're still going to use the test.

  Surely -f isn't right?  I think that checks for a regular file, and I do not
know whether a link would necessarily count as one.  I really only want to
check for existence of any kind at all, I don't care what because if someone's
already created 'ldscripts' in the objdir, they must know what they're doing,
it wouldn't just happen by accident.  So I want the simplest portable
existence test of any kind.  I don't even want to care if it's readable,
although I suppose it wouldn't matter either way if it's not.  What's the best
most portable way to just test for existence?

    cheers,
      DaveK

Re: [PATCH] fix testsuite ldscripts problem

[Ralf Wildenhues]

* Dave Korn wrote on Wed, Oct 27, 2010 at 01:37:26AM CEST:
> On 26/10/2010 21:22, Ralf Wildenhues wrote:
> > * Dave Korn wrote on Mon, Oct 25, 2010 at 10:23:18PM CEST:
> >> -check-DEJAGNU: site.exp
> >> +ldscripts-link:
> >> +	-eval "x`$(LIBTOOL) --config | $(GREP) ^objdir=`" && \

By the way, *really* old shells interpret unquoted ^ as synonym to |
(single quotes are safe here around ^objdir=, unlike double quotes).

> >> +	if test -d $$xobjdir; then \
> >> +	    test ! -e $$xobjdir/ldscripts \
> >> +		&& $(LN_S) ../ldscripts $$xobjdir/ldscripts; \

> > Besides, Solaris /bin/sh test does not have -e, so you might want to
> > prefer -f or -r if you're still going to use the test.
> 
>   Surely -f isn't right?  I think that checks for a regular file, and I do not
> know whether a link would necessarily count as one.

If the link points to a regular file, then yes.

Since you link to a directory (sorry for not realizing this sooner),
$(LN_S) won't work anyway if it's not 'ln -s', so at that point you'd
probably have to mkdir and cp -p or cp -R anyway.  test -d should
otherwise work also for symlinks to directories.

>  I really only want to
> check for existence of any kind at all, I don't care what because if someone's
> already created 'ldscripts' in the objdir, they must know what they're doing,
> it wouldn't just happen by accident.  So I want the simplest portable
> existence test of any kind.  I don't even want to care if it's readable,
> although I suppose it wouldn't matter either way if it's not.  What's the best
> most portable way to just test for existence?

One of test -r, test -x, test -f.  Or, given you have suitable
permissions in its enclosing directory, trying to mkdir it, and seeing
that fail.  ;-)

Cheers,
Ralf

Re: [PATCH] fix testsuite ldscripts problem

[Dave Korn]

On 27/10/2010 19:22, Ralf Wildenhues wrote:
> * Dave Korn wrote on Wed, Oct 27, 2010 at 01:37:26AM CEST:
>> On 26/10/2010 21:22, Ralf Wildenhues wrote:
>>> * Dave Korn wrote on Mon, Oct 25, 2010 at 10:23:18PM CEST:
>>>> -check-DEJAGNU: site.exp
>>>> +ldscripts-link:
>>>> +	-eval "x`$(LIBTOOL) --config | $(GREP) ^objdir=`" && \
> 
> By the way, *really* old shells interpret unquoted ^ as synonym to |
> (single quotes are safe here around ^objdir=, unlike double quotes).

  Thanks Ralf, I was hoping you'd show up with advice :)  I'll change as you
advise; as far as I know, it's only DOS that has a different objdir from
.libs, and that's exactly also where '^' is a line continuation....

>>>> +	if test -d $$xobjdir; then \
>>>> +	    test ! -e $$xobjdir/ldscripts \
>>>> +		&& $(LN_S) ../ldscripts $$xobjdir/ldscripts; \
> 
>>> Besides, Solaris /bin/sh test does not have -e, so you might want to
>>> prefer -f or -r if you're still going to use the test.
>>   Surely -f isn't right?  I think that checks for a regular file, and I do not
>> know whether a link would necessarily count as one.
> 
> If the link points to a regular file, then yes.

  Actually it points to a directory, so perhaps not?

> Since you link to a directory (sorry for not realizing this sooner),

  Heh!

> $(LN_S) won't work anyway if it's not 'ln -s', so at that point you'd
> probably have to mkdir and cp -p or cp -R anyway.  test -d should
> otherwise work also for symlinks to directories.

  I thought the whole point of $(LN_S) was that it would turn into a copy on
platforms like mingw?

    cheers,
      DaveK

Re: [PATCH] fix testsuite ldscripts problem

[Ralf Wildenhues]

* Dave Korn wrote on Wed, Oct 27, 2010 at 10:43:19PM CEST:
> On 27/10/2010 19:22, Ralf Wildenhues wrote:
> > * Dave Korn wrote on Wed, Oct 27, 2010 at 01:37:26AM CEST:
> >> On 26/10/2010 21:22, Ralf Wildenhues wrote:
> >>> Besides, Solaris /bin/sh test does not have -e, so you might want to
> >>> prefer -f or -r if you're still going to use the test.
> >>   Surely -f isn't right?  I think that checks for a regular file, and I do not
> >> know whether a link would necessarily count as one.
> > 
> > If the link points to a regular file, then yes.
> 
>   Actually it points to a directory, so perhaps not?

Right; see below.

> > Since you link to a directory (sorry for not realizing this sooner),
> 
>   Heh!
> 
> > $(LN_S) won't work anyway if it's not 'ln -s', so at that point you'd
> > probably have to mkdir and cp -p or cp -R anyway.  test -d should
> > otherwise work also for symlinks to directories.
> 
>   I thought the whole point of $(LN_S) was that it would turn into a copy on
> platforms like mingw?

Right; but $(LN_S) really only is applicable to regular files, not
directories.  I wouldn't see how to easily abstract away both within
one command.

Cheers,
Ralf

Re: [PATCH] fix testsuite ldscripts problem

[Dave Korn]

On 27/10/2010 22:13, Ralf Wildenhues wrote:

> Right; but $(LN_S) really only is applicable to regular files, not
> directories.  

  Augh!  Thanks for telling me.  Time out for a quick rethink, then.  Probably
a copy is the simplest answer.

    cheers,
      DaveK

Re: [PATCH] fix testsuite ldscripts problem [was Re: mips and frv testsuite failures after plugin patch]

[Alan Modra]

On Tue, Oct 26, 2010 at 06:10:39PM +1030, Alan Modra wrote:
> On Mon, Oct 25, 2010 at 09:23:18PM +0100, Dave Korn wrote:
> >   [ A bigger problem ISTM is that find_scripts_dir() will always prefer an
> > existing installed ldscripts directory if there is one
> 
> Err, yeah.  For a typical setup,
> -DSCRIPTDIR='"/usr/local/powerpc-linux/lib"'
> -DBINDIR='"/usr/local/bin"'
> -DTOOLBINDIR='"/usr/local/powerpc-linux/bin"'
> the first call to check_for_scripts_dir in find_scripts_dir will look
> for dir_path_to_ld/../powerpc-linux/lib/ldscripts, the second for
> dir_path_to_ld/../lib/ldscripts, and the third for
> /usr/local/powerpc-linux/lib/ldscripts.  The comment on the third call
> says "We've been installed normally", however, that actually handles
> the case where ld *hasn't* been installed normally.  The normal ld
> install will be in /usr/local/bin, so the first call to
> check_for_scripts_dir will find it, even when ld is a symbolic link to
> /usr/local/bin/ld.
> 
> I think that we could probably remove the third call.  If ld is moved
> somewhere without its accompanying ldscripts dir (assuming ld is built
> to use ldscripts), I'd say that is an error on the part of whoever
> moved ld.

Applied.

	* ldfile.c (find_scripts_dir): Don't look in absolute SCRIPTDIR.

Index: ld/ldfile.c
===================================================================
RCS file: /cvs/src/src/ld/ldfile.c,v
retrieving revision 1.55
diff -u -p -r1.55 ldfile.c
--- ld/ldfile.c	14 Oct 2010 01:31:31 -0000	1.55
+++ ld/ldfile.c	28 Oct 2010 03:49:48 -0000
@@ -547,7 +547,6 @@ check_for_scripts_dir (char *dir)
 
    SCRIPTDIR (passed from Makefile)
 	     (adjusted according to the current location of the binary)
-   SCRIPTDIR (passed from Makefile)
    the dir where this program is (for using it from the build tree).  */
 
 static char *
@@ -571,10 +570,6 @@ find_scripts_dir (void)
       free (dir);
     }
 
-  if (check_for_scripts_dir (SCRIPTDIR))
-    /* We've been installed normally.  */
-    return SCRIPTDIR;
-
   /* Look for "ldscripts" in the dir where our binary is.  */
   dir = make_relative_prefix (program_name, ".", ".");
   if (dir)

-- 
Alan Modra
Australia Development Lab, IBM