Re: [PATCH] gold: Don't adjust local symbol values in relocatable link twice

[James Clarke <jrtc27 at jrtc27 dot 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
>