Re: Plugin interfaces to do Pettis Hansen style code layout in the gold linker.

[Sriraman Tallam <tmsriram at google dot com>]

Hi,

  Finally, I managed to make all these changes and I have attached the
new patch. Thanks.

-Sri.


	* layout.cc (Layout::Layout): Initialize section_ordering_specified_,
	input_section_position_, and input_section_glob_.
	(read_layout_from_file): Call function section_ordering_specified.
	* layout.h (is_section_ordering_specified): New function.
	(section_ordering_specified): New function.
	(section_ordering_specified_): New boolean member.
	* main.cc(main): Call load_plugins after layout object is defined.
	* output.cc (Output_section::add_input_section): Use
	function section_ordering_specified to check if section ordering is
	needed.
	* output.cc (Output_section::add_relaxed_input_section): Use
	function section_ordering_specified to check if section ordering is
	needed.
	(Output_section::update_section_layout): New function.
	(Output_section::sort_attached_input_sections): Check if input section
	must be reordered.
	* output.h (Output_section::update_section_layout): New function.
	* plugin.cc (register_inspect_unclaimed_object): New function.
	(get_section_count): New function.
	(get_section_type): New function.
	(get_section_name): New function.
	(get_section_contents): New function.
	(update_section_order): New function.
	(allow_section_ordering): New function.	
	(Plugin::inspect_unclaimed_object): New function.
	(Plugin_manager::inspect_unclaimed_object): New function.
	(Plugin_manager::get_unclaimed_object): New function.
	(Plugin::load): Add the new interfaces to the transfer vector.
	(Plugin_manager::load_plugins): New parameter.
	(Plugin_manager::all_symbols_read): New parameter.
	* plugin.h (input_objects): New function
	(Plugin::inspect_unclaimed_object): New function.
	(Plugin::set_inspect_unclaimed_object_handler): New function.
	(Plugin__manager::load_plugins): New parameter.
	(Plugin_manager::inspect_unclaimed_object): New function.
	(Plugin_manager::get_unclaimed_object): New function.
	(Plugin_manager::in_inspect_unclaimed_object_handler): New function.
	(Plugin_manager::set_inspect_unclaimed_object_handler): New function.
	(Plugin_manager::unclaimed_object): New function.
	(Plugin_manager::Unclaimed_object_list): New typedef.
	(Plugin_manager::unclaimed_objects_): New member.
	(Plugin_manager::in_inspect_unclaimed_object_handler_): New member.
	(layout): New function.
	* readsyms.cc (Read_symbols::do_read_symbols): Call the plugin handler.
	* testsuite/plugin_test.c (register_inspect_unclaimed_object_hook):
	New function pointer.
	(get_section_count): New function pointer.
	(get_section_type): New function pointer.
	(get_section_name): New function pointer.
	(get_section_contents): New function pointer.
	(update_section_order): New function pointer.
	(allow_section_ordering): New function pointer.
	(inspect_unclaimed_object_hook): New function.
	(onload): Check if the new interfaces exist.


	* plugin-api.h
	(ld_plugin_inspect_unclaimed_object_handler): New typedef.
	(ld_plugin_register_inspect_unclaimed_object): New typedef.
	(ld_plugin_get_section_count): New typedef.
	(ld_plugin_get_section_type): New typedef.
	(ld_plugin_get_section_name): New typedef.
	(ld_plugin_get_section_contents): New typedef.
	(ld_plugin_update_section_order): New typedef.
	(ld_plugin_allow_section_ordering): New typedef.
	(LDPT_REGISTER_INSPECT_UNCLAIMED_OBJECT_HOOK): New enum value.
	(LDPT_GET_SECTION_COUNT): New enum value.
	(LDPT_GET_SECTION_TYPE): New enum value.
	(LDPT_GET_SECTION_NAME): New enum value.
	(LDPT_GET_SECTION_CONTENTS): New enum value.
	(LDPT_UPDATE_SECTION_ORDER): New enum value.
	(LDPT_ALLOW_SECTION_ORDERING): New enum value.
	(tv_get_section_count): New struct members.
	(tv_get_section_type): New struct members.
	(tv_get_section_name): New struct members.
	(tv_get_section_contents): New struct members.
	(tv_update_section_order): New struct members.
	(tv_allow_section_ordering): New struct members.



