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

[Sriraman Tallam <tmsriram at google dot com>]

Hi Cary,

   I have attached the revised patch with all the mentioned changes done.

Thanks,
-Sri.

	* gold.cc (queue_middle_tasks): Call layout again when unique
	segments for sections is desired.
	* layout.cc (Layout::Layout): Initialize new members.
	(Layout::layout): Make output section for mapping to a unique segment.
	(Layout::attach_allocated_section_to_segment): Make unique segment for
	output sections marked so.
	(Layout::segment_precedes): Check for unique segments when sorting.
	* layout.h (Layout::Unique_segment_info): New struct.
	(Layout::Section_segment_map): New typedef.
	(Layout::get_section_segment_map): New function.
	(Layout::is_unique_segment_for_sections_specified): New function.
	(Layout::set_unique_segment_for_sections_specified): New function.
	(Layout::unique_segment_for_sections_specified_): New member.
	(Layout::section_segment_map_): New member.
	* object.cc (Sized_relobj_file<size, big_endian>::do_layout):
	Rename is_gc_pass_one to is_pass_one.
	Rename is_gc_pass_two to is_pass_two.
	Rename is_gc_or_icf to is_two_pass.
	Check for which pass based on whether symbols data is present.
	Make it two pass when unique segments for sections is desired.
	* output.cc (Output_section::Output_section): Initialize new
	members.
	* output.h (Output_section::is_unique_segment): New function.
	(Output_section::set_is_unique_segment): New function.
	(Output_section::is_unique_segment_): New member.
	(Output_section::extra_segment_flags): New function.
	(Output_section::set_extra_segment_flags): New function.
	(Output_section::extra_segment_flags_): New member.
	(Output_section::segment_alignment): New function.
	(Output_section::set_segment_alignment): New function.
	(Output_section::segment_alignment_): New member.
	(Output_segment::Output_segment): Initialize is_unique_segment_.
	(Output_segment::is_unique_segment): New function.
	(Output_segment::set_is_unique_segment): New function.
	(Output_segment::is_unique_segment_): New member.
	* plugin.cc (allow_unique_segment_for_sections): New function.
	(unique_segment_for_sections): New function.
	(Plugin::load): Add new functions to transfer vector.
	* Makefile.am (plugin_final_layout.readelf.stdout): Add readelf output.
	* Makefile.in: Regenerate.
	* testsuite/plugin_final_layout.sh: Check if unique segment
	functionality works.
	* testsuite/plugin_section_order.c (onload): Check if new interfaces
	are available.
	(allow_unique_segment_for_sections): New global.
	(unique_segment_for_sections): New global.
	(claim_file_hook): Call allow_unique_segment_for_sections.
	(all_symbols_read_hook): Call unique_segment_for_sections.


	* plugin-api.h (ld_plugin_allow_unique_segment_for_sections):
	New interface.
	(ld_plugin_unique_segment_for_sections): New interface.
	(LDPT_ALLOW_UNIQUE_SEGMENT_FOR_SECTIONS): New enum val.
	(LDPT_UNIQUE_SEGMENT_FOR_SECTIONS): New enum val.
	(tv_allow_unique_segment_for_sections): New member.
	(tv_unique_segment_for_sections): New member.


On Thu, Aug 2, 2012 at 6:02 PM, Cary Coutant <ccoutant@google.com> wrote:
>         * 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.

Attachment: segment_patch.txt
Description: Text document