This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [patch] Gold linker patch to provide plugin support for mapping some text sections to an unique ELF segment.
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