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 wrote: > On Thu, Aug 9, 2012 at 2:32 PM, Sriraman Tallam 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::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 >> + (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