This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: PATCH: 2 stage BFD linker for LTO plugin
Alan Modra <amodra@gmail.com> writes:
> On Mon, Dec 06, 2010 at 09:57:14AM -0800, H.J. Lu wrote:
>> Personally, I think 2 stage linking is one way to fix this issue.
>
> Ian has stated that he thinks this is a really bad idea. I haven't
> approved the patch because I value Ian's opinion, and can see why he
> thinks it is the wrong way to go. On the other hand, BFD is full of
> bad ideas.. I'm not strongly opposed to your patch myself.
Here is an alternative proposal, with a patch for gold.
We add a new plugin vector: LDPT_REGISTER_RESCAN_ARCHIVE_HOOK. Like
LDPT_REGISTER_CLAIM_FILE_HOOK, this gives the plugin the address of a
function which can register a plugin function: rescan_archive.
typedef
enum ld_plugin_status
(*ld_plugin_rescan_archive_handler) (
const struct ld_plugin_input_file *file, int *rescan);
If the plugin registers this hook, the linker will call the hook for
each archive that it sees. If the hook sets the *rescan variable to a
non-zero value, then the linker will rescan the archive after calling
the all_symbols_read hook. The archive will be rescanned using the same
position dependent options as when it was originally scanned. In
particular, if the archive occurred within --start-group/--end-group,
the entire group will be rescanned.
The point of this patch is that the known problems with the LTO plugin
are when the plugin introduces a previously unknown symbol which must be
satisfied from some archive. The new symbol is introduced when the
all_symbols_hook read hook calls add_input_file to add a new object file
to the link, and the new object file refers to the new symbol. Since
the symbol was not previously seen, if the definition should come from
an archive, the archive will not have been searched. Hence the test
case in GCC PR 42690 comment #32.
Fortunately, we know that gcc is not going to introduce arbitrary new
symbol references. The current system hacks around the problem by using
a special -pass-through option which directs the plugin to add specific
archives to the link, namely -lc and -lgcc. That works for most cases,
but not when gcc introduces a symbol which comes from -lm. Still,
despite the -lm issue, we do not have to concern ourselves with
arbitrary archives, because gcc is not going to act arbitrarily.
Also we know that the problem can only occur with archives, not with
shared objects.
The rescan_archive proposal fixes the problems and obviates the need to
use -pass-through. It avoids the unnecessary cost of a complete relink.
I've appended a patch for gold which implements this proposal. I've
also appended a patch for lto-plugin. This patched lto-plugin does not
use -pass-through when rescan_archive is available. It rescans libc.a,
libgcc.a, and libm.a. It handles the PR 42690 comment #32 test case
correctly.
Any thoughts on this approach?
Ian
Index: include/plugin-api.h
===================================================================
RCS file: /cvs/src/src/include/plugin-api.h,v
retrieving revision 1.12
diff -p -u -r1.12 plugin-api.h
--- include/plugin-api.h 14 Oct 2010 01:31:28 -0000 1.12
+++ include/plugin-api.h 13 Dec 2010 20:53:16 -0000
@@ -163,6 +163,13 @@ typedef
enum ld_plugin_status
(*ld_plugin_all_symbols_read_handler) (void);
+/* The plugin library's "rescan archive" handler. */
+
+typedef
+enum ld_plugin_status
+(*ld_plugin_rescan_archive_handler) (
+ const struct ld_plugin_input_file *file, int *rescan);
+
/* The plugin library's cleanup handler. */
typedef
@@ -182,6 +189,13 @@ enum ld_plugin_status
(*ld_plugin_register_all_symbols_read) (
ld_plugin_all_symbols_read_handler handler);
+/* The linker's interface for registering the "rescan archive" handler. */
+
+typedef
+enum ld_plugin_status
+(*ld_plugin_register_rescan_archive) (
+ ld_plugin_rescan_archive_handler handler);
+
/* The linker's interface for registering the cleanup handler. */
typedef
@@ -269,7 +283,8 @@ enum ld_plugin_tag
LDPT_ADD_INPUT_LIBRARY,
LDPT_OUTPUT_NAME,
LDPT_SET_EXTRA_LIBRARY_PATH,
- LDPT_GNU_LD_VERSION
+ LDPT_GNU_LD_VERSION,
+ LDPT_REGISTER_RESCAN_ARCHIVE_HOOK
};
/* The plugin transfer vector. */
@@ -283,6 +298,7 @@ struct ld_plugin_tv
const char *tv_string;
ld_plugin_register_claim_file tv_register_claim_file;
ld_plugin_register_all_symbols_read tv_register_all_symbols_read;
+ ld_plugin_register_rescan_archive tv_register_rescan_archive;
ld_plugin_register_cleanup tv_register_cleanup;
ld_plugin_add_symbols tv_add_symbols;
ld_plugin_get_symbols tv_get_symbols;
Index: plugin.cc
===================================================================
RCS file: /cvs/src/src/gold/plugin.cc,v
retrieving revision 1.41
diff -p -u -r1.41 plugin.cc
--- plugin.cc 5 Nov 2010 21:14:12 -0000 1.41
+++ plugin.cc 13 Dec 2010 20:52:15 -0000
@@ -60,6 +60,9 @@ static enum ld_plugin_status
register_all_symbols_read(ld_plugin_all_symbols_read_handler handler);
static enum ld_plugin_status
+register_rescan_archive(ld_plugin_rescan_archive_handler handler);
+
+static enum ld_plugin_status
register_cleanup(ld_plugin_cleanup_handler handler);
static enum ld_plugin_status
@@ -130,7 +133,7 @@ Plugin::load()
sscanf(ver, "%d.%d", &major, &minor);
// Allocate and populate a transfer vector.
- const int tv_fixed_size = 16;
+ const int tv_fixed_size = 17;
int tv_size = this->args_.size() + tv_fixed_size;
ld_plugin_tv* tv = new ld_plugin_tv[tv_size];
@@ -209,6 +212,10 @@ Plugin::load()
tv[i].tv_u.tv_set_extra_library_path = set_extra_library_path;
++i;
+ tv[i].tv_tag = LDPT_REGISTER_RESCAN_ARCHIVE_HOOK;
+ tv[i].tv_u.tv_register_rescan_archive = register_rescan_archive;
+
+ ++i;
tv[i].tv_tag = LDPT_NULL;
tv[i].tv_u.tv_val = 0;
@@ -246,6 +253,21 @@ Plugin::all_symbols_read()
(*this->all_symbols_read_handler_)();
}
+// Call the rescan-archive handler.
+
+inline bool
+Plugin::rescan_archive(struct ld_plugin_input_file* plugin_input_file)
+{
+ if (this->rescan_archive_handler_ != NULL)
+ {
+ int rescan = 0;
+ (*this->rescan_archive_handler_)(plugin_input_file, &rescan);
+ if (rescan)
+ return true;
+ }
+ return false;
+}
+
// Call the cleanup handler.
inline void
@@ -324,6 +346,34 @@ Plugin_manager::claim_file(Input_file* i
return NULL;
}
+// Call the plugin rescan-archive handlers in turn to see if any want
+// to rescan the archive.
+
+bool
+Plugin_manager::rescan_archive(Input_file* input_file, off_t offset,
+ off_t filesize)
+{
+ if (this->in_replacement_phase_)
+ return false;
+
+ this->input_file_ = input_file;
+ this->plugin_input_file_.name = input_file->filename().c_str();
+ this->plugin_input_file_.fd = input_file->file().descriptor();
+ this->plugin_input_file_.offset = offset;
+ this->plugin_input_file_.filesize = filesize;
+ this->plugin_input_file_.handle = NULL;
+
+ for (this->current_ = this->plugins_.begin();
+ this->current_ != this->plugins_.end();
+ ++this->current_)
+ {
+ if ((*this->current_)->rescan_archive(&this->plugin_input_file_))
+ return true;
+ }
+
+ return false;
+}
+
// Call the all-symbols-read handlers.
void
@@ -348,6 +398,27 @@ Plugin_manager::all_symbols_read(Workque
++this->current_)
(*this->current_)->all_symbols_read();
+ for (std::vector<const Input_argument*>::const_iterator p =
+ this->rescan_.begin();
+ p != this->rescan_.end();
+ ++p)
+ {
+ Task_token* next_blocker = new Task_token(true);
+ next_blocker->add_blocker();
+ workqueue->queue_soon(new Read_symbols(input_objects,
+ symtab,
+ layout,
+ dirpath,
+ 0,
+ mapfile,
+ *p,
+ NULL,
+ NULL,
+ this->this_blocker_,
+ next_blocker));
+ this->this_blocker_ = next_blocker;
+ }
+
*last_blocker = this->this_blocker_;
}
@@ -994,6 +1065,16 @@ register_all_symbols_read(ld_plugin_all_
return LDPS_OK;
}
+// Register a rescan-archive handler.
+
+static enum ld_plugin_status
+register_rescan_archive(ld_plugin_rescan_archive_handler handler)
+{
+ gold_assert(parameters->options().has_plugins());
+ parameters->options().plugins()->set_rescan_archive_handler(handler);
+ return LDPS_OK;
+}
+
// Register a cleanup handler.
static enum ld_plugin_status
Index: plugin.h
===================================================================
RCS file: /cvs/src/src/gold/plugin.h,v
retrieving revision 1.15
diff -p -u -r1.15 plugin.h
--- plugin.h 25 Aug 2010 08:36:54 -0000 1.15
+++ plugin.h 13 Dec 2010 20:52:15 -0000
@@ -54,6 +54,7 @@ class Plugin
args_(),
claim_file_handler_(NULL),
all_symbols_read_handler_(NULL),
+ rescan_archive_handler_(NULL),
cleanup_handler_(NULL),
cleanup_done_(false)
{ }
@@ -73,6 +74,11 @@ class Plugin
void
all_symbols_read();
+ // Call the rescan-archive handler. Return true if the archive
+ // should be rescanned.
+ bool
+ rescan_archive(struct ld_plugin_input_file* plugin_input_file);
+
// Call the cleanup handler.
void
cleanup();
@@ -87,6 +93,11 @@ class Plugin
set_all_symbols_read_handler(ld_plugin_all_symbols_read_handler handler)
{ this->all_symbols_read_handler_ = handler; }
+ // Register a rescan archive handler.
+ void
+ set_rescan_archive_handler(ld_plugin_rescan_archive_handler handler)
+ { this->rescan_archive_handler_ = handler; }
+
// Register a claim-file handler.
void
set_cleanup_handler(ld_plugin_cleanup_handler handler)
@@ -112,6 +123,7 @@ class Plugin
// The plugin's event handlers.
ld_plugin_claim_file_handler claim_file_handler_;
ld_plugin_all_symbols_read_handler all_symbols_read_handler_;
+ ld_plugin_rescan_archive_handler rescan_archive_handler_;
ld_plugin_cleanup_handler cleanup_handler_;
// TRUE if the cleanup handlers have been called.
bool cleanup_done_;
@@ -127,7 +139,7 @@ class Plugin_manager
plugin_input_file_(), in_replacement_phase_(false),
options_(options), workqueue_(NULL), task_(NULL), input_objects_(NULL),
symtab_(NULL), layout_(NULL), dirpath_(NULL), mapfile_(NULL),
- this_blocker_(NULL), extra_search_path_()
+ this_blocker_(NULL), extra_search_path_(), rescan_()
{ this->current_ = plugins_.end(); }
~Plugin_manager();
@@ -160,6 +172,11 @@ class Plugin_manager
Layout* layout, Dirsearch* dirpath, Mapfile* mapfile,
Task_token** last_blocker);
+ // Call the rescan-archive handlers. Return true if the archive
+ // should be rescanned.
+ bool
+ rescan_archive(Input_file* input_file, off_t offset, off_t filesize);
+
// Run deferred layout.
void
layout_deferred_objects();
@@ -184,6 +201,14 @@ class Plugin_manager
(*this->current_)->set_all_symbols_read_handler(handler);
}
+ // Register a rescan archive handler.
+ void
+ set_rescan_archive_handler(ld_plugin_rescan_archive_handler handler)
+ {
+ gold_assert(this->current_ != plugins_.end());
+ (*this->current_)->set_rescan_archive_handler(handler);
+ }
+
// Register a claim-file handler.
void
set_cleanup_handler(ld_plugin_cleanup_handler handler)
@@ -232,6 +257,12 @@ class Plugin_manager
ld_plugin_status
add_input_file(const char* pathname, bool is_lib);
+ // Add an archive or group to be rescanned after the
+ // all-symbols-read handler has been called.
+ void
+ add_rescan(const Input_argument* input_argument)
+ { this->rescan_.push_back(input_argument); }
+
// Set the extra library path.
ld_plugin_status
set_extra_library_path(const char* path);
@@ -283,6 +314,8 @@ class Plugin_manager
// An extra directory to seach for the libraries passed by
// add_input_library.
std::string extra_search_path_;
+ // A list of archives to rescan.
+ std::vector<const Input_argument*> rescan_;
};
Index: readsyms.cc
===================================================================
RCS file: /cvs/src/src/gold/readsyms.cc,v
retrieving revision 1.45
diff -p -u -r1.45 readsyms.cc
--- readsyms.cc 20 Aug 2010 00:35:12 -0000 1.45
+++ readsyms.cc 13 Dec 2010 20:52:16 -0000
@@ -316,6 +316,21 @@ Read_symbols::do_read_symbols(Workqueue*
this->input_group_,
this->this_blocker_,
this->next_blocker_));
+
+ if (!this->input_argument_->file().options().whole_archive()
+ && parameters->options().has_plugins()
+ && parameters->options().plugins()->rescan_archive(input_file,
+ 0, filesize))
+ {
+ if (this->input_group_ != NULL)
+ this->input_group_->set_needs_rescan();
+ else
+ {
+ Plugin_manager* plugins = parameters->options().plugins();
+ plugins->add_rescan(this->input_argument_);
+ }
+ }
+
return true;
}
}
@@ -456,6 +471,7 @@ Read_symbols::do_group(Workqueue* workqu
this->symtab_,
this->layout_,
this->mapfile_,
+ this->input_argument_,
input_group,
this->next_blocker_);
@@ -680,6 +696,12 @@ Finish_group::run(Workqueue*)
}
}
+ if (this->input_group_->needs_rescan())
+ {
+ gold_assert(parameters->options().has_plugins());
+ parameters->options().plugins()->add_rescan(this->input_argument_);
+ }
+
// Now that we're done with the archives, record the incremental layout
// information, then delete them.
for (Input_group::const_iterator p = this->input_group_->begin();
Index: readsyms.h
===================================================================
RCS file: /cvs/src/src/gold/readsyms.h,v
retrieving revision 1.21
diff -p -u -r1.21 readsyms.h
--- readsyms.h 22 Mar 2010 14:18:24 -0000 1.21
+++ readsyms.h 13 Dec 2010 20:52:16 -0000
@@ -188,7 +188,7 @@ class Input_group
typedef Archives::const_iterator const_iterator;
Input_group()
- : archives_()
+ : archives_(), needs_rescan_(false)
{ }
// Add an archive to the group.
@@ -196,6 +196,17 @@ class Input_group
add_archive(Archive* arch)
{ this->archives_.push_back(arch); }
+ // Whether some plugin has requested that this group be rescanned
+ // after all symbols have been read.
+ bool
+ needs_rescan() const
+ { return this->needs_rescan_; }
+
+ // Note that some plugin has requested that this group be rescanned.
+ void
+ set_needs_rescan()
+ { this->needs_rescan_ = true; }
+
// Loop over the archives in the group.
const_iterator
@@ -208,6 +219,8 @@ class Input_group
private:
Archives archives_;
+ // Whether a plugin has asked to rescan this group.
+ bool needs_rescan_;
};
// This class starts the handling of a group. It exists only to pick
@@ -254,11 +267,13 @@ class Finish_group : public Task
{
public:
Finish_group(Input_objects* input_objects, Symbol_table* symtab,
- Layout* layout, Mapfile* mapfile, Input_group* input_group,
+ Layout* layout, Mapfile* mapfile,
+ const Input_argument* input_argument, Input_group* input_group,
Task_token* next_blocker)
: input_objects_(input_objects), symtab_(symtab),
- layout_(layout), mapfile_(mapfile), input_group_(input_group),
- saw_undefined_(0), this_blocker_(NULL), next_blocker_(next_blocker)
+ layout_(layout), mapfile_(mapfile), input_argument_(input_argument),
+ input_group_(input_group), saw_undefined_(0), this_blocker_(NULL),
+ next_blocker_(next_blocker)
{ }
~Finish_group();
@@ -297,6 +312,7 @@ class Finish_group : public Task
Symbol_table* symtab_;
Layout* layout_;
Mapfile* mapfile_;
+ const Input_argument* input_argument_;
Input_group* input_group_;
size_t saw_undefined_;
Task_token* this_blocker_;
Index: lto-plugin/lto-plugin.c
===================================================================
--- lto-plugin/lto-plugin.c (revision 167736)
+++ lto-plugin/lto-plugin.c (working copy)
@@ -129,6 +129,7 @@ enum symbol_style
static char *arguments_file_name;
static ld_plugin_register_claim_file register_claim_file;
static ld_plugin_register_all_symbols_read register_all_symbols_read;
+static ld_plugin_register_rescan_archive register_rescan_archive;
static ld_plugin_get_symbols get_symbols;
static ld_plugin_register_cleanup register_cleanup;
static ld_plugin_add_input_file add_input_file;
@@ -622,7 +623,7 @@ all_symbols_read_handler (void)
free (lto_argv);
- if (pass_through_items)
+ if (pass_through_items != NULL && register_rescan_archive == NULL)
{
unsigned int i;
for (i = 0; i < num_pass_through_items; i++)
@@ -913,6 +914,24 @@ claim_file_handler (const struct ld_plug
return LDPS_OK;
}
+/* Callback used by gold to see if an archive should be rescanned
+ after all the symbols have been read. We use this to direct gold
+ to rescan the archives which may contain symbols definition
+ introduced by the gcc optimizers. */
+
+static enum ld_plugin_status
+rescan_archive_handler (const struct ld_plugin_input_file *file, int *rescan)
+{
+ const char *base;
+
+ base = lbasename (file->name);
+ if (strcmp (base, "libgcc.a") == 0
+ || strcmp (base, "libc.a") == 0
+ || strcmp (base, "libm.a") == 0)
+ *rescan = 1;
+ return LDPS_OK;
+}
+
/* Parse the plugin options. */
static void
@@ -998,6 +1017,9 @@ onload (struct ld_plugin_tv *tv)
case LDPT_OPTION:
process_option (p->tv_u.tv_string);
break;
+ case LDPT_REGISTER_RESCAN_ARCHIVE_HOOK:
+ register_rescan_archive = p->tv_u.tv_register_rescan_archive;
+ break;
default:
break;
}
@@ -1025,5 +1047,12 @@ onload (struct ld_plugin_tv *tv)
"could not register the all_symbols_read callback");
}
+ if (register_rescan_archive)
+ {
+ status = register_rescan_archive (rescan_archive_handler);
+ check (status == LDPS_OK, LDPL_FATAL,
+ "could not register the rescan_archive callback");
+ }
+
return LDPS_OK;
}