This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |