[PATCH,trunk+2.20] Fix issues in ld.bfd plugin interface [0/6]

[PATCH,trunk+2.20] Fix issues in ld.bfd plugin interface [0/6]

[Dave Korn]

    Hi list,

  This set of patches (following, rather than attached) aims at fixing the
problems currently extant in the first implementation of the linker plugin API
in LD.  As well as fixing the linking issues, there are a couple of related
bugfixes that arose in the course of testing.  Relevant PRs include:

http://sourceware.org/bugzilla/show_bug.cgi?id=12369
Symbols present in LTO symbol table resolved as PREVAILING_DEF_IRONLY and
optimized out appears in final symbol table.

http://sourceware.org/bugzilla/show_bug.cgi?id=12248
Linker plugin failed to preserve linking order

http://sourceware.org/bugzilla/show_bug.cgi?id=12365
undefined references produced by linker plugin are silently ignored

http://sourceware.org/bugzilla/show_bug.cgi?id=12323
linker plugin does not handle TLS

  The patches (to follow, as replies to this post) contain the following changes:

* 001ld-pe-orphan-section-alignment.diff

  Fixes a problem with orphan section alignment in the PE-COFF linker, this
was causing "ld -r" of objects containing LTO sections to break.

* 002ld-plugin-api-no-ironly-suffix.diff

  Removes the ugly name suffix from the dummy IR BFDs as previously discussed.

* 003ld-plugin-api-link-order.diff

  One of the two main functional changes to the plugin API.  This patch works
by taking all the files added by the lto plugin and inserting them into the
list of input statements at a point immediately after the first claimed object
file.

* 004ld-pr47527.diff

  The dummy IR BFDs added by the plugin API don't have any format-specific
contents, such as ELF attributes and ARM EABI versions.  This was causing
lang_check to barf on imaginary merging problems; avoided by not performing
the checks on IR files.

* 005ld-link-stage2.diff

  This is the second main functional change, and it's really the only way to
resolve the problems caused by discrepancies between the initial set of
symbols returned by the plugin based on the LTO symtabs in the IR files, and
the actual set of symbols used and defined by the eventual object file(s)
added to the link after LTRANS.  It's largely the same approach as HJ's
2-stage link, except that it closes and reopens the existing input BFDs in
place, rather than adding a second set of input statements.  This results in
some different behaviour between HJ's linker and this patched version, which I
will mention in the section below with test results.

* 006ld-plugin-sym-wrappers.diff

  I found this running the GCC testsuite, where on cygwin we use ld wrappers
to allow replacement of libstdc++ new/delete operators.

* 00xld-lto-test-tweaks.diff

  This is out of the main series, because it's a couple of tweaks to the LTO
tests in HJ's branch; included here for reference, since I've been running
tests with it folded in.  I notice it's slightly outdated here, as upstream HJ
has already added gcc_ld_flag (and gcc_gas_flag, in fact) to the ld/12365 test
in lto.exp, so that bit won't apply cleanly, but the second change in that
hunk (adding a '_?' prefix to the regex) is still needed.  FYI, YMMV.

  Results: current binutils HEAD gives the following failures from HJ's
ld-plugin/lto.exp tests, on i686-pc-cygwin:

> FAIL: PR ld/12365
> FAIL: ld-plugin/lto-4r-a
> FAIL: ld-plugin/lto-4r-b
> FAIL: ld-plugin/lto-4r-c
> FAIL: ld-plugin/lto-4r-d
> FAIL: LTO 5 symbol
> FAIL: LTO 3b
> FAIL: LTO 4a
> FAIL: LTO 4c
> FAIL: LTO 4d
> FAIL: LTO 5
> FAIL: LTO 10

  HJ's mixed-lto branch fixes 12365 and handles mixed IR and non-IR object files:

> FAIL: ld-plugin/lto-10r
> FAIL: LTO 5 symbol
> FAIL: LTO 3b
> FAIL: LTO 4a
> FAIL: LTO 4c
> FAIL: LTO 4d
> FAIL: LTO 5
> FAIL: LTO 10

  After my patches, LD gives the following results:

> FAIL: ld-plugin/lto-4r-a
> FAIL: ld-plugin/lto-4r-b
> FAIL: ld-plugin/lto-4r-c
> FAIL: ld-plugin/lto-4r-d
> FAIL: LTO 4a
> FAIL: LTO 4c
> FAIL: LTO 4d
> FAIL: LTO 10
> FAIL: LTO 12a
> FAIL: LTO 12b
> FAIL: LTO 15

  Relative to current LD, that has fixes to 12365, 5 symbol, 3b and 5.  The
new FAILures on 12a/b and 15 are actually a difference of interpretation about
how the linker is supposed to work between me and HJ's code.  The tests assume
that it is not possible to override builtins such as __udivdi3 or memcpy, but
it works OK with the attached patches, so I'm not sure why these tests do what
they do.  For example, test LTO 15 has this source file compiled as an object:

> #include <stdio.h>
> 
> int main(int argc, char **argv)
> {
>   printf ("PASS\n");
>   return (int) ((unsigned long long) argc / argv[0][0]);
> }

... and this one in a library archive member:

> extern void abort (void);
> unsigned long long
> __udivdi3(unsigned long long n, unsigned long long d)
> {
>   abort ();
>   return n + d;
> }

... so I would have thought that the ideal case would be for an LTO link to
mimic a non-LTO link, and pull in the definition of __udivdi3 there and abort.

  That aside, the only thing that LD still doesn't handle after these patches
is mixed object files that are created by using "ld -r" to merge multiple IR
and non-IR object files.

  Patches to follow, as separate posts, so that we can discuss each in its own
thread.  I'm going to spend a couple of days testing it on 32- and 64-bit
linux, where I can also compare it to GOLD, verifying that it fixes the bugs I
think it should fix, and doing some lto-bootstrap-and-tests of GCC for
comparison, before I go committing anything, but I think the series is ready
to review so I'm posting it now.  Wouldn't mind if anyone wanted to help run
some testing on it either! :-)

    cheers,
      DaveK

[1/6][PATCH] Fix PE-COFF bug in orphan section alignment handling.

[Dave Korn]

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


    Hi list,

  Fixes a problem with orphan section alignment in the PE-COFF linker, this
was causing "ld -r" of objects containing LTO sections to break.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* emultempl/pe.em (gld_${EMULATION_NAME}_place_orphan): Preserve
	alignment of input sections when creating orphan output sections
	during relocatable link.
	* emultempl/pep.em (gld_${EMULATION_NAME}_place_orphan): Likewise.

  I could just approve and commit this myself, but since it came up as part of
developing the series I'll post it for comment anyway, in case there's perhaps
a better way to do what I want.

    cheers,
      DaveK


[-- Attachment #2: 001ld-pe-orphan-section-alignment.diff --]
[-- Type: text/x-c, Size: 2480 bytes --]

From 1aefb61e70c166a04e79afba432082247bd69e77 Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Sat, 19 Feb 2011 23:18:55 +0000
Subject: [PATCH] Fix PE-COFF bug in orphan section alignment handling.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* emultempl/pe.em (gld_${EMULATION_NAME}_place_orphan): Preserve
	alignment of input sections when creating orphan output sections
	during relocatable link.
	* emultempl/pep.em (gld_${EMULATION_NAME}_place_orphan): Likewise.
---
 ld/emultempl/pe.em  |    7 ++++++-
 ld/emultempl/pep.em |    7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/ld/emultempl/pe.em b/ld/emultempl/pe.em
index a3e4cdd..55041f3 100644
--- a/ld/emultempl/pe.em
+++ b/ld/emultempl/pe.em
@@ -2007,10 +2007,15 @@ gld_${EMULATION_NAME}_place_orphan (asection *s,
 		     ->output_section_statement);
 	}
 
-      /* All sections in an executable must be aligned to a page boundary.  */
+      /* All sections in an executable must be aligned to a page boundary.
+	 In a relocatable link, just preserve the incoming alignment; the
+	 address is discarded by lang_insert_orphan in that case, anyway.  */
       address = exp_unop (ALIGN_K, exp_nameop (NAME, "__section_alignment__"));
       os = lang_insert_orphan (s, secname, constraint, after, place, address,
 			       &add_child);
