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 Ian, I committed the patch after making the changes you mentioned. I did not add a warning when using the SORT clause and --section-ordering file. Like you said, I will do this as a separate patch after I figure out how to do this effectively. 2010-06-01 Sriraman Tallam <tmsriram@google.com> * gold.h (is_wildcard_string): New function. * layout.cc (Layout::layout): Pass this pointer to add_input_section. (Layout::layout_eh_frame): Ditto. (Layout::find_section_order_index): New method. (Layout::read_layout_from_file): New method. * layout.h (Layout::find_section_order_index): New method. (Layout::read_layout_from_file): New method. (Layout::input_section_position_): New private member. (Layout::input_section_glob_): New private member. * main.cc (main): Call read_layout_from_file here. * options.h (--section-ordering-file): New option. * output.cc (Output_section::input_section_order_specified_): New member. (Output_section::Output_section): Initialize new member. (Output_section::add_input_section): Add new parameter. Keep input sections when --section-ordering-file is used. (Output_section::set_final_data_size): Sort input sections when section ordering file is specified. (Output_section::Input_section_sort_entry): Add new parameter. Check sorting type. (Output_section::Input_section_sort_entry::compare_section_ordering): New method. (Output_section::Input_section_sort_compare::operator()): Change to consider section_order_index. (Output_section::Input_section_sort_init_fini_compare::operator()): Change to consider section_order_index. (Output_section::Input_section_sort_section_order_index_compare ::operator()): New method. (Output_section::sort_attached_input_sections): Change to sort according to section order when specified. (Output_section::add_input_section<32, true>): Add new parameter. (Output_section::add_input_section<64, true>): Add new parameter. (Output_section::add_input_section<32, false>): Add new parameter. (Output_section::add_input_section<64, false>): Add new parameter. * output.h (Output_section::add_input_section): Add new parameter. (Output_section::input_section_order_specified): New method. (Output_section::set_input_section_order_specified): New method. (Input_section::Input_section): Initialize section_order_index_. (Input_section::section_order_index): New method. (Input_section::set_section_order_index): New method. (Input_section::section_order_index_): New member. (Input_section::Input_section_sort_section_order_index_compare): New struct. (Output_section::input_section_order_specified_): New member. * script-sections.cc (is_wildcard_string): Delete and move modified method to gold.h. (Output_section_element_input::Output_section_element_input): Modify call to is_wildcard_string. (Output_section_element_input::Input_section_pattern ::Input_section_pattern): Ditto. (Output_section_element_input::Output_section_element_input): Ditto. * testsuite/Makefile.am (final_layout): New test case. * testsuite/Makefile.in: Regenerate. * testsuite/final_layout.cc: New file. * testsuite/final_layout.sh: New file. Thanks, -Sri. On Tue, Jun 1, 2010 at 7:40 AM, Ian Lance Taylor <iant@google.com> wrote: > Sriraman Tallam <tmsriram@google.com> writes: > >> ? ? ? * gold.h (is_wildcard_string): New function. >> ? ? ? * layout.cc (Layout::layout): Pass this pointer to add_input_section. >> ? ? ? (Layout::layout_eh_frame): Ditto. >> ? ? ? (Layout::find_section_order_index): New method. >> ? ? ? (Layout::read_layout_from_file): New method. >> ? ? ? * layout.h (Layout::find_section_order_index): New method. >> ? ? ? (Layout::read_layout_from_file): New method. >> ? ? ? (Layout::input_section_position_): New private member. >> ? ? ? (Layout::input_section_glob_): New private member. >> ? ? ? * main.cc (main): Call read_layout_from_file here. >> ? ? ? * options.h (--section-ordering-file): New option. >> ? ? ? * output.cc (Output_section::input_section_order_specified_): New >> ? ? ? member. >> ? ? ? (Output_section::Output_section): Initialize new member. >> ? ? ? (Output_section::add_input_section): Add new parameter. >> ? ? ? Keep input sections when --section-ordering-file is used. >> ? ? ? (Output_section::set_final_data_size): Sort input sections when >> ? ? ? section ordering file is specified. >> ? ? ? (Output_section::Input_section_sort_entry): Add new parameter. >> ? ? ? Check sorting type. >> ? ? ? (Output_section::Input_section_sort_entry::is_before_in_sequence): >> ? ? ? New method. >> ? ? ? (Output_section::Input_section_sort_compare::operator()): Change to >> ? ? ? consider section_order_index. >> ? ? ? (Output_section::Input_section_sort_init_fini_compare::operator()): >> ? ? ? Change to consider section_order_index. >> ? ? ? (Output_section::Input_section_sort_section_order_index_compare >> ? ? ? ::operator()): New method. >> ? ? ? (Output_section::sort_attached_input_sections): Change to sort >> ? ? ? according to section order when specified. >> ? ? ? (Output_section::add_input_section<32, true>): Add new parameter. >> ? ? ? (Output_section::add_input_section<64, true>): Add new parameter. >> ? ? ? (Output_section::add_input_section<32, false>): Add new parameter. >> ? ? ? (Output_section::add_input_section<64, false>): Add new parameter. >> ? ? ? * output.h (Output_section::add_input_section): Add new parameter. >> ? ? ? (Output_section::input_section_order_specified): New >> ? ? ? method. >> ? ? ? (Output_section::set_input_section_order_specified): New method. >> ? ? ? (Input_section::Input_section): Initialize section_order_index_. >> ? ? ? (Input_section::section_order_index): New method. >> ? ? ? (Input_section::set_section_order_index): New method. >> ? ? ? (Input_section::section_order_index_): New member. >> ? ? ? (Input_section::Input_section_sort_section_order_index_compare): New >> ? ? ? struct. >> ? ? ? (Output_section::input_section_order_specified_): New member. >> ? ? ? * script-sections.cc (is_wildcard_string): Delete and move modified >> ? ? ? method to gold.h. >> ? ? ? (Output_section_element_input::Output_section_element_input): Modify >> ? ? ? call to is_wildcard_string. >> ? ? ? (Output_section_element_input::Input_section_pattern >> ? ? ? ::Input_section_pattern): Ditto. >> ? ? ? (Output_section_element_input::Output_section_element_input): Ditto. >> ? ? ? * testsuite/Makefile.am (final_layout): New test case. >> ? ? ? * testsuite/Makefile.in: Regenerate. >> ? ? ? * testsuite/final_layout.cc: New file. >> ? ? ? * testsuite/final_layout.sh: New file. > > >> +void >> +Layout::read_layout_from_file() >> +{ >> + ?const char* filename = parameters->options().section_ordering_file(); >> + ?std::ifstream in; >> + ?std::string line; >> + >> + ?in.open(filename); >> + ?std::getline(in, line); ? // this chops off the trailing \n, if any >> + ?if (!in) >> + ? ?gold_fatal(_("unable to open --section-ordering-file file %s: %s"), >> + ? ? ? ? ? ? ? filename, strerror(errno)); > > You should check the result of in.open before calling std::getline. > >> + >> + ?unsigned int position = 1; >> + ?while (in) >> + ? ?{ >> + ? ? ?if (!line.empty() && line[line.length() - 1] == '\r') ? // Windows >> + ? ? ? ?line.resize(line.length() - 1); >> + ? ? ?this->input_section_position_[line] = position; >> + ? ? ?// Store all glob patterns in a vector. >> + ? ? ?if (is_wildcard_string(line.c_str())) >> + ? ? ? ?this->input_section_glob_.push_back(line); >> + ? ? ?position++; >> + ? ? ?std::getline(in, line); >> + ? ?} >> +} > > I think it would be useful to have some provision for comments in this > file. ?I suggest that all lines which begin with '#' be discarded. ?I > don't think we need to worry about section names which begin with '#'. > > >> + ?// Hash a pattern to its position in the section ordering file. >> + ?std::map<std::string, unsigned int> input_section_position_; > > The comment says "Hash" but the code uses a map. ?It does seem that a > hash would be appropriate here--e.g., use Unordered_map. > > >> + ?DEFINE_string(section_ordering_file, options::TWO_DASHES, '\0', NULL, >> + ? ? ? ? ? ? N_("Layout functions and data in the order specified."), >> + ? ? ? ? ? ? N_("FILENAME")); > > s/functions and data/sections/ > > >> @@ -1999,7 +2000,8 @@ Output_section::add_input_section(Sized_ >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const char* secname, >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const elfcpp::Shdr<size, big_endian>& shdr, >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned int reloc_shndx, >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool have_sections_script) >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bool have_sections_script, >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? Layout* layout) > > The convention in gold is that general capability objects are passed > first. ?In this case layout should be the first parameter, not the > last. > >> @@ -2090,16 +2092,30 @@ Output_section::add_input_section(Sized_ >> ? ?// We need to keep track of this section if we are already keeping >> ? ?// track of sections, or if we are relaxing. ?Also, if this is a >> ? ?// section which requires sorting, or which may require sorting in >> - ?// the future, we keep track of the sections. >> + ?// the future, we keep track of the sections. ?If the >> + ?// --section-ordering-file option is used to specify the order of >> + ?// sections, we need to keep track of sections. >> ? ?if (have_sections_script >> ? ? ? ?|| !this->input_sections_.empty() >> ? ? ? ?|| this->may_sort_attached_input_sections() >> ? ? ? ?|| this->must_sort_attached_input_sections() >> ? ? ? ?|| parameters->options().user_set_Map() >> - ? ? ?|| parameters->target().may_relax()) >> - ? ?this->input_sections_.push_back(Input_section(object, shndx, >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? shdr.get_sh_size(), >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? addralign)); >> + ? ? ?|| parameters->target().may_relax() >> + ? ? ?|| parameters->options().section_ordering_file()) >> + ? ?{ >> + ? ? ?Input_section isecn(object, shndx, shdr.get_sh_size(), addralign); >> + ? ? ?if (parameters->options().section_ordering_file()) >> + ? ? ? ?{ >> + ? ? ? ? ?unsigned int section_order_index = >> + ? ? ? ? ? ?layout->find_section_order_index(std::string(secname)); >> + ? ? ? if (section_order_index) >> + ? ? ? ? ? ?{ >> + ? ? ? ? ? ? ?isecn.set_section_order_index(section_order_index); >> + ? ? ? ? ? ? ?this->set_input_section_order_specified(); >> + ? ? ? ? ? ?} >> + ? ? ? ?} >> + ? ? ?this->input_sections_.push_back(isecn); >> + ? ?} > > Write if (section_order_index != 0); it's not a boolean value. > >> @@ -2782,6 +2801,22 @@ class Output_section::Input_section_sort >> ? ? ?return memcmp(base_name + base_len - 2, ".o", 2) == 0; >> ? ?} >> >> + ?// Returns 0 if no order is specified. Returns 1 if "this" is the >> + ?// first section in order (is_before), returns 2 for s. > > s/"this"/THIS/. ?s/for s/for S/. > >> + ?unsigned int >> + ?is_before_in_sequence(const Input_section_sort_entry& s) const >> + ?{ >> + ? ?gold_assert(this->index_ != -1U); >> + ? ?if (this->input_section_.section_order_index() == 0 >> + ? ? ? ?|| s.input_section().section_order_index() == 0) >> + ? ? ?return 0; >> + ? ?if (this->input_section_.section_order_index() >> + ? ? ? ? < s.input_section().section_order_index()) > > Indentation looks wrong. > >> + ? ? ?return 1; >> + ? ?else >> + ? ? ?return 2; >> + ?} > > A more conventional comparator would return an int value, and return > -1 if THIS is first, 1 if S is first, 0 if they are incomparable. ?I > think we should use that convention unless there is some reason it > won't work. > > The name "is_before_in_sequence" implies that this function returns a > boolean value, which it does not. ?Change the name to something like > "compare_section_ordering". > >> @@ -2843,6 +2878,12 @@ Output_section::Input_section_sort_compa >> ? ?if (!s1_has_priority && s2_has_priority) >> ? ? ?return true; >> >> + ?// Check if a section order exists for these sections through a section >> + ?// ordering file. ?If sequence_num is 0, an order does not exist. >> + ?unsigned int sequence_num = s1.is_before_in_sequence(s2); >> + ?if (sequence_num) >> + ? ?return sequence_num == 1; > > Write "if (sequence_num != 0)". ?When the function changes to return > an int, write "return sequence_num < 0". > >> @@ -2879,6 +2920,12 @@ Output_section::Input_section_sort_init_ >> ? ?if (!s1_has_priority && s2_has_priority) >> ? ? ?return false; >> >> + ?// Check if a section order exists for these sections through a section >> + ?// ordering file. ?If sequence_num is 0, an order does not exist. >> + ?unsigned int sequence_num = s1.is_before_in_sequence(s2); >> + ?if (sequence_num) >> + ? ?return sequence_num == 1; > > Same here. > >> +// Return true if S1 should come before S2. >> +bool >> +Output_section::Input_section_sort_section_order_index_compare::operator()( >> + ? ?const Output_section::Input_section_sort_entry& s1, >> + ? ?const Output_section::Input_section_sort_entry& s2) const >> +{ >> + ?// Check if a section order exists for these sections through a section >> + ?// ordering file. ?If sequence_num is 0, an order does not exist. >> + ?unsigned int sequence_num = s1.is_before_in_sequence(s2); >> + ?if (sequence_num) >> + ? ?return sequence_num == 1; > > Same here. > >> @@ -2913,17 +2976,27 @@ Output_section::sort_attached_input_sect >> ? ?for (Input_section_list::iterator p = this->input_sections_.begin(); >> ? ? ? ? p != this->input_sections_.end(); >> ? ? ? ? ++p, ++i) >> - ? ?sort_list.push_back(Input_section_sort_entry(*p, i)); >> + ? ? ?sort_list.push_back(Input_section_sort_entry(*p, i, >> + ? ? ? ? ? ? ? ? ? ? ? ? this->must_sort_attached_input_sections())); > > Indentation change looks wrong. > >> ? ?// Sort the input sections. >> - ?if (this->type() == elfcpp::SHT_PREINIT_ARRAY >> - ? ? ?|| this->type() == elfcpp::SHT_INIT_ARRAY >> - ? ? ?|| this->type() == elfcpp::SHT_FINI_ARRAY) >> - ? ?std::sort(sort_list.begin(), sort_list.end(), >> - ? ? ? ? ? Input_section_sort_init_fini_compare()); >> + ?if (this->must_sort_attached_input_sections()) >> + ? ?{ >> + ? ? ?if (this->type() == elfcpp::SHT_PREINIT_ARRAY >> + ? ? ? ? ?|| this->type() == elfcpp::SHT_INIT_ARRAY >> + ? ? ? ? ?|| this->type() == elfcpp::SHT_FINI_ARRAY) >> + ? ? ? ?std::sort(sort_list.begin(), sort_list.end(), >> + ? ? ? ? ? ? ? Input_section_sort_init_fini_compare()); >> + ? ? ?else >> + ? ? ? ?std::sort(sort_list.begin(), sort_list.end(), >> + ? ? ? ? ? ? ? Input_section_sort_compare()); >> + ? ?} >> ? ?else >> - ? ?std::sort(sort_list.begin(), sort_list.end(), >> - ? ? ? ? ? Input_section_sort_compare()); >> + ? ?{ >> + ? ? ?gold_assert(parameters->options().section_ordering_file()); >> + ? ? ?std::sort(sort_list.begin(), sort_list.end(), >> + ? ? ? ? ? ? Input_section_sort_section_order_index_compare()); >> + ? ?} > > This code is probably right but it means that in some cases people > will not get the expected result from --section-ordering-file. ?I > wonder if there is some good way that we can warn if somebody tries to > use --section-ordering-file to reorder the input sections which appear > in a SORT clause in the section script. > > > This is OK with those changes. ?If you're not sure how to handle the > warning, commit everything else and send a separate small patch to add > a warning. > > Thanks. > > Ian >
Attachment:
final_layout_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] |