On Tue, Mar 15, 2011 at 7:44 AM, Ian Lance Taylor <iant@google.com> wrote:
> Sriraman Tallam <tmsriram@google.com> writes:
>
>> +void
>> +Layout::update_layout_of_sections()
>> +{
>> + ?for (Section_list::iterator p = this->section_list_.begin();
>> + ? ? ? p != this->section_list_.end();
>> + ? ? ? ++p)
>> + ? ?{
>> + ? ? ?if ((*p) == NULL)
>> + ? ? ? ?continue;
>> + ? ? ?(*p)->update_section_layout(this);
>
> The check for NULL should be unnecessary. ?Nothing should push NULL on
> section_list_. ?Please remove it. ?If it were needed, several other
> loops would have to be changed as well.
>
>> + ?void
>> + ?section_ordering_specified()
>> + ?{ section_ordering_specified_ = true; }
>
> This needs to be named set_section_ordering_specified as in other cases
> in gold. ?Calling it section_ordering_specified makes it look like an
> accessor function.
>
>
>> @@ -1147,6 +1161,9 @@ class Layout
>> ? ?Segment_states* segment_states_;
>> ? ?// A relaxation debug checker. ?We only create one when in debugging mode.
>> ? ?Relaxation_debug_check* relaxation_debug_check_;
>> + ?// True if the input sections in the output sections should be sorted
>> + ?// as specified in a section ordering file.
>> + ?bool section_ordering_specified_;
>
> Put this with the other bool members.
>
>> + ?// Load plugin libraries.
>> + ?if (command_line.options().has_plugins())
>> + ? ?command_line.options().plugins()->load_plugins(&layout);
>> +
>> + ?const char* section_ordering_file
>> + ? ? ?= parameters->options().section_ordering_file();
>> +
>> + ?if (section_ordering_file)
>> + ? ?layout.read_layout_from_file(section_ordering_file);
>
> Remove the blank line after the declaration. ?Write the conditional as
> ?if (section_ordering_file != NULL)
>
>> +// Updates the ordering of input sections in this output section.
>> +
>> +void
>> +Output_section::update_section_layout(Layout* layout)
>> +{
>> + ?for (Input_section_list::iterator p = this->input_sections_.begin();
>> + ? ? ? p != this->input_sections_.end();
>> + ? ? ? ++p)
>> + ? ?{
>> + ? ? ?if ((*p).is_input_section()
>> + ? ? ? || (*p).is_relaxed_input_section())
>> + ? ? ? ?{
>> + ? ? ? Object* obj = ((*p).is_input_section()
>> + ? ? ? ? ? ? ? ? ? ? ?? (*p).relobj()
>> + ? ? ? ? ? ? ? ? ? ? ?: (*p).relaxed_input_section()->relobj());
>> +
>> + ? ? ? ? ?std::string section_name = obj->section_name((*p).shndx());
>> + ? ? ? unsigned int section_order_index =
>> + ? ? ? ? ? ?layout->find_section_order_index(section_name);
>> + ? ? ? if (section_order_index != 0)
>> + ? ? ? ? ? ?{
>> + ? ? ? ? ? ? ?(*p).set_section_order_index(section_order_index);
>> + ? ? ? ? ? ? ?this->set_input_section_order_specified();
>> + ? ? ? ? }
>> + ? ? ? ? }
>> + ? ?}
>> +}
>
> When calling obj->section_name(), you must lock the object. ?See the
> Output_section::Input_section_sort_entry constructor. ?As that code
> says, section_name() is a slow operation. ?It would be better if we
> could avoid calling it for every single input section if possible.
>
>> +// Call the plugin inspect_unclaimed_object handler.
>> +
>> +inline void
>> +Plugin::inspect_unclaimed_object(unsigned int index)
>> +{
>> + ?void* handle = ?reinterpret_cast<void*>(index);
>> + ?if (this->inspect_unclaimed_object_handler_ != NULL)
>> + ? ? ?(*this->inspect_unclaimed_object_handler_)(handle);
>> +}
>
> Extra space after '=' sign.
>
> Write the cast as
> ?reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(index))
>
> However, I don't see why you are using an index here. ?You have an
> Object* pointer. ?Why not just use that as the handle?
>
>> +Object*
>> +Plugin_manager::get_unclaimed_object(const void* handle)
>> +{
>> + ?unsigned int index
>> + ? ? ?= ?static_cast<unsigned int>(reinterpret_cast<intptr_t>(handle));
>> + ?Object* obj = NULL;
>> +
>> + ?obj = parameters->options().plugins()->unclaimed_object(index);
>> +
>> + ?return obj;
>> +}
>
> Extra space after '=' in index, but as I say I don't see why this
> function is needed at all.
>
>> +// Get the name of the specified section in the object corresponding
>> +// to the handle. ?This plugin interface can only be called in the
>> +// inspect_unclaimed_object handler of the plugin.
>> +
>> +static enum ld_plugin_status
>> +get_section_name(void* handle, unsigned int shndx, char** section_name_ptr)
>> +{
>> + ?gold_assert(parameters->options().has_plugins());
>> +
>> + ?if (!parameters->options().plugins()->in_inspect_unclaimed_object_handler())
>> + ? ?return LDPS_ERR;
>> +
>> + ?Object* obj = parameters->options().plugins()->get_unclaimed_object(handle);
>> +
>> + ?if (obj == NULL)
>> + ? ?return LDPS_BAD_HANDLE;
>> +
>> + ?const char* section_name = obj->section_name(shndx).c_str();
>> + ?*section_name_ptr = (char*)malloc(strlen(section_name) + 1);
>> + ?strcpy(*section_name_ptr, section_name);
>> + ?return LDPS_OK;
>> +}
>
> *section_name_ptr = static_cast<char*>(malloc(strlen(section_name) + 1));
>
>> +// Get the contents of the specified section in the object corresponding
>> +// to the handle. ?This plugin interface can only be called in the
>> +// inspect_unclaimed_object handler of the plugin.
>> +
>> +static enum ld_plugin_status
>> +get_section_contents(void* handle, unsigned int shndx,
>> + ? ? ? ? ? ? ? ? ? ? unsigned char** section_contents_ptr,
>> + ? ? ? ? ? ? ? ? ?unsigned int* len)
>> +{
>> + ?gold_assert(parameters->options().has_plugins());
>> +
>> + ?if (!parameters->options().plugins()->in_inspect_unclaimed_object_handler())
>> + ? ?return LDPS_ERR;
>> +
>> + ?Object* obj = parameters->options().plugins()->get_unclaimed_object(handle);
>> +
>> + ?if (obj == NULL)
>> + ? ?return LDPS_BAD_HANDLE;
>> +
>> + ?section_size_type plen;
>> + ?const unsigned char* section_contents = obj->section_contents(shndx, &plen,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?false);
>> + ?*section_contents_ptr = (unsigned char*)malloc(plen);
>> + ?memcpy(*section_contents_ptr, section_contents, plen);
>> + ?*len = plen;
>> + ?return LDPS_OK;
>> +}
>
> If we expect plugins to actually use this, I think a better interface
> would be to return a pointer to the data rather than to copy the entire
> section contents. ?We could say that the pointer returned would be valid
> until the inspect_unclaimed_object_handler returns. ?The plugin could
> copy the section contents itself if it needs it longer than that.
>
>> +// Specify the ordering of sections in the final layout in a file. This
>> +// does what the linker option --section-ordering-file does.
>> +
>> +static enum ld_plugin_status
>> +read_layout_from_file(const char* filename)
>> +{
>> + ?gold_assert(parameters->options().has_plugins());
>> + ?Layout* layout = parameters->options().plugins()->layout();
>> + ?layout->read_layout_from_file(filename);
>> + ?layout->update_layout_of_sections();
>> + ?return LDPS_OK;
>> +}
>
> This is a strange interface. ?We have a plugin that has assembled all
> the information it needs in memory. ?Now we are saying that it should
> write it out to a file, and then ask the linker to read that file. ?That
> does not make sense to me. ?All that the file contains is a sequence of
> strings. ?Why not just have an interface which we can pass strings to?
> Either an array of strings, or call the interface once per string,
> whatever is most convenient.
>
>> + ?// The list of unclaimed objects. ?The the index of an item in this
>> + ?// in this list serves as the "handle" that we pass to the plugins
>> + ?// in inspect_unclaimed_object_handler.
>> + ?Unclaimed_object_list unclaimed_objects_;
>
> If you use an Object* as the handle, I don't think you need this list at
> all.
>
>> Index: plugin-api.h
>
> Note that this file lives in the gcc repository too.
>
>> +/* The plugin library's "observe file" handler. ?*/
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_inspect_unclaimed_object_handler) (void *);
>
> I think it would be useful to use a typedef here, to lessen the change
> of confusion between the two different kinds of handles. ?E.g.,
>
> typedef void *unclaimed_object_handle;
>
> Then use unclaimed_object_handle where appropriate.
>
>> +/* The linker's interface for registering the "observe file" handler. ?*/
>
> s/observe/inspect/.
>
>> +/* The linker's interface for retrieving the number of sections in an object.
>> + ? The handle is obtained in the inspect_unclaimed_object_handler. ?This
>> + ? interface should only be invoked in the inspect_unclaimed_object_handler. */
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_get_section_count) (const void* handle, unsigned int* count);
>
> I think this should be named get_input_section_count.
>
> The comment should say that the function sets *COUNT.
>
>> +
>> +/* The linker's interface for retrieving the section type of a specific
>> + ? section in an object. ?The handle is obtained in the
>> + ? inspect_unclaimed_object_handler. ?This interface should only be
>> + ? invoked in the inspect_unclaimed_object_handler. */
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_get_section_type) (void* handle, unsigned int shndx,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned int* type);
>
> I think this should be named get_input_section_type.
>
> The comment should say that the function sets *TYPE to an ELF SHT_xxx
> value.
>
>> +/* The linker's interface for retrieving the name of specific section in
>> + ? an object. The handle is obtained in the inspect_unclaimed_object_handler.
>> + ? This interface should only be invoked in the
>> + ? inspect_unclaimed_object_handler. */
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_get_section_name) (void* handle, unsigned int shndx,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? char** section_name_ptr);
>
> I think this should be named get_input_section_name.
>
> The command needs to say that the function sets *section_name_ptr to a
> null-terminated buffer allocated by malloc.
>
>> +/* The linker's interface for retrieving the contents of a specific section
>> + ? in an object. ?The handle is obtained in the
>> + ? inspect_unclaimed_object_handler. ?This interface should only be invoked
>> + ? in the inspect_unclaimed_object_handler. */
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_get_section_contents) (void* handle, unsigned int shndx,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned char** section_contents,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned int* len);
>
> I think this should be named get_input_section_contents.
>
> As noted above, I think we should return a pointer to a memory buffer
> that is valid until inspect_unclaimed_object_handle returns.
>
>
>> +/* The linker's interface for specifying the desired order of sections
>> + ? through a file. ?*/
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_read_layout_from_file) (const char* filename);
>> +
>> +/* The linker's interface for specifying that reordering of sections is
>> + ? desired. ?*/
>> +
>> +typedef
>> +enum ld_plugin_status
>> +(*ld_plugin_allow_section_ordering) (void);
>
> The comments need to say something about why both hooks are needed, and
> when they may be called.
>
> Ian
>

Attachment: latest_plugin_patch.txt
Description: Text document