This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |