gold --emit-relocs

gold --emit-relocs

[Alan Modra]

A symbol value in an ELF final linked binary is absolute, in contrast
to a relocatable object file where the value is section relative.  For
--emit-relocs it is therefore incorrect to use the value of a section
symbol as the addend when adjusting relocs against input section
symbols to output section symbols.

I believe it is correct to always subtract os->address() from the
addend.  Relocatable linking will set output section addresses to
zero, but if for some reason the addresses changed to be non-zero then
you'd need to subtract os->address() for ld -r too.

Is this OK to commit?

	PR gold/18846
	* target-reloc.h (relocate_relocs): Move handling of addend for
	RELOC_ADJUST_FOR_SECTION_RELA.  Subtract os->address() from addend.
	* powerpc.cc (relocate_relocs): Likewise.

diff --git a/gold/powerpc.cc b/gold/powerpc.cc
index 540e197..a9d93aa 100644
--- a/gold/powerpc.cc
+++ b/gold/powerpc.cc
@@ -8095,7 +8095,6 @@ Target_powerpc<size, big_endian>::relocate_relocs(
       typename elfcpp::Elf_types<size>::Elf_WXword r_info = reloc.get_r_info();
       unsigned int r_sym = elfcpp::elf_r_sym<size>(r_info);
       unsigned int r_type = elfcpp::elf_r_type<size>(r_info);
-      const unsigned int orig_r_sym = r_sym;
       typename elfcpp::Elf_types<size>::Elf_Swxword addend
 	= reloc.get_r_addend();
       const Symbol* gsym = NULL;
@@ -8130,6 +8129,8 @@ Target_powerpc<size, big_endian>::relocate_relocs(
 		// the output section corresponding to input section
 		// in which this symbol is defined.
 		gold_assert(r_sym < local_count);
+		const Symbol_value<size>* psymval = object->local_symbol(r_sym);
+		addend = psymval->value(object, addend);
 		bool is_ordinary;
 		unsigned int shndx =
 		  object->local_symbol_input_shndx(r_sym, &is_ordinary);
@@ -8138,6 +8139,7 @@ Target_powerpc<size, big_endian>::relocate_relocs(
 		gold_assert(os != NULL);
 		gold_assert(os->needs_symtab_index());
 		r_sym = os->symtab_index();
+		addend -= os->address();
 	      }
 	      break;
 
@@ -8185,10 +8187,8 @@ Target_powerpc<size, big_endian>::relocate_relocs(
       if (strategy == Relocatable_relocs::RELOC_COPY)
 	;
       else if (strategy == Relocatable_relocs::RELOC_ADJUST_FOR_SECTION_RELA)
-	{
-	  const Symbol_value<size>* psymval = object->local_symbol(orig_r_sym);
-	  addend = psymval->value(object, addend);
-	}
+	// Handled above
+	;
       else if (strategy == Relocatable_relocs::RELOC_SPECIAL)
 	{
 	  if (addend >= 32768)
diff --git a/gold/target-reloc.h b/gold/target-reloc.h
index c135459..3688e4e 100644
--- a/gold/target-reloc.h
+++ b/gold/target-reloc.h
@@ -702,6 +702,18 @@ relocate_relocs(
 		gold_assert(os != NULL);
 		gold_assert(os->needs_symtab_index());
 		new_symndx = os->symtab_index();
+		if (strategy == Relocatable_relocs::RELOC_ADJUST_FOR_SECTION_RELA)
+		  {
+		    typename elfcpp::Elf_types<size>::Elf_Swxword addend;
+		    const Symbol_value<size>* psymval;
+		    psymval = object->local_symbol(r_sym);
+		    addend = Reloc_types<sh_type, size, big_endian>::
+		      get_reloc_addend(&reloc);
+		    addend = psymval->value(object, addend);
+		    addend -= os->address();
+		    Reloc_types<sh_type, size, big_endian>::
+		      set_reloc_addend(&reloc_write, addend);
+		  }
 	      }
 	      break;
 
@@ -776,14 +788,7 @@ relocate_relocs(
 	  switch (strategy)
 	    {
 	    case Relocatable_relocs::RELOC_ADJUST_FOR_SECTION_RELA:
-	      {
-		typename elfcpp::Elf_types<size>::Elf_Swxword addend;
-		addend = Reloc_types<sh_type, size, big_endian>::
-			   get_reloc_addend(&reloc);
-		addend = psymval->value(object, addend);
-		Reloc_types<sh_type, size, big_endian>::
-		  set_reloc_addend(&reloc_write, addend);
-	      }
+	      // Handled above
 	      break;
 
 	    case Relocatable_relocs::RELOC_ADJUST_FOR_SECTION_0:

-- 
Alan Modra
Australia Development Lab, IBM

Re: gold --emit-relocs

[Cary Coutant]

> A symbol value in an ELF final linked binary is absolute, in contrast
> to a relocatable object file where the value is section relative.  For
> --emit-relocs it is therefore incorrect to use the value of a section
> symbol as the addend when adjusting relocs against input section
> symbols to output section symbols.
>
> I believe it is correct to always subtract os->address() from the
> addend.  Relocatable linking will set output section addresses to
> zero, but if for some reason the addresses changed to be non-zero then
> you'd need to subtract os->address() for ld -r too.

Yes, I agree with your analysis. Thanks!

> diff --git a/gold/target-reloc.h b/gold/target-reloc.h
> index c135459..3688e4e 100644
> --- a/gold/target-reloc.h
> +++ b/gold/target-reloc.h
> @@ -702,6 +702,18 @@ relocate_relocs(
>                 gold_assert(os != NULL);
>                 gold_assert(os->needs_symtab_index());
>                 new_symndx = os->symtab_index();
> +               if (strategy == Relocatable_relocs::RELOC_ADJUST_FOR_SECTION_RELA)
> +                 {
> +                   typename elfcpp::Elf_types<size>::Elf_Swxword addend;
> +                   const Symbol_value<size>* psymval;
> +                   psymval = object->local_symbol(r_sym);
> +                   addend = Reloc_types<sh_type, size, big_endian>::
> +                     get_reloc_addend(&reloc);
> +                   addend = psymval->value(object, addend);
> +                   addend -= os->address();
> +                   Reloc_types<sh_type, size, big_endian>::
> +                     set_reloc_addend(&reloc_write, addend);
> +                 }

Rather than move this code to here, I'd prefer to leave it where it
is, but promote "os" to the scope of the loop and adjust the addend
using os->address():

diff --git a/gold/target-reloc.h b/gold/target-reloc.h
index c135459..89906af 100644
--- a/gold/target-reloc.h
+++ b/gold/target-reloc.h
@@ -666,6 +666,7 @@ relocate_relocs(

       // Get the new symbol index.

+      Output_section* os = NULL;
       unsigned int new_symndx;
       if (r_sym < local_count)
        {
@@ -698,7 +699,7 @@ relocate_relocs(
                unsigned int shndx =
                  object->local_symbol_input_shndx(r_sym, &is_ordinary);
                gold_assert(is_ordinary);
-               Output_section* os = object->output_section(shndx);
+               os = object->output_section(shndx);
                gold_assert(os != NULL);
                gold_assert(os->needs_symtab_index());
                new_symndx = os->symtab_index();
@@ -780,7 +781,8 @@ relocate_relocs(
                typename elfcpp::Elf_types<size>::Elf_Swxword addend;
                addend = Reloc_types<sh_type, size, big_endian>::
                           get_reloc_addend(&reloc);
-               addend = psymval->value(object, addend);
+               gold_assert(os != NULL);
+               addend = psymval->value(object, addend) - os->address();
                Reloc_types<sh_type, size, big_endian>::
                  set_reloc_addend(&reloc_write, addend);
              }

... And a similar change in powerpc.cc.

-cary

Re: gold --emit-relocs

[Alan Modra]

On Wed, Aug 19, 2015 at 12:39:50AM -0700, Cary Coutant wrote:
> Rather than move this code to here, I'd prefer to leave it where it
> is, but promote "os" to the scope of the loop and adjust the addend
> using os->address():

OK, I probably would have written the patch this way myself if I'd
started with target-reloc.h.  Patch revised as you suggest and
committed 9215b98b.

-- 
Alan Modra
Australia Development Lab, IBM