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

[Sriraman Tallam <tmsriram at google dot com>]

Hi Cary,


  Like we discussed, I made all the changes. I now have a separate
list of unclaimed objects in the Plugin manager and the handle will be
an index into it. I have also added a new hook called
"inspect_unclaimed_object". This handler allows the plugin to examine
the contents of an unclaimed object. Now, I have disallowed examining
claimed objects here as the methods to get their contents etc. are not
defined in Pluginobj. Also, I have kept the original interfaces to get
section count or type or contents. I think I would need this interface
for my work rather than only get contents based on filters. I guess
more interfaces can be added easily.

Thanks,
-Sri.


Change Log for plugin-api.h


	* 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_read_layout_from_file): 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_READ_LAYOUT_FROM_FILE): 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_read_layout_from_file): New struct members.
	(tv_allow_section_ordering): New struct members.

ChangeLog for gold:


	* layout.cc (Layout::Layout): Initialize section_ordering_specified_,
	input_section_position_, and input_section_glob_.
	(Layout::update_layout_of_sections): New function.
	(read_layout_from_file): Add parameter filename and call function
	section_ordering_specified.
	* layout.h (is_section_ordering_specified): New function.
	(section_ordering_specified): New function.
	(read_layout_from_file): Add parameter filename.
	(update_layout_of_sections): New function.
	(section_ordering_specified_): New boolean member.
	* main.cc(main): Call load_plugins after layout object is defined.
	Call read_layout_from_file when section ordering is specified.
	* output.cc (Output_section::add_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.
	(read_layout_from_file): 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.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.
	(read_layout_from_file): New function pointer.
	(allow_section_ordering): New function pointer.
	(inspect_unclaimed_object_hook): New function.
	(onload): Check if the new interfaces exist.



On Wed, Mar 9, 2011 at 4:40 PM, Cary Coutant <ccoutant@google.com> wrote:
> Here are my comments on your patch. Thanks for working on this -- I
> think it's going to be valuable.
>
> -cary
>
>
>
> Index: plugin-api.h
> ===================================================================
>
> You'll need a separate ChangeLog entry for include/. Also, we use
> C-style coding conventions in include/plugin-api.h. In particular,
> "char* filename" should be "char *filename", etc.
>
> +/* The linker's interface for retrieving the handle (pointer) to an object. */
> +
> +typedef
> +enum ld_plugin_status
> +(*ld_plugin_get_object_handle) (const char* filename, void** handle);
>
> I'm curious how this interface is going to be used. Where does the
> plugin obtain a filename where it doesn't already have a handle? Are
> you going to claim the files, or merely observe them and pass them
> through unclaimed? If this is the case, why not just keep a list of
> the handles in your plugin, and arrange for the handles to remain
> valid even for unclaimed files (in the current plugin architecture,
> the handle is just an index into the list of claimed files)?
>
> +typedef
> +enum ld_plugin_status
> +(*ld_plugin_get_section_name) (void* handle, unsigned int shndx,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? char** section_name_ptr);
> +/* The linker's interface for retrieving the contents of a specific section
> + ? in an object. ?*/
>
> Missing blank line before this comment.
>
> +/* 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);
>
> Why do we need both of these interfaces? Couldn't
> read_layout_from_file imply allow_section_ordering?
>
> Index: layout.cc
> ===================================================================
> + ? ?relaxation_debug_check_(NULL),
> + ? ?section_ordering_specified_(false),
> + ? ?input_section_position_(),
> + ? ?input_section_glob_()
> +
>
> Extra blank line here.
>
> + ? ? ?if ((*p) == NULL)
> + ? ? ? ?continue;
> + ? ? ?(*p)->update_section_layout(this);
>
> This could just be:
>
> + ? ? ?if ((*p) != NULL)
> + ? ? ? ?(*p)->update_section_layout(this);
>
> Index: layout.h
> ===================================================================
>
> + ?bool
> + ?is_section_ordering_specified()
> + ?{ return section_ordering_specified_; }
> +
> + ?void
> + ?section_ordering_specified()
> + ?{ section_ordering_specified_ = true; }
> +
> +
>
> Extra blank line here.
>
> ? unsigned int
> ? find_section_order_index(const std::string&);
>
> + ? void
> + ?read_layout_from_file(const char* filename);
> +
> ? void
> + ?update_layout_of_sections();
>
> These functions should all have comments.
>
> + ?bool section_ordering_specified_;
>
> Need a comment here, too.
>
> Index: main.cc
> ===================================================================
>
> + ?const char* section_ordering_file =
> parameters->options().section_ordering_file();
>
> Line too long.
>
> Index: output.cc
> ===================================================================
>
> + ? ? ? ? Object* obj = ((*p).is_input_section() ? (*p).relobj()
> + ? ? ? ? ? ? ? ? ? ? ? ?: (*p).relaxed_input_section()->relobj());
>
> I'd prefer to see this written like this:
>
> + ? ? ? ? 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(std::string(section_name));
>
> section_name is already a string here -- no conversion necessary.
>
> Index: plugin.cc
> ===================================================================
>
> @@ -404,7 +458,7 @@ Plugin_manager::all_symbols_read(Workque
> - ?this->layout_ = layout;
> + ?gold_assert (this->layout_ == layout);
>
> Why not just drop layout from the parameter list for all_symbols_read?
>
> +// Find the object having the given name.
> +// Parameters :
> +// FILENAME : The name of the object whose handle needs to be
> +// ? ? ? ? ? ?retrieved.
> +// HANDLE ? : Storage for the retrieved handle.
>
> This comment formatting style doesn't match the rest of the file (or
> anywhere else in gold, except for icf.cc). I don't need to see it
> changed, though -- I'm not sure about Ian's preferences.
>
> + ?if (input_objects == NULL)
> + ? ?{
> + ? ? ?return LDPS_ERR;
> + ? ?}
>
> Excessive use of braces?
>
>
> + ? ? ?if (strcmp((*p)->name().c_str(), filename) == 0)
> + ? ? ? ?{
> + ? ? ? ? ?*handle = static_cast<void*>(*p);
> + ? ? ? ? ?break;
> + ? ? ? ?}
>
> You could just return LDPS_OK here, and return LDPS_ERR
> unconditionally below the loop.
>
> The handle returned here is very different from the handle returned by
> the existing set of interfaces, and you don't seem to do anything to
> tell them apart; this effectively makes it two separate and
> non-interoperable APIs. There also isn't any way for the linker side
> of the plugin interface to do a sanity check on the handle given it.
> I'd suggest keeping the notion of handle as an index into a list of
> objects, but extend that list to cover all objects rather than just
> the claimed ones.
>
> +static enum ld_plugin_status
> +get_section_type(void* handle, unsigned int shndx, unsigned int* type)
>
> +static enum ld_plugin_status
> +get_section_name(void* handle, unsigned int shndx, char** section_name_ptr)
>
> It's looking like you intend for the plugin to iterate over the
> sections one at a time, looking for a section by either type or name,
> or both. Depending on how you intend to actually use this (and keeping
> in mind generality for other potential uses), it might be better to
> replace these two interfaces with a single one that can find a section
> filtered by type, name, or both (with a provision for iterating
> through multiple results).
>
> Also note that the section_type() and section_name() object methods go
> to a File_view, and you need a task lock in order to call them (else
> you'll get a file descriptor leak). When during the link do you expect
> these interfaces to be used? If it's only from the claim-file handler,
> it should be OK, but if it's from the all-files-read handler, this is
> going to re-open the files. There should be some check in place to
> ensure it's called only when it's designed to be called.
>

Attachment: gold_patch_new.txt
Description: Text document