This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[PATCH,take 2,trunk+2.21.1] Re: Fix link order problem with LD plugin API.
On 29/01/2011 04:41, H.J. Lu wrote:
> On Fri, Jan 28, 2011 at 8:37 PM, Dave Korn wrote:
>> On 29/01/2011 02:05, H.J. Lu wrote:
>>
>>> I run your patch on those new LTO tests. I got relevant failures:
>>>
>>> FAIL: LTO 2
>>> FAIL: LTO 11
>>> FAIL: LTO 8
>>>
>>> Your patch doesn't fix any failures and introduce a new one.
>> It's not too bad. The new fail is the assert firing where it can't find a
>> node to insert after when the only claimed objects come from a library; at the
>> moment it's checking the claimed flag only for the overall archive, which is
>> never set. I'll just make it check the claimed flag for the archive members
>> and place the added files after the original archive in that case.
>>
>> LTO 2 passes both before and after on PE-COFF, and LTO 8 is ELF-only, so I'm
>> building the whole thing up on cfarm to see what's going on with those. You
>> didn't mention your target - x86_64-linux, I'm guessing?
>>
>
> I tested it on Linux/x86.
Right you are. This updated version is cleverer about where it inserts the
replacement files into the statement list and file chains.
ld/ChangeLog:
2011-01-29 Dave Korn <...
* ldlang.h (lang_input_statement_type): Add new 'claim_archive' flag.
* 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.
I tested this one on i686-unknown-linux-gnu. LTO 2 still fails, because
there's no pass-through for -lm. LTO 8 still fails, because we're still
generating undefined references to symbols from the original IR file's symtab
that aren't supplied by the replacement file, which runs afoul of the elf linker:
> /* If a non-weak symbol with non-default visibility is not defined
> locally, it is a fatal error. */
... despite the fact that there are no references to the symbol in question.
Neither of those is to do with the link order though, so this patch wouldn't
be expected to fix them. The one real bug, LTO 11 is OK now.
On i686-pc-cygwin, LTO 2 and LTO 11 pass (as in fact do LTO 7 and LTO 8;
they're not as ELF-specific as all that, after all). I've run
g++.dg/stackalign.exp from the gcc testsuite, and all the EH failures I was
previously experiencing are fixed. I'll run a full C/C++ testsuite later.
I also tested it on x86_64-unknown-linux-gnu, including your ld-lto tests
and a gcc testsuite run (c,c++ only); no regressions in any of that.
OK now?
cheers,
DaveK
Index: ld/ldlang.h
===================================================================
RCS file: /cvs/src/src/ld/ldlang.h,v
retrieving revision 1.95
diff -p -u -r1.95 ldlang.h
--- ld/ldlang.h 13 Jan 2011 13:34:54 -0000 1.95
+++ ld/ldlang.h 31 Jan 2011 00:41:22 -0000
@@ -290,6 +290,9 @@ typedef struct lang_input_statement_stru
/* 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;
+
} lang_input_statement_type;
typedef struct
Index: ld/ldmain.c
===================================================================
RCS file: /cvs/src/src/ld/ldmain.c,v
retrieving revision 1.148
diff -p -u -r1.148 ldmain.c
--- ld/ldmain.c 14 Jan 2011 12:37:17 -0000 1.148
+++ ld/ldmain.c 31 Jan 2011 00:41:22 -0000
@@ -834,6 +834,7 @@ add_archive_element (struct bfd_link_inf
/* 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;
}
Index: ld/ldlang.c
===================================================================
RCS file: /cvs/src/src/ld/ldlang.c,v
retrieving revision 1.360
diff -p -u -r1.360 ldlang.c
--- ld/ldlang.c 14 Jan 2011 02:18:22 -0000 1.360
+++ ld/ldlang.c 31 Jan 2011 00:41:23 -0000
@@ -85,6 +85,11 @@ static void print_statement_list (lang_s
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 *);
+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);
static void lang_record_phdrs (void);
static void lang_do_version_exports_section (void);
static void lang_finalize_version_expr_head
@@ -1126,6 +1131,8 @@ new_afile (const char *name,
p->whole_archive = whole_archive;
p->loaded = FALSE;
p->missing_file = FALSE;
+ p->claimed = FALSE;
+ p->claim_archive = FALSE;
lang_statement_append (&input_file_chain,
(lang_statement_union_type *) p,
@@ -6394,6 +6401,38 @@ lang_relax_sections (bfd_boolean need_la
}
}
+/* 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;
+}
+
void
lang_process (void)
{
@@ -6420,21 +6459,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 +6929,50 @@ lang_statement_append (lang_statement_li
list->tail = field;
}
+/* 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)
+{
+ /* Are we adding at the very end of the list? */
+ if (*field == NULL)
+ {
+ /* (*field == NULL) should imply (destlist->tail == field),
+ if not then the element isn't really in the DESTLIST. */
+ ASSERT (destlist->tail == field);
+ /* Yes, append to and update tail pointer. */
+ *(destlist->tail) = srclist->head;
+ destlist->tail = srclist->tail;
+ }
+ else
+ {
+ /* We're inserting in the middle somewhere. */
+ *(srclist->tail) = *field;
+ *field = srclist->head;
+ }
+}
+
+/* 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;
+}
+
/* Set the output format type. -oformat overrides scripts. */
void