Re: [PATCH][GOLD] Add support for relaxation.

[Ian Lance Taylor <iant at google dot com>]

"Doug Kwan (éæå)" <dougkwan@google.com> writes:

> 2009-09-04  Doug Kwan  <dougkwan@google.com>
>
>         * gold.cc (demangle.h): New include.
>         (demangled_typename): New function.
>         * gold.h (typeinfo): New include.
>         (RELXAXATION_DEBUG): Add comment.
>         (demangled_typename): New declaration.
>         * layout.cc (Relaxation_debug_check): New class.
>         (Relaxation_debug_check::check_output_data_for_reset_values,
>         Relaxation_debug_check::read_sections,
>         Relaxation_debug_check::read_sections): New method definitions.
>         (Layout::Layout): Initialize data members
>         record_output_section_data_from_scrips_ and
>         script_output_section_data_list_.
>         (Layout::save_segments, Layout::restore_segments,
>         Layout::clean_up_after_relaxation): New method definitions.
>         (Layout::finalize): Support relaxation.
>         (Layout::set_asection_address_from_script): Move code for orphan
>         section placement out.
>         (Layout::place_orphan_sections_in_script, Layout::print_segments,
>         Layout::print_sections, Layout::print_special_outputs): New method
>         definitions.
>         * layout.h (Layout::print_segments, Layout::print_sections,
>         Layout::print_special_outputs): Declare new methods.
>         (Layout::new_output_section_data_from_script): New method definition.
>         (Layout::place_orphan_sections_in_script): New method declaration.
>         (Layout::Segment_states): New type declaration.
>         (Layout::save_segments, Layout::restore_segments,
>         Layout::clean_up_after_relaxation): New method declarations.
>         (Layout::Output_section_data_list): New type declaration.
>         (Layout::record_output_section_data_from_script_,
>         Layout::script_output_section_data_list_): New data member.
>         * options.h (--relax-debug): New option.
>         * output.cc (Output_data::do_debug): New method definition.
>         (Output_section_headers::do_size): New method definition.
>         (Output_section_headers::Output_section_headers): Move size
>         computation to Output_section_headers::do_size.
>         (Output_segment_headers::do_size): New method definition.
>         (Output_file_header::Output_file_header): Move size computation to
>         Output_file_header::do_size and call it.
>         (Output_file_header::do_size): New method definition.
>         (Output_data_group::Output_data_group): Adjust call to
>         Output_section_data.
>         (Output_data_dynamic::set_final_data_size): Add DT_NULL tag only once.
>         (Output_symtab_xindex::do_write): Add array bound check.
>         (Output_section::Input_section::print_to_mapfile): Handle
>         RELAXED_INPUT_SECTION_CODE.
>         (Output_section::Input_section::debug): New method definition.
>         (Output_section::Output_section): Initialize data member checkpoint_.
>         (Output_section::~Output_section): Delete checkpoint object pointed
>         by checkpoint_.
>         (Output_section::add_input_section): Always add an Input_section if
>         relaxing.
>         (Output_section::add_merge_input_section): Add assert.
>         (Output_section::relax_input_section): New method definition.
>         (Output_section::set_final_data_size): Set load address to zero for
>         an unallocated section.
>         (Output_section::do_address_and_file_offset_have_reset_values):
>         New method definition.
>         (Output_section::Input_section_sort_enty::Input_section_sort_enty):
>         Handle relaxed input section.
>         (Output_section::sort_attached_input_sections): Checkpoint input section
>         list lazily.
>         (Output_section::get_input_sections): Change type of input_sections to
>         list of Simple_input_section pointers.  Checkpoint input section list
>         lazily.  Also handle relaxed input sections.
>         (Output_section::add_input_section_for_script): Take a reference to
>         a Simple_input_section object instead of Relobj pointer and section
>         index as parameter.  Handle relaxed input sections.
>         (Output_section::save_states, Output_section::restore_states): New
>         method definitions.
>         (Output_section::do_debug): New method definition.
>         (Output_section::debug): New method definition.
>         * output.h (Output_data::Output_data): Initialize is_data_size_fixed_.
>         (Output_data::is_data_size_fixed): New method definition.
>         (Output_data::reset_addresss_and_file_offset): Do not reset data size
>         if it is fixed.
>         (Output_data::address_and_file_offset_have_reset_values): New method
>         definition.
>         (Output_data::debug): New method definitions.
>         (Output_data::do_address_and_file_offset_have_reset_values): New method
>         definition.
>         (Output_data::do_debug): New method definitions.
>         (Output_data::set_data_size): Check that data size is not fixed.
>         (Output_data::fix_data_size): New method definition.
>         (Output_data::is_data_size_fixed_): New data member.
>         (Output_section_headers::set_final_data_size): New method definition.
>         (Output_section_headers::do_size): New method declaration.
>         (Output_segment_headers::set_final_data_size): New method definition.
>         (Output_segment_headers::do_size): New method declaration.
>         (Output_file_header::set_final_data_size)::New method definition.
>         (Output_file_header::do_size)::New method declaration.
>         (Output_section_data::Output_section_data): Add new parameter
>         is_data_size_fixed and use it to fix data size.
>         (Output_data_const::Output_data_const): Adjust call to base class
>         constructor and fix data size.
>         (Output_data_const_buffer::Output_data_const_buffer): Adjust call to
>         base class constructor and fix data size.
>         (Output_data_fixed_space::Output_data_fixed_space): Adjust call to
>         base class constructor and fix data size.
>         (Output_data_zero_fill::Output_data_zero_fill): Adjust call to base
>         class constructor and fix data size.
>         (Output_data_group::set_final_data_size): New method definition.
>         (Output_data_dynamic::Dynamic_entry::tag): New method definition.
>         (Output_symtab_xindex::Output_symtab_xindex): Adjust call to base
>         class constructor and fix data size.
>         (Output_relaxed_input_section): New class definition.
>         (Output_section::Simple_input_section): New class definition.
>         (Output_section::get_input_sections): Adjust parameter list.
>         (Output_section::add_input_section_for_script): Same.
>         (Output_section::save_states, Output_section::restore_states,
>         Output_section::do_address_and_file_offset_have_reset_values,
>         Output_section::do_debug): New method declarations.
>         (Output_section::Input_section::Input_section): Handle
>         RELAXED_INPUT_SECTION_CODE.  Add new overload for
>         Output_relaxed_input_section.
>         (Output_section::Input_section::is_input_section): Handle relaxed
>         input section.
>         (Output_section::Input_section::is_relaxed_input_section,
>         Output_section::Input_section::output_section_data,
>         Output_section::Input_section::relaxed_input_section): New method
>         definitions.
>         (Output_section::Input_section::debug): New method declaration and
>         definition.
>         (Output_section::Input_section::RELAXED_INPUT_SECTION_CODE): New enum
>         value.
>         (Output_section::Input_section::u1_, Output_section::Input_section::u2_
>         ): Update comments.
>         (Output_section::Checkpoint): New classs definition.
>         (Output_section::relax_input_section): New method declaration.
>         (Output_section::checkpoint_): New data member.
>         (Output_segment): Update comments.
>         (Output_segment::Output_segment): Un-privatize copy constructor.
>         (Output_segment::operator=): Un-privatize.
>         (Output_segment::debug): New method declaration and definition.
>         * script-sections.cc (Output_section_element::Input_section_list):
>         Change element type to Output_section::Simple_input_section.
>         (Output_section_element_dot_assignment::set_section_addresses):
>         Register output section data for relaxation clean up.
>         (Output_data_exression::Output_data_expression): Adjust call to base
>         constructor to fix data size.
>         (Output_section_element_data::set_section_addresses): Register
>         Output_data_expression object for relaxation clean up.
>         (struct Input_section_info): Replace Relobj pointer and section index
>         pair with Output_section::Simple_input_section and Convert struct to a
>         class.
>         (Input_section_sorter::operator()): Adjust access to
>         Input_section_info data member to use accessors.
>         (Output_section_element_input::set_section_addresses): Use layout
>         parameter.  Adjust code to use Output_section::Simple_input_section
>         and Input_secction_info classes.  Register filler for relaxation
>         clean up.
>         (Orphan_output_section::set_section_addresses): Replace Relobj pointer
>         and section index pair with Output_section::Simple_input_section
>         class.  Adjust code accordingly.
>         (Phdrs_element::release_segment): New method definition.
>         (Script_sections::attach_sections_using_phdrs_clause): Do not modify
>         segment list.
>         (Script_sections::release_segments): New method definition.
>         * gold/script-sections.h (Script_sections::release_segments): New
>         method declaration.
>         * gold/target.h (Target::may_relax, Target::relax,
>        Target::do_may_relax, Target::do_relax): New method definitions.