+      if (link_info.relocatable)
+	os->bfd_section->alignment_power = os->section_alignment
+					 = s->alignment_power;
     }
 
   /* If the section name has a '\$', sort it with the other '\$'
diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em
index a307c14..51dffff 100644
--- a/ld/emultempl/pep.em
+++ b/ld/emultempl/pep.em
@@ -1746,10 +1746,15 @@ gld_${EMULATION_NAME}_place_orphan (asection *s,
 		     ->output_section_statement);
 	}
 
-      /* All sections in an executable must be aligned to a page boundary.  */
+      /* All sections in an executable must be aligned to a page boundary.
+	 In a relocatable link, just preserve the incoming alignment; the
+	 address is discarded by lang_insert_orphan in that case, anyway.  */
       address = exp_unop (ALIGN_K, exp_nameop (NAME, "__section_alignment__"));
       os = lang_insert_orphan (s, secname, constraint, after, place, address,
 			       &add_child);
+      if (link_info.relocatable)
+	os->bfd_section->alignment_power = os->section_alignment
+					 = s->alignment_power;
     }
 
   /* If the section name has a '\$', sort it with the other '\$'

Re: [PATCH,trunk+2.21] Fix issues in ld.bfd plugin interface [0/6]

[Dave Korn]

  Oops.  Subject line typo corrected above ^^^^

    cheers,
      DaveK

[4/6][PATCH] Fix issue from GCC PR47527: no ELF flags, EABI attribs, etc. in dummy IR BFD.

[Dave Korn]

    Hi list,

  The dummy IR BFDs added by the plugin API don't have any format-specific
contents, such as ELF attributes and ARM EABI versions.  This was causing
lang_check to barf on imaginary merging problems; avoided by not performing
the checks on IR files.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* ldlang.c (lang_check): Don't run checks on dummy IR BFDs.


    cheers,
      DaveK

[5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Dave Korn]

    Hi list,

  This is the second main functional change, and it's really the only way to
resolve the problems caused by discrepancies between the initial set of
symbols returned by the plugin based on the LTO symtabs in the IR files, and
the actual set of symbols used and defined by the eventual object file(s)
added to the link after LTRANS.  It's largely the same approach as HJ's
2-stage link, except that it closes and reopens the existing input BFDs in
place, rather than adding a second set of input statements.  This results in
some different behaviour between HJ's linker and this patched version.


ld/ChangeLog:

2011-02-20  Dave Korn  <...

	PR ld/12365
	* ldcref.c (cref_hash_table_free): New function.
	* ld.h (cref_hash_table_free): Add prototype.
	* ldlang.c (lang_gc_sections): Dont de-exclude claimed file sections.
	(set_exclude): New function.
	(reopen_inputs): Likewise.  Walk list of input objects, excluding
	claimed (IR-only) files and archive members, then re-walk list, closing
	and re-opening and re-adding the symbols from objects and libs.
	(lang_process): After opening plugin-supplied objects and scanning
	their library dependencies, tear down existing link, cref and
	already-linked-section hashes, erase link_info input bfds list, finally
	call reopen_inputs.
	* plugin.c (plugin_opt_plugin_arg): Discard any instances of the
	now-obsolete "-pass-through=" option if found.

  I know Ian would rather avoid this approach, but I don't think that there's
any other way to do LTO in LD without either rearchitecting BFD quite a bit.
We can't do symbol resolution without adding symbols from the LTO symtabs into
the linker hash table, but then after LTRANS we may have a different final set
of symbols present.  There's no way to excise individual symbols from the
linker hash table in BFD, so you have to achieve the same effect by tearing
down the symbol table and rebuilding it without adding the unwanted symbols at
all second time round.  That means you have to close and reopen all the BFDs,
because BFD backends cache pointers to hash table entries in the bfd's private
data, which are now stale once you've built a new symbol table.  I can't see
any way around having to re-do at least this amount of work, but without it
we'll get bogus symbols in our final outputs, which is a real correctness issue.

    cheers,
      DaveK

[3/6][PATCH] Revise linker plugin API to better preserve link order.

[Dave Korn]

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


    Hi list,

  One of the two main functional changes to the plugin API.  This patch works
by taking all the files added by the lto plugin and inserting them into the
list of input statements at a point immediately after the first claimed object
file.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* ldlang.h (lang_input_statement_type): Add new 'claim_archive' flag,
	wrapping both it and 'claim' flag in #ifdef ENABLE_PLUGINS.
	* ldmain.c (add_archive_element): Set it if the member is claimed.
	* ldlang.c (new_afile): Initialise claim_archive and claimed members.
	(find_replacements_insert_point): New helper function.
	(lang_process): After adding and opening replacement files passed
	from plugin, splice them into correct place in statement list and
	file chains to preserve critical link order.
	(lang_list_insert_after): New helper function.
	(lang_list_remove_tail): Likewise.


    cheers,
      DaveK


[-- Attachment #2: 003ld-plugin-api-link-order.diff --]
[-- Type: text/x-c, Size: 8919 bytes --]

From f96f6949a3658da742d5f625fe012c1e007fe67c Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Sat, 19 Feb 2011 23:36:12 +0000
Subject: [PATCH] Revise linker plugin API to better preserve link order.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* ldlang.h (lang_input_statement_type): Add new 'claim_archive' flag,
	wrapping both it and 'claim' flag in #ifdef ENABLE_PLUGINS.
	* ldmain.c (add_archive_element): Set it if the member is claimed.
	* ldlang.c (new_afile): Initialise claim_archive and claimed members.
	(find_replacements_insert_point): New helper function.
	(lang_process): After adding and opening replacement files passed
	from plugin, splice them into correct place in statement list and
	file chains to preserve critical link order.
	(lang_list_insert_after): New helper function.
	(lang_list_remove_tail): Likewise.
---
 ld/ldlang.c |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 ld/ldlang.h |    5 ++
 ld/ldmain.c |    1 +
 3 files changed, 127 insertions(+), 9 deletions(-)

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 7ffbf45..b36dc6e 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -85,6 +85,13 @@ static void print_statement_list (lang_statement_union_type *,
 static void print_statements (void);
 static void print_input_section (asection *, bfd_boolean);
 static bfd_boolean lang_one_common (struct bfd_link_hash_entry *, void *);
+#ifdef ENABLE_PLUGINS
+static void lang_list_insert_after (lang_statement_list_type *destlist,
+				    lang_statement_list_type *srclist,
+				    lang_statement_union_type **field);
+static void lang_list_remove_tail (lang_statement_list_type *destlist,
+				   lang_statement_list_type *origlist);
+#endif /* ENABLE_PLUGINS */
 static void lang_record_phdrs (void);
 static void lang_do_version_exports_section (void);
 static void lang_finalize_version_expr_head
@@ -1126,6 +1133,10 @@ new_afile (const char *name,
   p->whole_archive = whole_archive;
   p->loaded = FALSE;
   p->missing_file = FALSE;
+#ifdef ENABLE_PLUGINS
+  p->claimed = FALSE;
+  p->claim_archive = FALSE;
+#endif /* ENABLE_PLUGINS */
 
   lang_statement_append (&input_file_chain,
 			 (lang_statement_union_type *) p,
@@ -6394,6 +6405,40 @@ lang_relax_sections (bfd_boolean need_layout)
     }
 }
 
