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

[Cary Coutant <ccoutant at google dot com>]

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.