> +// Return the demangled version of the type_info's name.  This is used to
> +// print debugging information.
> +
> +std::string
> +demangled_typename(const std::type_info* info)
> +{
> +  // cplus_demangle allocates memory for the result it returns,
> +  // and returns NULL if the name is already demangled.
> +  char* demangled_name = cplus_demangle(info->name(), DMGL_TYPES);
> +  if (demangled_name == NULL)
> +    return info->name();
> +
> +  std::string retval = std::string(demangled_name);
> +  free(demangled_name);
> +  return retval;
> +}

This is cute but there should not be any need for it.  We already have
print_to_mapfile in the Output_data class, which gives every Output_data
type a readable name.  We should simply adjust that to implement this
kind of debugging information.


> +// Uncomment this to enable relaxation debugging code.
> +// #define RELAXATION_DEBUG	1

This could be a debugging type in debug.h.  Is there a reason to keep it
as a #ifdef?  The problem with a #ifdef is that it will break over time
and become useless.


> +// Save states of all current output segments.  Store saved states
> +// in SEGMENT_STATES.
> +
> +void
> +Layout::save_segments(Segment_states& segment_states)
> +{
> +  for (Segment_list::const_iterator p = this->segment_list_.begin();
> +       p != this->segment_list_.end();
> +       ++p)
> +    {
> +      Output_segment* segment = *p;
> +      // Shallow copy.
> +      Output_segment* copy = new Output_segment(*segment);
> +      segment_states[segment] = copy;
> +    }
> +}

