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

[Ian Lance Taylor <iant at google dot 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