This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: Patch to do reorder text and data sections according to a user specified sequence.
- From: Ian Lance Taylor <iant at google dot com>
- To: Sriraman Tallam <tmsriram at google dot com>
- Cc: Taras Glek <tglek at mozilla dot com>, binutils <binutils at sourceware dot org>
- Date: Tue, 01 Jun 2010 07:40:16 -0700
- Subject: Re: Patch to do reorder text and data sections according to a user specified sequence.
- References: <863b0cbf1002101820r104951e6ve2d6f4468263a839@mail.gmail.com> <mcrmxzf6mzl.fsf@dhcp-172-17-9-151.mtv.corp.google.com> <863b0cbf1003021843m1f9413e9wfc2c47e89124f064@mail.gmail.com> <AANLkTilOd7S1Yf1cJyLclDhOP1I7f7SbFslGAucPhVGt@mail.gmail.com> <AANLkTimnQmbHIuSjvjDokJ66cJhWPKgoNQsx5BSB7IHc@mail.gmail.com> <4BFAD2FF.5050908@mozilla.com> <AANLkTimt0YGV-k0j1vgBnqIV6bXuOKPQQAbKru4KUtCi@mail.gmail.com>
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