+#ifdef ENABLE_PLUGINS
+/* Find the insert point for the plugin's replacement files.  We
+   place them after the first claimed real object file, or if the
+   first claimed object is an archive member, after the last real
+   object file immediately preceding the archive.  In the event
+   no objects have been claimed at all, we return the first dummy
+   object file on the list as the insert point; that works, but
+   the callee must be careful when relinking the file_chain as it
+   is not actually on that chain, only the statement_list and the
+   input_file list; in that case, the replacement files must be
+   inserted at the head of the file_chain.  */
+
+static lang_input_statement_type *
+find_replacements_insert_point (void)
+{
+  lang_input_statement_type *claim1, *lastobject;
+  lastobject = &input_file_chain.head->input_statement;
+  for (claim1 = &file_chain.head->input_statement;
+       claim1 != NULL;
+       claim1 = &claim1->next->input_statement)
+    {
+      if (claim1->claimed)
+	return claim1->claim_archive ? lastobject : claim1;
+      /* Update lastobject if this is a real object file.  */
+      if (claim1->the_bfd && (claim1->the_bfd->my_archive == NULL))
+	lastobject = claim1;
+    }
+  /* No files were claimed by the plugin.  Choose the last object
+     file found on the list (maybe the first, dummy entry) as the
+     insert point.  */
+  return lastobject;
+}
+#endif /* ENABLE_PLUGINS */
+
 void
 lang_process (void)
 {
@@ -6420,21 +6465,54 @@ lang_process (void)
   open_input_bfds (statement_list.head, FALSE);
 
 #ifdef ENABLE_PLUGINS
+  if (plugin_active_plugins_p ())
     {
-      union lang_statement_union **listend;
+      lang_statement_list_type added;
+      lang_statement_list_type files, inputfiles;
       /* Now all files are read, let the plugin(s) decide if there
 	 are any more to be added to the link before we call the
-	 emulation's after_open hook.  */
-      listend = statement_list.tail;
-      ASSERT (!*listend);
+	 emulation's after_open hook.  We create a private list of
+	 input statements for this purpose, which we will eventually
+	 insert into the global statment list after the first claimed
+	 file.  */
+      added = *stat_ptr;
+      /* We need to manipulate all three chains in synchrony.  */
+      files = file_chain;
+      inputfiles = input_file_chain;
       if (plugin_call_all_symbols_read ())
 	einfo (_("%P%F: %s: plugin reported error after all symbols read\n"),
 	       plugin_error_plugin ());
-      /* If any new files were added, they will be on the end of the
-	 statement list, and we can open them now by getting open_input_bfds
-	 to carry on from where it ended last time.  */
-      if (*listend)
-	open_input_bfds (*listend, FALSE);
+      /* Open any newly added files, updating the file chains.  */
+      open_input_bfds (added.head, FALSE);
+      /* Restore the global list pointer now they have all been added.  */
+      lang_list_remove_tail (stat_ptr, &added);
+      /* And detach the fresh ends of the file lists.  */
+      lang_list_remove_tail (&file_chain, &files);
+      lang_list_remove_tail (&input_file_chain, &inputfiles);
+      /* Were any new files added?  */
+      if (added.head != NULL)
+	{
+	  /* If so, we will insert them into the statement list immediately
+	     after the first input file that was claimed by the plugin.  */
+	  lang_input_statement_type *claim1 = find_replacements_insert_point ();
+	  /* If a plugin adds input files without having claimed any, we
+	     don't really have a good idea where to place them.  Just putting
+	     them at the start or end of the list is liable to leave them
+	     outside the crtbegin...crtend range.  */
+	  ASSERT (claim1 != NULL);
+	  /* Splice the new statement list into the old one after claim1.  */
+	  lang_list_insert_after (stat_ptr, &added, &claim1->header.next);
+	  /* Likewise for the file chains.  */
+	  lang_list_insert_after (&input_file_chain, &inputfiles,
+				  &claim1->next_real_file);
+	  /* We must be careful when relinking file_chain; we may need to
+	     insert the new files at the head of the list if the insert
+	     point chosen is the dummy first input file.  */
+	  if (claim1->filename)
+	    lang_list_insert_after (&file_chain, &files, &claim1->next);
+	  else
+	    lang_list_insert_after (&file_chain, &files, &file_chain.head);
+	}
     }
 #endif /* ENABLE_PLUGINS */
 
@@ -6857,6 +6935,40 @@ lang_statement_append (lang_statement_list_type *list,
   list->tail = field;
 }
 
+#ifdef ENABLE_PLUGINS
+/* Insert SRCLIST into DESTLIST after given element by chaining
+   on FIELD as the next-pointer.  (Counterintuitively does not need
+   a pointer to the actual after-node itself, just its chain field.)  */
+
+static void
+lang_list_insert_after (lang_statement_list_type *destlist,
+			lang_statement_list_type *srclist,
+			lang_statement_union_type **field)
+{
+  *(srclist->tail) = *field;
+  *field = srclist->head;
+  if (destlist->tail == field)
+    destlist->tail = srclist->tail;
+}
+
+/* Detach new nodes added to DESTLIST since the time ORIGLIST
+   was taken as a copy of it and leave them in ORIGLIST.  */
+
+static void
+lang_list_remove_tail (lang_statement_list_type *destlist,
+		       lang_statement_list_type *origlist)
+{
+  union lang_statement_union **savetail;
+  /* Check that ORIGLIST really is an earlier state of DESTLIST.  */
+  ASSERT (origlist->head == destlist->head);
+  savetail = origlist->tail;
+  origlist->head = *(savetail);
+  origlist->tail = destlist->tail;
+  destlist->tail = savetail;
+  *savetail = NULL;
+}
+#endif /* ENABLE_PLUGINS */
+
 /* Set the output format type.  -oformat overrides scripts.  */
 
 void
diff --git a/ld/ldlang.h b/ld/ldlang.h
index 8aff0b3..db47af8 100644
--- a/ld/ldlang.h
+++ b/ld/ldlang.h
@@ -287,9 +287,14 @@ typedef struct lang_input_statement_struct
   /* Set if the file does not exist.  */
   unsigned int missing_file : 1;
 
+#ifdef ENABLE_PLUGINS
   /* Set if the file was claimed by a plugin.  */
   unsigned int claimed : 1;
 
+  /* Set if the file was claimed from an archive.  */
+  unsigned int claim_archive : 1;
+#endif /* ENABLE_PLUGINS */
+
 } lang_input_statement_type;
 
 typedef struct
diff --git a/ld/ldmain.c b/ld/ldmain.c
index 6e86c3d..005a894 100644
--- a/ld/ldmain.c
+++ b/ld/ldmain.c
@@ -833,6 +833,7 @@ add_archive_element (struct bfd_link_info *info,
 	      /* Substitute the dummy BFD.  */
 	      input->the_bfd = file.handle;
 	      input->claimed = TRUE;
+	      input->claim_archive = TRUE;
 	      bfd_make_readable (input->the_bfd);
 	      *subsbfd = input->the_bfd;
 	    }

[2/6][PATCH] Do not use dummy bfd suffix for recognition, make it human-readable instead.

[Dave Korn]

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


    Hi list,

  Removes the ugly name suffix from the dummy IR BFDs as previously discussed.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* plugin.c (IRONLY_SUFFIX): Revise to nicely human-readable form.
	(IRONLY_SUFFIX_LEN): Delete.
	(plugin_get_ir_dummy_bfd): Don't append IRONLY_SUFFIX.
	(is_ir_dummy_bfd): Don't look for suffix; check claimed flag of
	enclosing lang input statement instead.

  Think this one is straightforward and uncontroversial now.

    cheers,
      DaveK


