Home Original page

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

Ian Lance Taylor iant@google.com
Wed Aug 22 21:41:00 GMT 2012 
On Thu, Aug 9, 2012 at 2:32 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>
>         * 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.

> +      if (it != this->section_segment_map_.end())
> +	{
> +	  // We know the name of the output section, directly call
> +	  // get_output_section here by-passing choose_output_section.
> +	  elfcpp::Elf_Xword flags = shdr.get_sh_flags();

Please invert this conditional, so the common and shorter case appears.  That is
  if (it == this->section_segment_map_.end())
    os = this->choose_output_section(...);
  else
    {
      ...
    }

> +	  // We know the name of the output section, directly call
> +	  // get_output_section here by-passing choose_output_section.
> +	  elfcpp::Elf_Xword flags = shdr.get_sh_flags();
> +	  // Some flags in the input section should not be automatically
> +	  // copied to the output section.
> +	  flags &= ~ (elfcpp::SHF_INFO_LINK
> +		      | elfcpp::SHF_GROUP
> +		      | elfcpp::SHF_MERGE
> +		      | elfcpp::SHF_STRINGS);
> +
> +	  // We only clear the SHF_LINK_ORDER flag in for
> +	  // a non-relocatable link.
> +	  if (!parameters->options().relocatable())
> +	    flags &= ~elfcpp::SHF_LINK_ORDER;

This code is too finicky to have two copies.  Please refactor into a
common function one way or another.

> +	  os_name = this->namepool_.add_with_length(os_name, strlen(os_name),
> +						    true, &name_key);

Don't call add_with_length(x, strlen(x), ...).  Just call add().

> +      // If this segment is marked unique, skip.

This comment adds nothing to the code.

> +	  if (os->segment_alignment())

os->segment_alignment is not a bool: write os->segment_alignment() != 0.

> +  typedef struct
> +  {
> +    // Identifier for the Segment.  ELF Segments dont have names.
> +    const char* name;
> +    // Segment flags.
> +    uint64_t flags;
> +    uint64_t align;
> +  } Unique_segment_info;

This is C++, not C.  Don't write typedef struct { } S.  Just write struct S { }.

The align field should have a comment.  The flags field comment should
say something like "Additional segment flags."

> +  Section_segment_map&
> +  get_section_segment_map()
> +  { return this->section_segment_map_; }

gold doesn't use non-const references as function parameter or return
types.  This should be a pointer.

> +	  gold_assert (sd != NULL);

Remove space before left parenthesis.

Thanks for the cleanups here.

> +  const unsigned char* pnamesu = (is_two_pass)
>  				 ? gc_sd->section_names_data
>  				 : sd->section_names->data();

It's not new in this patch, but the parenthesization here is wrong.
It should be
   ... = (is_two_pass
           ? ...
           : ...);

The ? and : should be lined up under the start of the condition (i.e.,
under the 'i').

L +  gold_assert(!(is_two_pass) || reloc_sections.empty());

No need to parenthesize is_two_pass.

> +  uint64_t extra_segment_flags() const
> +  { return extra_segment_flags_; }
> +
> +  void
> +  set_extra_segment_flags(uint64_t flags)
> +  { extra_segment_flags_ = flags; }
> +
> +  uint64_t segment_alignment() const
> +  { return segment_alignment_; }
> +
> +  void
> +  set_segment_alignment(uint64_t align)
> +  { segment_alignment_ = align; }

This functions are all missing "this->".

> +  bool
> +  is_unique_segment() const
> +  { return is_unique_segment_; }
> +
> +  // Mark segment as unique, happens when linker plugins request that
> +  // certain input sections be mapped to unique segments.
> +  void
> +  set_is_unique_segment()
> +  { this->is_unique_segment_ = true; }

Missing this-> here too.

> +  Layout* layout = parameters->options().plugins()->layout();
> +  gold_assert (layout != NULL);

Somewhere here you should ensure that the plugin called the
LDPT_ALLOW_UNIQUE_SEGMENT_FOR_SECTIONS entry point.

> +  Layout::Unique_segment_info* s = static_cast<Layout::Unique_segment_info*>
> +      (malloc(sizeof(Layout::Unique_segment_info)));

This is C++.  You mean
  Layout::Unique_segment_info* s = new Unique_segment_info;

> +      section_segment_map[secn_id] = s;

Rather than have Layout return a pointer to its private data member, I
would prefer to see a Layout method that saves this value.  And that
Layout method should assert unique_segment_for_sections_specified_.

> +check_DATA += plugin_final_layout.stdout plugin_final_layout.readelf.stdout

Stick to a single '.' per fliename.
plugin_final_layout_readelf.stdout is fine here.

> With readelf -l, an ELF Section to Segment mapping is printed as :

readelf can be a bit unpredictable: I recommend using the -W option as well.

> +/* The linker's interface for specifying that a subset of sections is
> +   to be mapped to a unique segment.  This should be invoked before
> +   unique_segment_for_sections, preferably in the claim_file handler.  */

This is a "must", not a "should".  How about something along the lines
of "If the plugin wants to call unique_segment_for_sections, it must
call this function from a claim_file handler or when it is first
loaded."

Sorry for the slow review.

Ian
More information about the Binutils mailing list