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]

Re: Patch to do reorder text and data sections according to a user specified sequence.


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]