[-- Attachment #2: 002ld-plugin-api-no-ironly-suffix.diff --]
[-- Type: text/x-c, Size: 2245 bytes --]

From 5284753e8fae37a619fee7683f234cfdf2dbc05d Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Sat, 19 Feb 2011 23:24:14 +0000
Subject: [PATCH] Do not use dummy bfd suffix for recognition, make it human-readable instead.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* plugin.c (IRONLY_SUFFIX): Revise to nicely human-readable form.
	(IRONLY_SUFFIX_LEN): Delete.
	(plugin_get_ir_dummy_bfd): Don't append IRONLY_SUFFIX.
	(is_ir_dummy_bfd): Don't look for suffix; check claimed flag of
	enclosing lang input statement instead.
---
 ld/plugin.c |   24 ++++++++++--------------
 1 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/ld/plugin.c b/ld/plugin.c
index 6e3f923..c1672f6 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -38,12 +38,8 @@
 
 /* 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
-   plugin.  */
-#define IRONLY_SUFFIX		".ironly\004"
-
-/* This is sizeof an array of chars, not sizeof a const char *.  We
-   also have to avoid inadvertently counting the trailing NUL.  */
-#define IRONLY_SUFFIX_LEN	(sizeof (IRONLY_SUFFIX) - 1)
+   plugin.  For cosmetic use only; appears in maps, crefs etc.  */
+#define IRONLY_SUFFIX " (symbol from plugin)"
 
 /* Stores a single argument passed to a plugin.  */
 typedef struct plugin_arg
@@ -250,14 +246,14 @@ plugin_get_ir_dummy_bfd (const char *name, bfd *srctemplate)
 static bfd_boolean
 is_ir_dummy_bfd (const bfd *abfd)
 {
-  size_t namlen;
-
-  if (abfd == NULL)
-    return FALSE;
-  namlen = strlen (abfd->filename);
-  if (namlen < IRONLY_SUFFIX_LEN)
-    return FALSE;
-  return !strcmp (abfd->filename + namlen - IRONLY_SUFFIX_LEN, IRONLY_SUFFIX);
+  /* ABFD can sometimes legitimately be NULL, e.g. when called from one
+     of the linker callbacks for a symbol in the *ABS* or *UND* sections.
+     Likewise, the usrdata field may be NULL if ABFD was added by the
+     backend without a corresponding input statement, as happens e.g.
+     when processing DT_NEEDED dependencies.  */
+  return abfd
+	 && abfd->usrdata
+	 && ((lang_input_statement_type *)(abfd->usrdata))->claimed;
 }
 
 /* Helpers to convert between BFD and GOLD symbol formats.  */

[x/6][PATCH] Portability tweaks for LTO tests.

[Dave Korn]

    Hi list,

  This is out of the main series, because it's a couple of tweaks to the LTO
tests in HJ's branch; included here for reference, since I've been running
tests with it folded in.  I notice it's slightly outdated here, as upstream HJ
has already added gcc_ld_flag (and gcc_gas_flag, in fact) to the ld/12365 test
in lto.exp, so that bit won't apply cleanly, but the second change in that
hunk (adding a '_?' prefix to the regex) is still needed.  FYI, YMMV.

ld/testsuite/ChangeLog:

2011-02-20  Dave Korn  <...

	* ld-plugin/lto-10r.d: Allow optional underscore prefixes when
	checking for known symbol name.
	* ld-plugin/lto-3r.d: Likewise.
	* ld-plugin/lto-5r.d: Likewise.
	* ld-plugin/lto-6.c: Handle non-standard etext name on Windows.
	* ld-plugin/lto.exp: Add $gcc_ld_flag carrying -B option to ld under
	test when invoking $CC directly rather than via lib support routines.


    cheers,
      DaveK

[6/6][PATCH] Respect symbol wrappers when computing symbol resolutions.

[Dave Korn]

    Hi list,

  I found this running the GCC testsuite, where on cygwin we use ld wrappers
to allow replacement of libstdc++ new/delete operators.

ld/ChangeLog:

2011-02-24  Dave Korn  <...

	* plugin.c (get_symbols): Use wrapped lookup for undefined symbols.

  I'd like it if someone can confirm that I've correctly interpreted the
comment that precedes the definition of bfd_wrapped_link_hash_lookup, about
how it should "only be used for references to an undefined symbol, not for
definitions of a symbol"; I took that to mean that I should key off the
initial state of defined-ness of the symbol passed from the plugin and call
either a wrapped or unwrapped lookup accordingly, but possibly since we're
asking about the resolution of a symbol, it would make as much sense to always
treat this as if we were looking up an undefined reference and simply always
call the wrapped lookup?

    cheers,
      DaveK

Re: [5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Dave Korn]

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

On 26/02/2011 00:46, Dave Korn wrote:

  I started forgetting to add the patches about half-way through there...


[-- Attachment #2: 005ld-link-stage2.diff --]
[-- Type: text/x-c, Size: 6601 bytes --]

From 8fb73a8834169d39ed90c5dcf25a1cdf62e95e09 Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Sun, 20 Feb 2011 00:38:54 +0000
Subject: [PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	PR ld/12365
	* ldcref.c (cref_hash_table_free): New function.
	* ld.h (cref_hash_table_free): Add prototype.
	* ldlang.c (lang_gc_sections): Dont de-exclude claimed file sections.
	(set_exclude): New function.
	(reopen_inputs): Likewise.  Walk list of input objects, excluding
	claimed (IR-only) files and archive members, then re-walk list, closing
	and re-opening and re-adding the symbols from objects and libs.
	(lang_process): After opening plugin-supplied objects and scanning
	their library dependencies, tear down existing link, cref and
	already-linked-section hashes, erase link_info input bfds list, finally
	call reopen_inputs.
	* plugin.c (plugin_opt_plugin_arg): Discard any instances of the
	now-obsolete "-pass-through=" option if found.
---
 ld/ld.h     |    1 +
 ld/ldcref.c |   18 +++++++++++
 ld/ldlang.c |   93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ld/plugin.c |    3 ++
 4 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/ld/ld.h b/ld/ld.h
index 564cb73..dfa3f2e 100644
--- a/ld/ld.h
+++ b/ld/ld.h
@@ -327,6 +327,7 @@ extern int parsing_defsym;
 
 extern int yyparse (void);
 extern void add_cref (const char *, bfd *, asection *, bfd_vma);
+extern void cref_hash_table_free (void);
 extern bfd_boolean handle_asneeded_cref (bfd *, enum notice_asneeded_action);
 extern void output_cref (FILE *);
 extern void check_nocrossrefs (void);
diff --git a/ld/ldcref.c b/ld/ldcref.c
index 2f6a46c..889b38a 100644
--- a/ld/ldcref.c
+++ b/ld/ldcref.c
@@ -198,6 +198,24 @@ add_cref (const char *name,
     r->def = TRUE;
 }
 
+/* Free the cref hash table.  */
+
+void
+cref_hash_table_free (void)
+{
+  if (cref_initialized)
+    {
+      bfd_hash_table_free (&cref_table.root);
+      cref_initialized = FALSE;
+      cref_symcount = 0;
+      old_table = NULL;
+      old_size = 0;
+      old_count = 0;
+      old_symcount = 0;
+      alloc_mark = NULL;
+    }
+}
+
 /* Called before loading an as-needed library to take a snapshot of
    the cref hash table, and after we have loaded or found that the
    library was not needed.  */
diff --git a/ld/ldlang.c b/ld/ldlang.c
index 7a1753d..7330dc0 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6269,6 +6269,10 @@ lang_gc_sections (void)
       LANG_FOR_EACH_INPUT_STATEMENT (f)
 	{
 	  asection *sec;
+#ifdef ENABLE_PLUGINS
+	  if (f->claimed)
+	    continue;
+#endif /* ENABLE_PLUGINS */
 	  for (sec = f->the_bfd->sections; sec != NULL; sec = sec->next)
 	    if ((sec->flags & SEC_DEBUGGING) == 0)
 	      sec->flags &= ~SEC_EXCLUDE;
@@ -6442,6 +6446,69 @@ find_replacements_insert_point (void)
      insert point.  */
   return lastobject;
 }
+
+/* Mapped over sections of claimed IR BFDs to exclude them from the
+   final link.  */
+
+static void
+set_exclude (bfd *abfd, asection *sect, void *obj ATTRIBUTE_UNUSED)
+{
+  bfd_set_section_flags (abfd,
+			 sect,
+			 SEC_EXCLUDE | bfd_get_section_flags (abfd, sect));
+  sect->output_section = NULL;
+}
+
+/* This runs two passes over the inputs.  First we exclude any IR
+   BFDs and archive members from the link; then we re-run it by
+   re-opening all the object files and archives and re-adding
+   their symbols to the fresh linker hash table.  */
+
+static void
+reopen_inputs (void)
+{
+  lang_input_statement_type *stmt;
+
+  /* Exclude dummy IR files and archive members.  */
+  for (stmt = &file_chain.head->input_statement;
+       stmt != NULL;
+       stmt = &stmt->next->input_statement)
+    {
+      if (stmt->claimed || (stmt->the_bfd && bfd_my_archive (stmt->the_bfd)))
+	{
+	  bfd_map_over_sections (stmt->the_bfd, set_exclude, 0);
+	  stmt->claimed = TRUE;
+	}
+    }
+
+  /* Re-open real objects and libs, recalculating link as we go.  */
+  for (stmt = &input_file_chain.head->input_statement;
+       stmt != NULL;
+       stmt = &stmt->next_real_file->input_statement)
+    {
+      if (!stmt->claimed && stmt->the_bfd && !bfd_my_archive (stmt->the_bfd))
+	{
+	  bfd *oldbfd = stmt->the_bfd;
+	  stmt->the_bfd = NULL;
+	  ldfile_open_file (stmt);
+	  bfd_check_format (stmt->the_bfd, bfd_get_format (oldbfd));
+	  if (bfd_get_format (stmt->the_bfd) != bfd_archive)
+	    {
+	      *link_info.input_bfds_tail = stmt->the_bfd;
+	      stmt->the_bfd->link_next = NULL;
+	      link_info.input_bfds_tail = &stmt->the_bfd->link_next;
+	    }
+	  stmt->the_bfd->usrdata = stmt;
+	  bfd_set_gp_size (stmt->the_bfd, g_switch_value);
+	  bfd_map_over_sections (stmt->the_bfd, section_already_linked, stmt);
+	  bfd_link_add_symbols (stmt->the_bfd, &link_info);
+	  /* We need to hold archive BFDs open while we still have statements
+	     indicating them; plain object files we are finished with now.  */
+	  if (bfd_get_format (oldbfd) == bfd_object)
+	    bfd_close (oldbfd);
+	}
+    }
+}
 #endif /* ENABLE_PLUGINS */
 
 void
@@ -6518,6 +6585,32 @@ lang_process (void)
 	  else
 	    lang_list_insert_after (&file_chain, &files, &file_chain.head);
 	}
+
+      /* Free the old already linked table and create a new one.  */
+      bfd_section_already_linked_table_free ();
+      if (!bfd_section_already_linked_table_init ())
+	einfo (_("%P%F: Failed to create hash table\n"));
+
+      /* Free the old hash table and create a new one.  */
+      bfd_link_hash_table_free (link_info.output_bfd,
+				link_info.hash);
+      link_info.hash
+	= bfd_link_hash_table_create (link_info.output_bfd);
+      if (link_info.hash == NULL)
+	einfo (_("%P%F: can not create hash table: %E\n"));
+
+      /* Re-add to the hash table all undefineds on the command line.  */
+      lang_place_undefineds ();
+
+      /* Free the old cref hash table.  */
+      cref_hash_table_free ();
+
+      /* And relink the input bfds list.  */
+      link_info.input_bfds = NULL;
+      link_info.input_bfds_tail = &link_info.input_bfds;
+
+      /* Now run round the input objects re-adding symbols.  */
+      reopen_inputs ();
     }
 #endif /* ENABLE_PLUGINS */
 
diff --git a/ld/plugin.c b/ld/plugin.c
index c1672f6..db3ad07 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -207,6 +207,9 @@ plugin_opt_plugin_arg (const char *arg)
   if (!last_plugin)
     return set_plugin_error (_("<no plugin>"));
 
+  if (CONST_STRNEQ (arg, "-pass-through="))
+    return 0;
+
   newarg = xmalloc (sizeof *newarg);
   newarg->arg = arg;
   newarg->next = NULL;

Re: [4/6][PATCH] Fix issue from GCC PR47527: no ELF flags, EABI attribs, etc. in dummy IR BFD.

[Dave Korn]

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

On 26/02/2011 00:46, Dave Korn wrote:

  ENOPATCH!


[-- Attachment #2: 004ld-pr47527.diff --]
[-- Type: text/x-c, Size: 969 bytes --]

From 6b631342632bfdfde0577465ba4e0fd543b7e463 Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Sat, 19 Feb 2011 23:48:10 +0000
Subject: [PATCH] Fix issue from GCC PR47527: no ELF flags, EABI attribs, etc. in dummy IR BFD.

ld/ChangeLog:

2011-02-20  Dave Korn  <...

	* ldlang.c (lang_check): Don't run checks on dummy IR BFDs.
---
 ld/ldlang.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/ld/ldlang.c b/ld/ldlang.c
index b36dc6e..7a1753d 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -5753,6 +5753,11 @@ lang_check (void)
 
   for (file = file_chain.head; file != NULL; file = file->input_statement.next)
     {
+#ifdef ENABLE_PLUGINS
+      /* Don't check format of files claimed by plugin.  */
+      if (file->input_statement.claimed)
+	continue;
+#endif /* ENABLE_PLUGINS */
       input_bfd = file->input_statement.the_bfd;
       compatible
 	= bfd_arch_get_compatible (input_bfd, link_info.output_bfd,

Re: [x/6][PATCH] Portability tweaks for LTO tests.

[Dave Korn]

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


  Patch.


[-- Attachment #2: 00xld-lto-test-tweaks.diff --]
[-- Type: text/x-c, Size: 2874 bytes --]

From 1dffc06f941e672e6bc68f1953834caeb5c494d9 Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Sat, 19 Feb 2011 23:02:23 +0000
Subject: [PATCH] Portability tweaks for LTO tests.

ld/testsuite/ChangeLog:

2011-02-20  Dave Korn  <...

	* ld-plugin/lto-10r.d: Allow optional underscore prefixes when
	checking for known symbol name.
	* ld-plugin/lto-3r.d: Likewise.
	* ld-plugin/lto-5r.d: Likewise.
	* ld-plugin/lto-6.c: Handle non-standard etext name on Windows.
	* ld-plugin/lto.exp: Add $gcc_ld_flag carrying -B option to ld under
	test when invoking $CC directly rather than via lib support routines.
---
 ld/testsuite/ld-plugin/lto-10r.d |    2 +-
 ld/testsuite/ld-plugin/lto-3r.d  |    2 +-
 ld/testsuite/ld-plugin/lto-5r.d  |    2 +-
 ld/testsuite/ld-plugin/lto-6.c   |    4 ++++
 ld/testsuite/ld-plugin/lto.exp   |    4 ++--
 5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/ld/testsuite/ld-plugin/lto-10r.d b/ld/testsuite/ld-plugin/lto-10r.d
index 689e6ec..c496377 100644
--- a/ld/testsuite/ld-plugin/lto-10r.d
+++ b/ld/testsuite/ld-plugin/lto-10r.d
@@ -3,5 +3,5 @@
 #nm: -p
 
 #...
-[0-9a-f]+ C __gnu_lto_v.*
+[0-9a-f]+ C _?__gnu_lto_v.*
 #pass
diff --git a/ld/testsuite/ld-plugin/lto-3r.d b/ld/testsuite/ld-plugin/lto-3r.d
index 1d1befe..3726718 100644
--- a/ld/testsuite/ld-plugin/lto-3r.d
+++ b/ld/testsuite/ld-plugin/lto-3r.d
@@ -3,5 +3,5 @@
 #nm: -p
 
 #...
-[0-9a-f]+ C __gnu_lto_v.*
+[0-9a-f]+ C _?__gnu_lto_v.*
 #pass
diff --git a/ld/testsuite/ld-plugin/lto-5r.d b/ld/testsuite/ld-plugin/lto-5r.d
index 43e9a5c..ad1da70 100644
--- a/ld/testsuite/ld-plugin/lto-5r.d
+++ b/ld/testsuite/ld-plugin/lto-5r.d
@@ -3,5 +3,5 @@
 #nm: -p
 
 #...
-[0-9a-f]+ C __gnu_lto_v.*
+[0-9a-f]+ C _?__gnu_lto_v.*
 #pass
diff --git a/ld/testsuite/ld-plugin/lto-6.c b/ld/testsuite/ld-plugin/lto-6.c
index 749e4e0..c34b805 100644
--- a/ld/testsuite/ld-plugin/lto-6.c
+++ b/ld/testsuite/ld-plugin/lto-6.c
@@ -1,6 +1,10 @@
 #include <stdio.h>
 
+#if defined(__CYGWIN32__) || defined (_WIN32)
+extern char _etext[] asm ("etext");
+#else
 extern char _etext[];
+#endif
 
 int main(void)
 {
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index 0c49cec..07aa5d0 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -201,8 +201,8 @@ if {![string match "" $catch_output]} {
 
 # Check exported LTO linker errors.
 set testname "PR ld/12365"
-set exec_output [run_host_cmd "$CC" "-O2 -flto -fuse-linker-plugin tmpdir/pr12365a.o tmpdir/pr12365b.o tmpdir/pr12365c.o"]
-if { [ regexp "undefined reference to `my_bcopy'" $exec_output ] } {
+set exec_output [run_host_cmd "$CC" "$gcc_ld_flag -O2 -flto -fuse-linker-plugin tmpdir/pr12365a.o tmpdir/pr12365b.o tmpdir/pr12365c.o"]
+if { [ regexp "undefined reference to `_?my_bcopy'" $exec_output ] } {
     pass $testname
 } {
     fail $testname

Re: [6/6][PATCH] Respect symbol wrappers when computing symbol resolutions.

[Dave Korn]

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


  The missing diff.


[-- Attachment #2: 006ld-plugin-sym-wrappers.diff --]
[-- Type: text/x-c, Size: 1145 bytes --]

From f1fdd296f2f4aac99a4ff959b0794f59a5557960 Mon Sep 17 00:00:00 2001
From: Dave Korn <dave.korn.cygwin@gmail.com>
Date: Thu, 24 Feb 2011 04:58:40 +0000
Subject: [PATCH] Respect symbol wrappers when computing symbol resolutions.

ld/ChangeLog:

2011-02-24  Dave Korn  <...

	* plugin.c (get_symbols): Use wrapped lookup for undefined symbols.
---
 ld/plugin.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/ld/plugin.c b/ld/plugin.c
index db3ad07..f9ee9f1 100644
--- a/ld/plugin.c
+++ b/ld/plugin.c
@@ -456,9 +456,12 @@ get_symbols (const void *handle, int nsyms, struct ld_plugin_symbol *syms)
       struct bfd_link_hash_entry *blhe;
       bfd_boolean ironly;
       asection *owner_sec;
-
-      blhe = bfd_link_hash_lookup (link_info.hash, syms[n].name,
-				   FALSE, FALSE, TRUE);
+      if (syms[n].def != LDPK_UNDEF)
+	blhe = bfd_link_hash_lookup (link_info.hash, syms[n].name,
+				     FALSE, FALSE, TRUE);
+      else
+	blhe = bfd_wrapped_link_hash_lookup (link_info.output_bfd, &link_info,
+					     syms[n].name, FALSE, FALSE, TRUE);
       if (!blhe)
 	{
 	  syms[n].resolution = LDPR_UNKNOWN;

Re: [PATCH,trunk+2.20] Fix issues in ld.bfd plugin interface [0/6]

[H.J. Lu]

On Fri, Feb 25, 2011 at 4:43 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
>
>  After my patches, LD gives the following results:
>
>> FAIL: ld-plugin/lto-4r-a
>> FAIL: ld-plugin/lto-4r-b
>> FAIL: ld-plugin/lto-4r-c
>> FAIL: ld-plugin/lto-4r-d
>> FAIL: LTO 4a
>> FAIL: LTO 4c
>> FAIL: LTO 4d
>> FAIL: LTO 10
>> FAIL: LTO 12a
>> FAIL: LTO 12b
>> FAIL: LTO 15
>
>  Relative to current LD, that has fixes to 12365, 5 symbol, 3b and 5.  The
> new FAILures on 12a/b and 15 are actually a difference of interpretation about
> how the linker is supposed to work between me and HJ's code.  The tests assume
> that it is not possible to override builtins such as __udivdi3 or memcpy, but
> it works OK with the attached patches, so I'm not sure why these tests do what
> they do.  For example, test LTO 15 has this source file compiled as an object:
>
>> #include <stdio.h>
>>
>> int main(int argc, char **argv)
>> {
>>   printf ("PASS\n");
>>   return (int) ((unsigned long long) argc / argv[0][0]);
>> }
>
> ... and this one in a library archive member:
>
>> extern void abort (void);
>> unsigned long long
>> __udivdi3(unsigned long long n, unsigned long long d)
>> {
>>   abort ();
>>   return n + d;
>> }
>
> ... so I would have thought that the ideal case would be for an LTO link to
> mimic a non-LTO link, and pull in the definition of __udivdi3 there and abort.
>
>  That aside, the only thing that LD still doesn't handle after these patches
> is mixed object files that are created by using "ld -r" to merge multiple IR
> and non-IR object files.
>

The question is if LTO linker should link in non-LTO copy of the same function
and ignore the LTO-IR copy:

[hjl@gnu-6 pr12496-2]$ make
as --32  -o start.o start.s
/usr/gcc-4.6/bin/gcc -m32 -B./ -flto   -c -o main.o main.c
/usr/gcc-4.6/bin/gcc -m32 -B./ -flto   -c -o div.o div.c
ar rv libdiv.a div.o
ar: creating libdiv.a
a - div.o
/usr/gcc-4.6/bin/gcc -m32 -B./ -O2 -flto -nostdlib -o prog
-Wl,--start-group start.o main.o libdiv.a -Wl,--end-group

Here div.o is compiled with -O0 -flto and the final link is compiled
with -O2 -flto. I would expect div.c is compiled with -O2 -flto, not -O0,
if div.c is linked in. Am I expecting too much?


-- 
H.J.

Re: [PATCH,trunk+2.20] Fix issues in ld.bfd plugin interface [0/6]

[Dave Korn]

On 26/02/2011 02:14, H.J. Lu wrote:

> 
> The question is if LTO linker should link in non-LTO copy of the same function
> and ignore the LTO-IR copy:

  Well, I think the linker should link at least one of them in, preferably the
IR one if it can so that it can be optimised along with the rest of the
program, but if for some reason the linker can't see that it's needed until
after ltrans, then it has no choice but to link the non-LTO copy, and settle
for what should after all only be a missed optimisation rather than
correctness problem.

> [hjl@gnu-6 pr12496-2]$ make
> as --32  -o start.o start.s
> /usr/gcc-4.6/bin/gcc -m32 -B./ -flto   -c -o main.o main.c
> /usr/gcc-4.6/bin/gcc -m32 -B./ -flto   -c -o div.o div.c
> ar rv libdiv.a div.o
> ar: creating libdiv.a
> a - div.o
> /usr/gcc-4.6/bin/gcc -m32 -B./ -O2 -flto -nostdlib -o prog
> -Wl,--start-group start.o main.o libdiv.a -Wl,--end-group

  When I tried this with ld.hjl on i686-pc-cygwin, I got an undefined
reference to __udivdi3, is that supposed to happen?

> Here div.o is compiled with -O0 -flto and the final link is compiled
> with -O2 -flto. I would expect div.c is compiled with -O2 -flto, not -O0,
> if div.c is linked in. Am I expecting too much?

  Well, to make that work, wouldn't we have to treat all archives as if they
were opened in --whole-archive mode during stage1, and let the plugin claim
every archive member that contains any IR and then rely on LTRANS to optimise
away all but the used code, right?

  I must admit I don't understand why LTO doesn't emit undefs in the LTO
symtab for builtins.  I think life would be easier all round if the LTO symtab
for main.o contained a reference to __udivdi3 all along, so that we could just
let the usual mechanism pull in all (and only) the needed stuff straight off
in stage1 and not give LTRANS so much redundant work to do.

    cheers,
      DaveK

Re: [PATCH,trunk+2.20] Fix issues in ld.bfd plugin interface [0/6]

[H.J. Lu]

On Fri, Feb 25, 2011 at 7:36 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
> On 26/02/2011 02:14, H.J. Lu wrote:
>
>>
>> The question is if LTO linker should link in non-LTO copy of the same function
>> and ignore the LTO-IR copy:
>
>  Well, I think the linker should link at least one of them in, preferably the
> IR one if it can so that it can be optimised along with the rest of the
> program, but if for some reason the linker can't see that it's needed until
> after ltrans, then it has no choice but to link the non-LTO copy, and settle
> for what should after all only be a missed optimisation rather than
> correctness problem.

The fact that linker doesn't link in the LTO copy means that the non-LTO copy
isn't required by GCC.

>> [hjl@gnu-6 pr12496-2]$ make
>> as --32  -o start.o start.s
>> /usr/gcc-4.6/bin/gcc -m32 -B./ -flto   -c -o main.o main.c
>> /usr/gcc-4.6/bin/gcc -m32 -B./ -flto   -c -o div.o div.c
>> ar rv libdiv.a div.o
>> ar: creating libdiv.a
>> a - div.o
>> /usr/gcc-4.6/bin/gcc -m32 -B./ -O2 -flto -nostdlib -o prog
>> -Wl,--start-group start.o main.o libdiv.a -Wl,--end-group
>
>  When I tried this with ld.hjl on i686-pc-cygwin, I got an undefined
> reference to __udivdi3, is that supposed to happen?

That is also my question. What should happen if div.o only has IR?
My take on this is this is a user error.

>> Here div.o is compiled with -O0 -flto and the final link is compiled
>> with -O2 -flto. I would expect div.c is compiled with -O2 -flto, not -O0,
>> if div.c is linked in. Am I expecting too much?
>
>  Well, to make that work, wouldn't we have to treat all archives as if they
> were opened in --whole-archive mode during stage1, and let the plugin claim
> every archive member that contains any IR and then rely on LTRANS to optimise
> away all but the used code, right?
>
>  I must admit I don't understand why LTO doesn't emit undefs in the LTO
> symtab for builtins.  I think life would be easier all round if the LTO symtab
> for main.o contained a reference to __udivdi3 all along, so that we could just
> let the usual mechanism pull in all (and only) the needed stuff straight off
> in stage1 and not give LTRANS so much redundant work to do.
>

That will be wrong since LTO may not need to call __udivdi3 if it can
be folded.

-- 
H.J.

Re: [x/6][PATCH] Portability tweaks for LTO tests.

[H.J. Lu]

On Fri, Feb 25, 2011 at 4:47 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
>
>    Hi list,
>
>  This is out of the main series, because it's a couple of tweaks to the LTO
> tests in HJ's branch; included here for reference, since I've been running
> tests with it folded in.  I notice it's slightly outdated here, as upstream HJ
> has already added gcc_ld_flag (and gcc_gas_flag, in fact) to the ld/12365 test
> in lto.exp, so that bit won't apply cleanly, but the second change in that
> hunk (adding a '_?' prefix to the regex) is still needed.  FYI, YMMV.
>
> ld/testsuite/ChangeLog:
>
> 2011-02-20  Dave Korn  <...
>
>        * ld-plugin/lto-10r.d: Allow optional underscore prefixes when
>        checking for known symbol name.
>        * ld-plugin/lto-3r.d: Likewise.
>        * ld-plugin/lto-5r.d: Likewise.
>        * ld-plugin/lto-6.c: Handle non-standard etext name on Windows.
>        * ld-plugin/lto.exp: Add $gcc_ld_flag carrying -B option to ld under
>        test when invoking $CC directly rather than via lib support routines.
>
>

I added a new test LTO-16 into my branch.  If you send me a patch against
commit d33f411f8648104187e369c99d71204e54303bbd, I will
include it on  my branch.

Thanks.


-- 
H.J.

Re: [5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Rafael Ávila de Espíndola]

On 2011-02-25 19:46, Dave Korn wrote:
> 
>     Hi list,
> 
>   This is the second main functional change, and it's really the only way to
> resolve the problems caused by discrepancies between the initial set of
> symbols returned by the plugin based on the LTO symtabs in the IR files, and
> the actual set of symbols used and defined by the eventual object file(s)
> added to the link after LTRANS.  It's largely the same approach as HJ's
> 2-stage link, except that it closes and reopens the existing input BFDs in
> place, rather than adding a second set of input statements.  This results in
> some different behaviour between HJ's linker and this patched version.
> 
> 
> ld/ChangeLog:
> 
> 2011-02-20  Dave Korn  <...
> 
> 	PR ld/12365
> 	* ldcref.c (cref_hash_table_free): New function.
> 	* ld.h (cref_hash_table_free): Add prototype.
> 	* ldlang.c (lang_gc_sections): Dont de-exclude claimed file sections.
> 	(set_exclude): New function.
> 	(reopen_inputs): Likewise.  Walk list of input objects, excluding
> 	claimed (IR-only) files and archive members, then re-walk list, closing
> 	and re-opening and re-adding the symbols from objects and libs.
> 	(lang_process): After opening plugin-supplied objects and scanning
> 	their library dependencies, tear down existing link, cref and
> 	already-linked-section hashes, erase link_info input bfds list, finally
> 	call reopen_inputs.
> 	* plugin.c (plugin_opt_plugin_arg): Discard any instances of the
> 	now-obsolete "-pass-through=" option if found.
> 
>   I know Ian would rather avoid this approach, but I don't think that there's
> any other way to do LTO in LD without either rearchitecting BFD quite a bit.
> We can't do symbol resolution without adding symbols from the LTO symtabs into
> the linker hash table, but then after LTRANS we may have a different final set
> of symbols present.  There's no way to excise individual symbols from the
> linker hash table in BFD, so you have to achieve the same effect by tearing
> down the symbol table and rebuilding it without adding the unwanted symbols at
> all second time round.  That means you have to close and reopen all the BFDs,
> because BFD backends cache pointers to hash table entries in the bfd's private
> data, which are now stale once you've built a new symbol table.  I can't see
> any way around having to re-do at least this amount of work, but without it
> we'll get bogus symbols in our final outputs, which is a real correctness issue.

Ian,

Assuming this is ok for bfd, would a patch implementing the same logic
be ok for gold? It would be bad to have different interfaces in bfd ld
and gold.

>     cheers,
>       DaveK
> 

Cheers,
Rafael

Re: [6/6][PATCH] Respect symbol wrappers when computing symbol resolutions.

[H.J. Lu]

On Fri, Feb 25, 2011 at 4:46 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
>
>    Hi list,
>
>  I found this running the GCC testsuite, where on cygwin we use ld wrappers
> to allow replacement of libstdc++ new/delete operators.
>
> ld/ChangeLog:
>
> 2011-02-24  Dave Korn  <...
>
>        * plugin.c (get_symbols): Use wrapped lookup for undefined symbols.
>
>  I'd like it if someone can confirm that I've correctly interpreted the
> comment that precedes the definition of bfd_wrapped_link_hash_lookup, about
> how it should "only be used for references to an undefined symbol, not for
> definitions of a symbol"; I took that to mean that I should key off the
> initial state of defined-ness of the symbol passed from the plugin and call
> either a wrapped or unwrapped lookup accordingly, but possibly since we're
> asking about the resolution of a symbol, it would make as much sense to always
> treat this as if we were looking up an undefined reference and simply always
> call the wrapped lookup?
>

You can add a testcase to LTO tests on lto-mixed branch.


-- 
H.J.

Re: [x/6][PATCH] Portability tweaks for LTO tests.

[H.J. Lu]

On Fri, Feb 25, 2011 at 7:49 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Feb 25, 2011 at 4:47 PM, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
>>
>>    Hi list,
>>
>>  This is out of the main series, because it's a couple of tweaks to the LTO
>> tests in HJ's branch; included here for reference, since I've been running
>> tests with it folded in.  I notice it's slightly outdated here, as upstream HJ
>> has already added gcc_ld_flag (and gcc_gas_flag, in fact) to the ld/12365 test
>> in lto.exp, so that bit won't apply cleanly, but the second change in that
>> hunk (adding a '_?' prefix to the regex) is still needed.  FYI, YMMV.
>>
>> ld/testsuite/ChangeLog:
>>
>> 2011-02-20  Dave Korn  <...
>>
>>        * ld-plugin/lto-10r.d: Allow optional underscore prefixes when
>>        checking for known symbol name.
>>        * ld-plugin/lto-3r.d: Likewise.
>>        * ld-plugin/lto-5r.d: Likewise.
>>        * ld-plugin/lto-6.c: Handle non-standard etext name on Windows.
>>        * ld-plugin/lto.exp: Add $gcc_ld_flag carrying -B option to ld under
>>        test when invoking $CC directly rather than via lib support routines.
>>
>>
>
> I added a new test LTO-16 into my branch.  If you send me a patch against
> commit d33f411f8648104187e369c99d71204e54303bbd, I will
> include it on  my branch.
>

I added a new testcase to my branch. Please use
commit 45a499bc8753fe2c3c32aab5cff4a9493ffdf006.


-- 
H.J.

Re: [5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Alan Modra]

On Sat, Feb 26, 2011 at 12:46:33AM +0000, Dave Korn wrote:
> There's no way to excise individual symbols from the
> linker hash table in BFD,

We do have a simple option that prevents LTO symbols being emitted.
http://sourceware.org/ml/binutils/2011-02/msg00037.html

-- 
Alan Modra
Australia Development Lab, IBM

Re: [1/6][PATCH] Fix PE-COFF bug in orphan section alignment handling.

[Alan Modra]

On Sat, Feb 26, 2011 at 12:45:16AM +0000, Dave Korn wrote:
> +      if (link_info.relocatable)
> +	os->bfd_section->alignment_power = os->section_alignment
> +					 = s->alignment_power;

Style and formatting issues here.  Better written as

      if (link_info.relocatable)
	{
	  os->section_alignment = s->alignment_power;
	  os->bfd_section->alignment_power = s->alignment_power;
	}

-- 
Alan Modra
Australia Development Lab, IBM

Re: [3/6][PATCH] Revise linker plugin API to better preserve link order.

[Alan Modra]

On Sat, Feb 26, 2011 at 12:45:57AM +0000, Dave Korn wrote:
> 	* ldlang.h (lang_input_statement_type): Add new 'claim_archive' flag,
> 	wrapping both it and 'claim' flag in #ifdef ENABLE_PLUGINS.
> 	* ldmain.c (add_archive_element): Set it if the member is claimed.
> 	* ldlang.c (new_afile): Initialise claim_archive and claimed members.
> 	(find_replacements_insert_point): New helper function.
> 	(lang_process): After adding and opening replacement files passed
> 	from plugin, splice them into correct place in statement list and
> 	file chains to preserve critical link order.
> 	(lang_list_insert_after): New helper function.
> 	(lang_list_remove_tail): Likewise.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [4/6][PATCH] Fix issue from GCC PR47527: no ELF flags, EABI attribs, etc. in dummy IR BFD.

[Alan Modra]

On Sat, Feb 26, 2011 at 12:48:03AM +0000, Dave Korn wrote:
> 	* ldlang.c (lang_check): Don't run checks on dummy IR BFDs.

OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [6/6][PATCH] Respect symbol wrappers when computing symbol resolutions.

[Alan Modra]

On Sat, Feb 26, 2011 at 12:46:49AM +0000, Dave Korn wrote:
> 	* plugin.c (get_symbols): Use wrapped lookup for undefined symbols.

OK.

>   I'd like it if someone can confirm that I've correctly interpreted the
> comment that precedes the definition of bfd_wrapped_link_hash_lookup, about
> how it should "only be used for references to an undefined symbol, not for
> definitions of a symbol"; I took that to mean that I should key off the
> initial state of defined-ness of the symbol passed from the plugin and call
> either a wrapped or unwrapped lookup accordingly

Yes, I think that is correct.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Ian Lance Taylor]

Rafael Ávila de Espíndola <respindola@mozilla.com> writes:

> Assuming this is ok for bfd, would a patch implementing the same logic
> be ok for gold? It would be bad to have different interfaces in bfd ld
> and gold.

What is the actual difference in behaviour?

I am going to continue to argue strenuously against rescanning all the
symbol tables of the input files in gold.  If GNU ld wants to rescan all
the symbol tables, I suppose that's OK with me, as long as gold is not
required to do it.

Ian

Re: [5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Rafael Ávila de Espíndola]

> What is the actual difference in behaviour?

The main one is the requirement for -pass-through=. A quick summary of
the possible ways of handling mixed IL/ELF files when given a sequence
like ELF IL IL ELF IL IL ELF is

1) Combine every contiguous IL file sequence (ELF COMBINED_ELF1 ELF
COMBINED_ELF2 ELF) and conceptually restart the link. This would provide
the least differences when compared to an all ELF link.

2) Combine every IL file into a single ELF, put it in the place of the
first IL file (ELF COMBINED_ELF ELF ELF) and conceptually restart the
link. If I understand it correctly, this is where the bfd ld is going.

3) Same as before, but put the combined elf file in the end.

4) Same as before, but do not restart the link. This is what gold does
currently and requires the -pass-through option.

> I am going to continue to argue strenuously against rescanning all the
> symbol tables of the input files in gold.  If GNU ld wants to rescan all
> the symbol tables, I suppose that's OK with me, as long as gold is not
> required to do it.

Can you think of a way that we can drop just the requirement of the
-pass-through option? Handling the strange case of user defined versions
of libgcc functions doesn't look as important.

> Ian

Cheers,
Rafael

Re: [5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Ian Lance Taylor]

Rafael Ávila de Espíndola <respindola@mozilla.com> writes:

>> What is the actual difference in behaviour?
>
> The main one is the requirement for -pass-through=.

It is already the case that gold does not require -pass-through=.  In
fact, I've already modified the gcc plugin to ignore -pass-through= when
using gold version 1.11 or greater (gold 1.11 will be in the binutils
2.21.1 release).

> A quick summary of
> the possible ways of handling mixed IL/ELF files when given a sequence
> like ELF IL IL ELF IL IL ELF is
>
> 1) Combine every contiguous IL file sequence (ELF COMBINED_ELF1 ELF
> COMBINED_ELF2 ELF) and conceptually restart the link. This would provide
> the least differences when compared to an all ELF link.
>
> 2) Combine every IL file into a single ELF, put it in the place of the
> first IL file (ELF COMBINED_ELF ELF ELF) and conceptually restart the
> link. If I understand it correctly, this is where the bfd ld is going.
>
> 3) Same as before, but put the combined elf file in the end.
>
> 4) Same as before, but do not restart the link. This is what gold does
> currently and requires the -pass-through option.

I would say that right now gold approximately implements your option 2
(not option 4) and does not require the -pass-through= option.  If the
plugin does not introduce any new symbol definitions in the single ELF
file that it creates, new symbol definitions which were not previously
returned as IL symbols, then gold exactly implements option 2 (it's OK
for the plugin to introduce new symbol references, gold will handle
those correctly).  New symbol definitions will not be handled correctly
in the case where the symbol was previously defined by a .o or .a file
seen after the first IL file.  However, I think that is OK because I
think it would be very dubious for a plugin to create an object file
with a new symbol definition which the plugin did not previously report.

Ian

Re: [5/6][PATCH] Perform second link stage and ignore now-obsolete linker -pass-through= option.

[Rafael Ávila de Espíndola]

> It is already the case that gold does not require -pass-through=.  In
> fact, I've already modified the gcc plugin to ignore -pass-through= when
> using gold version 1.11 or greater (gold 1.11 will be in the binutils
> 2.21.1 release).

I had completely missed the addition of Plugin_rescan! I agree that gold
is now probably in the performance/feature sweet spot. Sorry about the
noise.

> Ian

Thanks,
Rafael

Re: [PATCH,trunk+2.20] Fix issues in ld.bfd plugin interface [0/6]

[Dave Korn]

On 26/02/2011 00:43, Dave Korn wrote:

> * 001ld-pe-orphan-section-alignment.diff
> * 002ld-plugin-api-no-ironly-suffix.diff
> * 003ld-plugin-api-link-order.diff
> * 004ld-pr47527.diff
> * 006ld-plugin-sym-wrappers.diff

  Commmitted these uncontroversial ones.

> * 005ld-link-stage2.diff

  More to come on this one.

    cheers,
      DaveK