Re: [patch] Gold linker patch to provide plugin support for mapping some text sections to an unique ELF segment.

[Cary Coutant <ccoutant at google dot com>]

	* layout.cc (Layout::Layout): Initialize new members.
	(choose_output_section): New parameter.
	Modify call to output_section_name.
	(output_section_name): New parameter.
	Check if input section needs to be mapped to a different output
	section.

Please include the class name with the method name
(Layout::choose_output_section, Layout::output_section_name).

s/New parameter/Add parameter./

	(Layout::layout): Modify call to choose_output_section.
	Mark output section for mapping to an unique segment.

s/an unique/a unique/ (throughout)


@@ -939,7 +944,8 @@ Layout::choose_output_section(const Relo
   if (is_input_section
       && !this->script_options_->saw_sections_clause()
       && !parameters->options().relocatable())
-    name = Layout::output_section_name(relobj, name, &len);
+    name = Layout::output_section_name(relobj, shndx, name, &len,
+				       &this->section_segment_map_);

This seems to be pushing the check for a remapped section too deep --
you're essentially turning a class method into an instance method
by passing it a pointer to instance data that it could have accessed
directly if it weren't static.

I think the lookup in section_segment_map should be made in
Layout::layout, just before it would call choose_output_section;
then you can call choose_output_section with the remapped name.

That would avoid the large number of places where you have to
pass '0' for shndx.


+      // Check if this output section needs to be mapped to an unique segment.
+      // This can happen when using plugins.
+      if (!os->is_unique_segment()
+	  && (this->section_segment_map_.find(Const_section_id(object, shndx))
+	      != this->section_segment_map_.end()))
+	os->set_is_unique_segment();

This looks like a redundant test.  If you move the remap into
Layout::layout, you can just set a bool flag, and call
set_is_unique_segment right after choose_output_section returns.


+  /* Check if do_layout needs to be two-pass.  If so, find out which pass
+     should happen.  In the first pass, the data in sd is saved to be used
+     later in the second pass.  */
+  if (is_two_pass)
+    {
+      if (!this->get_symbols_data())

Write this as "if (this->get_symbols_data() == NULL)".

Also, since you call get_symbols_data() again just a bit lower,
you may as well simply write:

       gc_sd = this->get_symbols_data();
       if (gc_sd == NULL)

+	{
+	  gold_assert (sd != NULL);
+	  is_pass_one = true;
+	}
+      else
+	{
+	  if (parameters->options().gc_sections())
+	    gold_assert(symtab->gc()->is_worklist_ready());
+	  if (parameters->options().icf_enabled())
+	    gold_assert(symtab->icf()->is_icf_ready());
+	  is_pass_two = true;
+	}
+    }
+
+  // Both is_pass_one and is_pass_two should not be true.
+  gold_assert(!(is_pass_one  && is_pass_two));

I think the assert is unnecessary. It's pretty obvious from the code
immediately above that they're mutually exclusive.

What you could explain here (or, better, where the two bools are declared)
is that both will be false when it's *not* a two-pass layout.


+// This function should map the list of sections specified in the
+// SECTION_LIST to an unique segment.  ELF segments do not have names
+// and the NAME is used to identify Output Section which should contain
+// the list of sections.  This Output Section will then be mapped to
+// an unique segment.  FLAGS is used to specify if any additional segment
+// flags need to be set.  For instance, a specific segment flag can be
+// set to identify this segment.  Unsetting segment flags is not possible.

Please add a blank line after this comment block.

+static enum ld_plugin_status
+unique_segment_for_sections(const char* segment_name,
+			    uint64_t flags,
+			    const struct ld_plugin_section* section_list,
+			    unsigned int num_sections)

Given the discussion upstream, I think you'll want to add the ability
for the plugin to select a specific segment alignment here as well.

+{
+  gold_assert(parameters->options().has_plugins());
+
+  if (num_sections == 0)
+    return LDPS_OK;
+
+  if (section_list == NULL)
+    return LDPS_ERR;
+
+  Layout* layout = parameters->options().plugins()->layout();
+  gold_assert (layout != NULL);
+
+  std::map<Const_section_id, const char*>* section_segment_map
+    = layout->get_section_segment_map();

Indent by four spaces here.

I think it would clear up the code if you made this a reference instead
of a pointer. It would also help to have a typedef for the map.

+
+  for (unsigned int i = 0; i < num_sections; ++i)
+    {
+      Object* obj = parameters->options().plugins()->get_elf_object(
+          section_list[i].handle);
+      if (obj == NULL)
+	return LDPS_BAD_HANDLE;
+      unsigned int shndx = section_list[i].shndx;
+      Const_section_id secn_id(obj, shndx);
+      (*section_segment_map)[secn_id] = segment_name;
+    }
+
+  std::map<std::string, uint64_t>* output_section_flags_map
+    = layout->get_output_section_flags_map();
+
+  (*output_section_flags_map)[std::string(segment_name)] = flags;

Rather than have a second map keyed on the section name, I suggest
you store the flags (and alignment) as part of the section_segment_map.

Also, I think "section" flags is the wrong name -- it should be
"segment" flags.

It's also a bit confusing that you're creating a segment (which doesn't
have a name) by creating a new section (which does), and setting the
is_unique_segment flag on that section later when it actually gets
created. There's not much to do about that but clarify it in the
comments and documentation.


--- gold/testsuite/plugin_section_order.c	29 Sep 2011 23:45:57 -0000	1.1
+++ gold/testsuite/plugin_section_order.c	20 Jul 2012 22:51:50 -0000
@@ -36,6 +36,8 @@ static ld_plugin_get_input_section_name
 static ld_plugin_get_input_section_contents get_input_section_contents = NULL;
 static ld_plugin_update_section_order update_section_order = NULL;
 static ld_plugin_allow_section_ordering allow_section_ordering = NULL;
+static ld_plugin_allow_unique_segment_for_sections
allow_unique_segment_for_sections = NULL;
+static ld_plugin_unique_segment_for_sections
unique_segment_for_sections = NULL;

 enum ld_plugin_status onload(struct ld_plugin_tv *tv);
 enum ld_plugin_status claim_file_hook(const struct ld_plugin_input_file *file,
@@ -76,6 +78,11 @@ onload(struct ld_plugin_tv *tv)
 	case LDPT_ALLOW_SECTION_ORDERING:
 	  allow_section_ordering = *entry->tv_u.tv_allow_section_ordering;
 	  break;
+	case LDPT_ALLOW_UNIQUE_SEGMENT_FOR_SECTIONS:
+	  allow_unique_segment_for_sections =
*entry->tv_u.tv_allow_unique_segment_for_sections;
+	case LDPT_UNIQUE_SEGMENT_FOR_SECTIONS:
+	  unique_segment_for_sections = *entry->tv_u.tv_unique_segment_for_sections;
+	  break;

There are a few lines > 80 chars here.


+/* The linker's interface for specifying that a specific set of sections
+   must be mapped to an unique segment.  ELF segments do not have names
+   and the NAME is used as an identifier only.   FLAGS is used to specify
+   if any additional segment flags need to be set.  For instance, a
+   specific segment flag can be set to identify this segment.  Unsetting
+   segment flags that would be set by default is not possible. */

NAME is still subject to some constraints, I think. You're going to create
an output section with that name, so I think it will need to be unique
in the namespace of section names.

+
+typedef
+enum ld_plugin_status
+(*ld_plugin_unique_segment_for_sections) (const char* segment_name,
+					  uint64_t segment_flags,
+					  const struct ld_plugin_section *
+					    section_list,
+					  unsigned int num_sections);

Rewrite this parameter list with the first parameter on a new line,
indented by four spaces.