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

[Ian Lance Taylor <iant at google dot com>]

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