This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [patch][gold] Fix R_ARM_TLS_LE32 when there is no TLS segment
- From: Ian Lance Taylor <iant at google dot com>
- To: Rafael Espindola <espindola at google dot com>
- Cc: Binutils <binutils at sourceware dot org>, Doug Kwan (éæå) <dougkwan at google dot com>
- Date: Tue, 25 May 2010 17:03:41 -0700
- Subject: Re: [patch][gold] Fix R_ARM_TLS_LE32 when there is no TLS segment
- References: <AANLkTilnRgyNamFlzltj0_Ez5FUQcJ52Qj-DSqWFWv80@mail.gmail.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