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 Ian,

    I have made all the changes and attached the patch.

Thanks,
-Sri.

On Wed, Aug 22, 2012 at 10:59 AM, Ian Lance Taylor <iant@google.com> wrote:
> 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

Attachment: segment_patch.txt
Description: Text document