Re: [PATCH][GOLD] Interworking and other branch stubs, non-ARM specific part.

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

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

> -  while (target->may_relax() && target->relax(pass));
> +  while (target->may_relax()
> +	 && target->relax(pass, parameters->options(), input_objects,
> +			  symtab, this));

Don't pass in parameters->options().  The code which still passes
around a reference to the options is a holdover from before it was
accessible from parameters.  These days just use parameters.


> +  // Return the entry size.
> +  uint64_t
> +  entsize() const
> +  { return this->entsize_; }
> +
> +   // Whether this is a merge string section.  This is only true of
> +   // Output_merge_string.
> +   bool
> +   is_string()
> +   { return this->do_is_string(); }

There is something wrong with the indentation here.


> +  // Set the offset of a section to invalid_address.
> +  virtual void
> +  do_invalidate_section_offset(unsigned int shndx)
> +  {
> +    gold_assert(shndx < this->section_offsets_.size());
> +    this->section_offsets_[shndx] = invalid_address;
> +    this->set_relocs_must_follow_section_writes();
> +  }

I don't understand why you call set_relocs_must_follow_section_writes
here.  That is normally done only for very special cases like
.eh_frame sections.  We have to write out the .eh_frame sections
before we can apply relocations to them, because until we write them
out we don't know how the input data is going to map to the output
data.  It would also apply to merge sections if merge sections could
have relocs--we actually just refuse to merge sections which have
relocs.

This patch doesn't really explain why you need to invalidate the
section offset, but it's hard for me to see how that could be related
to setting relocs_must_follow_section_writes.


> +  // Return Symbol table section index.
> +  unsigned int
> +  symtab_shndx() const
> +  { return this->symtab_shndx_; }

s/Symbol/symbol/


> +  DEFINE_int(stub_group_size, options::TWO_DASHES , '\0', 1,
> +              N_("Size of stub group"), N_("SIZE"));
> +

This is ARM specific, and the help string should say so.  Eventually
we should have a mechanism for target specific options.

The GNU ld help string is much more informative, probably too long.  I
wonder if there is something more useful we can say here.


> +  // For a relaxed section, we use the current data size.  Linker scripts
> +  // get all the input sections, including relaxed one from an output
> +  // section and add them back to them same output section to compute the
> +  // output section size.  If we do not account for sizes of relaxed input
> +  // sections,  an output section would be incorrectly size.

s/size/sized/


> +// Relax input sections in an input section list.
>  
> -  // 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)
> +void
> +Output_section::relax_input_sections_in_list(
> +  const std::vector<Output_relaxed_input_section*>& relaxed_sections,
> +  const Relaxation_map& map,
> +  Input_section_list* input_sections)
> +{
> +  for(size_t i = 0; i < relaxed_sections.size(); ++i)

Space before parenthesis.

> +    {
> +      Output_relaxed_input_section* poris = relaxed_sections[i];
> +      Input_section_specifier iss(poris->relobj(), poris->shndx());
> +      Relaxation_map::const_iterator p = map.find(iss);
> +      gold_assert(p != map.end());
> +      gold_assert((*input_sections)[p->second].is_input_section());
> +      (*input_sections)[p->second] = Input_section(poris);
> +    }
> +}

The comment doesn't seem to describe this function.  This function is
not relaxing input sections.

> +// Relax existing input sections.
> +
> +void
> +Output_section::relax_input_sections(
> +  const std::vector<Output_relaxed_input_section*>& relaxed_sections)

Similarly, this comment also seems wrong.


> +  // Simply invalid the relaxed input section map since we do not keep
> +  // track of it.
> +  this->is_relaxed_input_section_map_valid_ = false;

s/invalid/invalidate/


> +  // Compute a hash value of this.
> +  size_t
> +  hash_value() const
> +  { return reinterpret_cast<intptr_t>(this->relobj_) ^ this->shndx_; }

It's dangerous to hash on a pointer as it can cause the linker to
generate different a output file from the same input files.  This use
seems to be safe, but I would still prefer that you hash on something
else, such as the name, or perhaps the size of the file.


> -  relax(int pass)
> +  relax(int pass, const General_options& options,
> +	const Input_objects* input_objects, Symbol_table* symtab,
> +	Layout* layout)
>    {
>      // Run the dummy relaxation pass twice if relaxation debugging is enabled.
>      if (is_debugging_enabled(DEBUG_RELAXATION))
>        return pass < 2;
>  
> -    return this->do_relax(pass);
> +    return this->do_relax(pass, options, input_objects, symtab, layout);
>    } 

As noted above, remove the options parameter.


This is all fine with changes as noted above except for the
set_relocs_must_follow_section_writes call.  We need to sort that out.

Thanks, and sorry for the slow review.

Ian