This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] gold: Don't adjust local symbol values in relocatable link twice
- From: James Clarke <jrtc27 at jrtc27 dot com>
- To: Binutils <binutils at sourceware dot org>, Cary Coutant <ccoutant at gmail dot com>
- Cc: Ben Gamari <bgamari at gmail dot com>
- Date: Fri, 27 Oct 2017 08:00:04 +0100
- Subject: Re: [PATCH] gold: Don't adjust local symbol values in relocatable link twice
- Authentication-results: sourceware.org; auth=none
- References: <20171014231353.18724-1-jrtc27@jrtc27.com>
Ping?
James
> On 15 Oct 2017, at 00:13, James Clarke <jrtc27@jrtc27.com> wrote:
>
> The fix committed for PR gold/19291 ended up breaking other cases. The
> commit added adjustment code to write_local_symbols, but in many cases
> compute_final_local_value_internal had already subtracted the output
> section's address. To fix this, all other adjustments are now removed, so
> only the one in write_local_symbols is left.
>
> gold/
> PR gold/22266
> * object.cc (Sized_relobj_file::compute_final_local_value_internal):
> Drop relocatable parameter and stop adjusting output value based on
> it.
> (Sized_relobj_file::compute_final_local_value): Stop passing
> relocatable to compute_final_local_value_internal.
> (Sized_relobj_file::do_finalize_local_symbols): Ditto.
> * object.h (Sized_relobj_file::compute_final_local_value_internal):
> Drop relocatable parameter.
> ---
>
> A simple test is available at http://paste.debian.net/990796/, which is
> much simpler than the real-world example in PR gold/22266. You can see
> that int_from_a_1 ends up having a negative (well, large positive since
> it's unsigned) symbol value when gold is used before this patch, so the
> value printed by the program is not the expected 11223344 but whatever
> happens to be before it in memory.
>
> gold/object.cc | 27 ++++++++-------------------
> gold/object.h | 4 +---
> 2 files changed, 9 insertions(+), 22 deletions(-)
>
> diff --git a/gold/object.cc b/gold/object.cc
> index 4110686ff3..22a41a2db6 100644
> --- a/gold/object.cc
> +++ b/gold/object.cc
> @@ -2318,7 +2318,6 @@ Sized_relobj_file<size, big_endian>::compute_final_local_value_internal(
> unsigned int r_sym,
> const Symbol_value<size>* lv_in,
> Symbol_value<size>* lv_out,
> - bool relocatable,
> const Output_sections& out_sections,
> const std::vector<Address>& out_offsets,
> const Symbol_table* symtab)
> @@ -2420,10 +2419,7 @@ Sized_relobj_file<size, big_endian>::compute_final_local_value_internal(
> os->find_relaxed_input_section(this, shndx);
> if (posd != NULL)
> {
> - Address relocatable_link_adjustment =
> - relocatable ? os->address() : 0;
> - lv_out->set_output_value(posd->address()
> - - relocatable_link_adjustment);
> + lv_out->set_output_value(posd->address());
> }
> else
> lv_out->set_output_value(os->address());
> @@ -2432,14 +2428,10 @@ Sized_relobj_file<size, big_endian>::compute_final_local_value_internal(
> {
> // We have to consider the addend to determine the
> // value to use in a relocation. START is the start
> - // of this input section. If we are doing a relocatable
> - // link, use offset from start output section instead of
> - // address.
> - Address adjusted_start =
> - relocatable ? start - os->address() : start;
> + // of this input section.
> Merged_symbol_value<size>* msv =
> new Merged_symbol_value<size>(lv_in->input_value(),
> - adjusted_start);
> + start);
> lv_out->set_merged_symbol_value(msv);
> }
> }
> @@ -2450,7 +2442,7 @@ Sized_relobj_file<size, big_endian>::compute_final_local_value_internal(
> + secoffset
> + lv_in->input_value());
> else
> - lv_out->set_output_value((relocatable ? 0 : os->address())
> + lv_out->set_output_value(os->address()
> + secoffset
> + lv_in->input_value());
> }
> @@ -2476,12 +2468,11 @@ Sized_relobj_file<size, big_endian>::compute_final_local_value(
> const Symbol_table* symtab)
> {
> // This is just a wrapper of compute_final_local_value_internal.
> - const bool relocatable = parameters->options().relocatable();
> const Output_sections& out_sections(this->output_sections());
> const std::vector<Address>& out_offsets(this->section_offsets());
> return this->compute_final_local_value_internal(r_sym, lv_in, lv_out,
> - relocatable, out_sections,
> - out_offsets, symtab);
> + out_sections, out_offsets,
> + symtab);
> }
>
> // Finalize the local symbols. Here we set the final value in
> @@ -2501,7 +2492,6 @@ Sized_relobj_file<size, big_endian>::do_finalize_local_symbols(
> const unsigned int loccount = this->local_symbol_count_;
> this->local_symbol_offset_ = off;
>
> - const bool relocatable = parameters->options().relocatable();
> const Output_sections& out_sections(this->output_sections());
> const std::vector<Address>& out_offsets(this->section_offsets());
>
> @@ -2510,9 +2500,8 @@ Sized_relobj_file<size, big_endian>::do_finalize_local_symbols(
> Symbol_value<size>* lv = &this->local_values_[i];
>
> Compute_final_local_value_status cflv_status =
> - this->compute_final_local_value_internal(i, lv, lv, relocatable,
> - out_sections, out_offsets,
> - symtab);
> + this->compute_final_local_value_internal(i, lv, lv, out_sections,
> + out_offsets, symtab);
> switch (cflv_status)
> {
> case CFLV_OK:
> diff --git a/gold/object.h b/gold/object.h
> index 508e79cb3c..c6c4927740 100644
> --- a/gold/object.h
> +++ b/gold/object.h
> @@ -2772,8 +2772,7 @@ class Sized_relobj_file : public Sized_relobj<size, big_endian>
> // LV_IN points to a local symbol value containing the input value.
> // LV_OUT points to a local symbol value storing the final output value,
> // which must not be a merged symbol value since before calling this
> - // method to avoid memory leak. RELOCATABLE indicates whether we are
> - // linking a relocatable output. OUT_SECTIONS is an array of output
> + // method to avoid memory leak. OUT_SECTIONS is an array of output
> // sections. OUT_OFFSETS is an array of offsets of the sections. SYMTAB
> // points to a symbol table.
> //
> @@ -2785,7 +2784,6 @@ class Sized_relobj_file : public Sized_relobj<size, big_endian>
> compute_final_local_value_internal(unsigned int r_sym,
> const Symbol_value<size>* lv_in,
> Symbol_value<size>* lv_out,
> - bool relocatable,
> const Output_sections& out_sections,
> const std::vector<Address>& out_offsets,
> const Symbol_table* symtab);
> --
> 2.14.2
>