This function changes segment_states.  Therefore, segment_states should
be passed as a pointer, not by reference.  In gold parameters are only
passed by reference if they are not changed.  Otherwise it is unclear at
the call site what is happening.

> +	  // This is a segment created during section layout.  It should be
> +	  // safe to remove it since we should have remove all pointers to it.

s/remove/removed/.

> +  // Fix up TLS and RELRO segment pointers as appropriate.
> +  this->tls_segment_ = NULL;
> +  this->relro_segment_ = NULL;
> +  for (Segment_list::iterator p = this->segment_list_.begin();
> +       p != this->segment_list_.end();
> +       ++p)
> +    {
> +      Output_segment* segment = *p;
> +      if (segment->type() == elfcpp::PT_TLS)
> +	this->tls_segment_ = segment;
> +      else if (segment->type() == elfcpp::PT_GNU_RELRO)
> +	this->relro_segment_ = segment;
> +    }

I think you should do this in the earlier loop, rather than adding
another loop.


> +  // A linker script may have created some output section data objects.
> +  // They are useless now.
> +  for (Output_section_data_list::const_iterator p =
> +	 this->script_output_section_data_list_.begin();
> +       p != this->script_output_section_data_list_.end();
> +       ++p)
> +    delete (*p);

Just "delete *p".  delete is not a function.


> +  // Relaxation Loop:  If target has no relaxation, this loop runs only one
> +  // iteration.  Otherwise, the target relaxation hook is called at the end of
> +  // each iteration.  If the hook returns true, it means re-layout of
> +  // section is required.  
> +  //
> +  // The number of segments created by a linking script without a PHDRS
> +  // clause may be affected by section sizes and alignments.  There is
> +  // a remote chance that relaxation causes different number of PT_LOAD
> +  // segments are created and sections are attached to different segments.
> +  // Therefore, we always throw away all segments created during section
> +  // layout.  In order to be able to restart the section layout, we keep
> +  // a copy of the segment list right before the relaxation loop and use
> +  // that to restore the segments.

This function was already too big.  Can the relaxation loop be moved
into a separate function?  Either the loop as a whole or the body of the
loop.

> +      // If this is not the iteration, we need to clean up after relaxation
> +      // so that we can lay out the sections again.

s/is not the iteration/is not the first iteration/.


> +// Relax an existing input section.
> +void
> +Output_section::relax_input_section(Output_relaxed_input_section *psection)
> +{
> +  Relobj* relobj = psection->relobj();
> +  unsigned int shndx = psection->shndx();
> +
> +  gold_assert(relobj->target()->may_relax());
> +
> +  // This is not very efficient if we a going to relax a number of sections
> +  // in an Output_section with lot of Input_sections.
> +  for (Input_section_list::iterator p = this->input_sections_.begin();
> +       p != this->input_sections_.end();
> +       ++p)
> +    {
> +      if (p->is_input_section())
> +	{
> +	  if (p->relobj() == relobj && p->shndx() == shndx)
> +	    {
> +	      gold_assert(p->addralign() == psection->addralign());
> +	      *p = Input_section(psection);
> +	      return;
> +	    }
> +	}
> +      else if (p->is_relaxed_input_section())
> +	gold_assert(p->relobj() != relobj || p->shndx() != shndx);
> +
> +    }
> +}

I'm unclear on how Output_relaxed_input_section is going to be used, and
when this function is going to be called.  As the comment says, this
function looks quite problematic in a large link.  It seems like it
might be better to ask the target whether it wants to relax the input
section in Output_section::add_input_section.


> +    // Return object of an input section.
> +    Relobj*
> +    relobj() const
> +    {
> +      return ((this->shndx_ != invalid_shndx)
> +	      ? this->u_.relobj : this->u_.relaxed_input_section->relobj());
> +    }

Newline before ':'.

> +    // Return index of an input section.
> +    unsigned int
> +    shndx() const
> +    {
> +      return ((this->shndx_ != invalid_shndx)
> +	      ? this->shndx_ : this->u_.relaxed_input_section->shndx());
> +    }

Same here.

> +    // Return the Output_relaxed_input_section object.
> +    Output_relaxed_input_section*
> +    relaxed_input_section() const
> +    {
> +      gold_assert(this->is_relaxed_input_section());
> +      return static_cast<Output_relaxed_input_section*>(this->u2_.posd);
> +    }

We already have a union here--can't we add another element to the union
rather than using a static_cast?

> +  // We only save enough information to undo the effects of section layout.
> +  class Checkpoint

Let's name this Checkpoint_output_section or something like that.
Checkpoint seems slightly too generic.


Overall this looks good.  Sorry for the slow review; I've been pretty
busy.

Ian