Re: [patch][gold] Fix R_ARM_TLS_LE32 when there is no TLS segment

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

Rafael Espindola <espindola@google.com> writes:

> The attached patch fixes R_ARM_TLS_LE32 relocations when there is no TLS
> segment. This can happen with linker scripts. I think the old code was
> also wrong in the case we have a .tbss and a .tdata section. The relocations
> would be correct for the section with the largest address, but not for the
> other one.
>
> 2010-05-20  Rafael Espindola  <espindola@google.com>
>
> 	* arm.cc (relocated_section): New.
> 	(Target_arm<big_endian>::Relocate::relocate_tls): Use
> 	relocated_section and Layout::tls_section.
> 	* layout.cc (Layout::Layout): Initialize tls_section_.
> 	(Layout::compute_tls_section): New.
> 	* layout.h (Layout::tls_section, Layout::compute_tls_section,
> 	Layout::tls_section_): New.

I'm having trouble understanding this patch.  If you have SHF_TLS
sections but you don't have a PT_TLS segment, then as far as I can see
your program isn't going to work.  While I can certainly see the
advantage of producing a better error message, I don't see why the
linker should be concerned about just which value it generates.  The
program will fail at runtime anyhow.

That said, a few mechanical comments on the last version of the patch:


> @@ -8679,6 +8679,23 @@ Target_arm<big_endian>::Relocate::relocate(
>    return true;
>  }
>  
> +template<bool big_endian>
> +Output_section *
> +relocated_section(const Relocate_info<32, big_endian>* relinfo,
> +                  const elfcpp::Rel<32, big_endian>& rel,
> +                  const Sized_symbol<32>* gsym)

This is going to make a global function relocated_section, not a good
idea.  It should be a static member of Target_arm.

> @@ -1524,6 +1525,30 @@ Layout::prepare_for_relaxation()
>    this->record_output_section_data_from_script_ = true;
>  }
>  
> +Output_section*
> +Layout::compute_tls_section() const
> +{

Function needs a comment.

> +  // Check that this method is used only once.
> +  gold_assert(this->tls_section_ == NULL);
> +
> +  uint64_t align = 0;
> +  Layout::Section_list list = this->section_list();

That is going to make a copy of the vector.  Write this intead:
  const Layout::Section_list& list(this->section_list());

> +  for (Section_list::iterator i = list.begin(); i != list.end(); ++i)
> +  {
> +    Output_section* sec = *i;
> +    if (!(sec->flags() & elfcpp::SHF_TLS))
> +      continue;

Write
    if ((sec->flags() & elfcpp::SHF_TLS) == 0)

> +    if (sec->addralign() > align)
> +      align = sec->addralign();
> +    if (this->tls_section_ == NULL ||
> +        sec->address() < this->tls_section_->address())
> +      this->tls_section_ = sec;

Put the "||" at the start of the next line.

Ian