Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

[Viktor Kutuzov]

[-- Attachment #1: Type: text/plain, Size: 2877 bytes --]

Ping. Can somebody commit this patch?

Best regards,
Viktor

----- Original Message ----- 
From: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
To: <binutils@sourceware.org>
Sent: Thursday, September 03, 2009 11:02 AM
Subject: Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation


Is this patch Ok?

Best regards,
Viktor
 
----- Original Message ----- 
From: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
To: <binutils@sourceware.org>
Sent: Tuesday, September 01, 2009 12:52 PM
Subject: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation


Thanks, Ian, for the review.

Please find attached the updated patch.

By the way, I notice that there is a mix of spaces and tabs used in the source code for indents.
I have done my best trying to follow the original stile.
But it might be a formal policy I should follow?

Best regards,
Viktor

* gold/arm.cc: Added R_ARM_ABS8 relocation

----- Original Message ----- 
From: "Ian Lance Taylor" <iant@google.com>
To: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
Cc: <binutils@sourceware.org>
Sent: Monday, August 31, 2009 5:56 PM
Subject: Re: [PATCH] Gold: Added R_ARM_ABS8 relocation


Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:

> +  template<int min_no_bits, int max_no_bits>
> +  static inline bool has_overflow(uint32_t bits) {
> +  gold_assert(max_no_bits >= 0 && max_no_bits <= 32);
> +  gold_assert(min_no_bits >= 0 && min_no_bits <= 32);
> +  gold_assert(min_no_bits <= max_no_bits);
> +  if (min_no_bits == 32)
> +  return false;
> +  int32_t max = (1 << (max_no_bits - 1)) - 1;
> +  int32_t min = -(1 << (min_no_bits - 1));
> +  int32_t as_signed = static_cast<int32_t> (bits);
> +  return as_signed > max || as_signed < min;
> +  }

Passing both MIN_NO_BITS and MAX_NO_BITS is not the way to go.  What you
are really testing here is that the value must fit in either a signed or
unsigned value.  You should write it like that.  The function should not
be named has_overflow--that overloads an existing function.  The
function needs a comment.

(Of course this relocation stuff should move to generic code anyhow, but
that is not your problem.)


> + case elfcpp::R_ARM_ABS8:
> +   //FIXME: This should handles properly the dynamic linking against the shared object.
> +   break;

This comment does not make sense as written.  There should be a space
after the "//".  It may be appropriate to warn if the symbol would
normally require a dynamic relocation.  The same may be true in
scan::local.

> + case elfcpp::R_ARM_ABS8:
> +   if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
> + output_section))
> +   reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
> + has_thumb_bit);
> +   break;

Testing should_apply_static_reloc here only makes sense if we warn in
scan::local and scan::global.  Otherwise we will in effect silently
ignore the relocation.

Thanks for sending the patch.

Ian

[-- Attachment #2: binutil-gold-arm-rel-abs8-02.patch --]
[-- Type: application/octet-stream, Size: 3701 bytes --]

diff -E -B -b -r -u binutils.orig/src/gold/arm.cc binutils.gold/src/gold/arm.cc
--- binutils.orig/src/gold/arm.cc	2009-08-11 10:09:14.000000000 -0700
+++ binutils.gold/src/gold/arm.cc	2009-09-01 12:26:04.000000000 -0700
@@ -117,6 +117,22 @@
     return as_signed > max || as_signed < min;
   }
 
+  // Detects overflow of an NO_BITS integer stored in a uint32_t when it
+  // fits in the given number of bits as either a signed or unsigned value.
+  // For example, has_signed_unsigned_overflow<8> would check
+  // -128 <= bits <= 255
+  template<int no_bits>
+  static inline bool
+  has_signed_unsigned_overflow(uint32_t bits) {
+    gold_assert(no_bits >= 2 && no_bits <= 32);
+  	if (no_bits == 32)
+  		return false;
+  	int32_t max = (1 << no_bits) - 1;
+  	int32_t min = -(1 << (no_bits - 1));
+    int32_t as_signed = static_cast<int32_t>(bits);
+    return as_signed > max || as_signed < min;
+  }
+
   // Select bits from A and B using bits in MASK.  For each n in [0..31],
   // the n-th bit in the result is chosen from the n-th bits of A and B.
   // A zero selects A and a one selects B.
@@ -555,6 +571,26 @@
   }
 
  public:
+
+  // R_ARM_ABS8: S + A
+  static inline typename This::Status
+  abs8(unsigned char *view,
+	const Sized_relobj<32, big_endian>* object,
+	const Symbol_value<32>* psymval, bool has_thumb_bit)
+  {
+	typedef typename elfcpp::Swap<8, big_endian>::Valtype Valtype;
+	typedef typename elfcpp::Swap<32, big_endian>::Valtype Reltype;
+	Valtype* wv = reinterpret_cast<Valtype*> (view);
+	Valtype val = elfcpp::Swap<8, big_endian>::readval(wv);
+	Reltype addend = utils::sign_extend<8>(val);
+	Reltype x = This::arm_symbol_value(object, psymval, addend, has_thumb_bit);
+	val = utils::bit_select(val, x, 0xffU);
+	elfcpp::Swap<8, big_endian>::writeval(wv, val);
+	return (utils::has_signed_unsigned_overflow<8>(x)
+		? This::STATUS_OVERFLOW
+		: This::STATUS_OKAY);
+  }
+
   // R_ARM_ABS32: (S + A) | T
   static inline typename This::Status
   abs32(unsigned char *view,
@@ -1074,6 +1110,15 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+    if (parameters->options().output_is_position_independent())
+    {
+  		// FIXME: Create a dynamic relocation for this location.
+			gold_warning(_("%s: symbol would normally require a dynamic reloc %u."),
+					 object->name().c_str(), r_type);
+    }
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       // If building a shared library (or a position-independent
       // executable), we need to create a dynamic relocation for
@@ -1187,6 +1232,16 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+		// Make a dynamic relocation if necessary.
+		if (gsym->needs_dynamic_reloc(Symbol::ABSOLUTE_REF))
+    {
+  		// FIXME: Create a dynamic relocation for this location.
+			gold_warning(_("%s: %s symbol would normally require a dynamic reloc %u."),
+					 object->name().c_str(), gsym->demangled_name().c_str(), r_type);
+    }
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       {
 	// Make a dynamic relocation if necessary.
@@ -1595,6 +1650,13 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+	  if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
+					output_section))
+		  reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
+							has_thumb_bit);
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, true,
 				    output_section))
@@ -1764,6 +1826,9 @@
     case elfcpp::R_ARM_NONE:
       return 0;
 
+  	case elfcpp::R_ARM_ABS8:
+  		return 1;
+
     case elfcpp::R_ARM_ABS32:
     case elfcpp::R_ARM_REL32:
     case elfcpp::R_ARM_THM_CALL:

Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

[Ian Lance Taylor]

Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:

> Ping. Can somebody commit this patch?

Sorry--I've been busy and have not had time to review this set of
patches.  I will get to them soon.  Sorry for the delay.

Ian

> ----- Original Message ----- 
> From: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
> To: <binutils@sourceware.org>
> Sent: Thursday, September 03, 2009 11:02 AM
> Subject: Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation
>
>
> Is this patch Ok?
>
> Best regards,
> Viktor
>
> ----- Original Message ----- 
> From: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
> To: <binutils@sourceware.org>
> Sent: Tuesday, September 01, 2009 12:52 PM
> Subject: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation
>
>
> Thanks, Ian, for the review.
>
> Please find attached the updated patch.
>
> By the way, I notice that there is a mix of spaces and tabs used in the source code for indents.
> I have done my best trying to follow the original stile.
> But it might be a formal policy I should follow?
>
> Best regards,
> Viktor
>
> * gold/arm.cc: Added R_ARM_ABS8 relocation
>
> ----- Original Message ----- 
> From: "Ian Lance Taylor" <iant@google.com>
> To: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
> Cc: <binutils@sourceware.org>
> Sent: Monday, August 31, 2009 5:56 PM
> Subject: Re: [PATCH] Gold: Added R_ARM_ABS8 relocation
>
>
> Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:
>
>> +  template<int min_no_bits, int max_no_bits>
>> +  static inline bool has_overflow(uint32_t bits) {
>> +  gold_assert(max_no_bits >= 0 && max_no_bits <= 32);
>> +  gold_assert(min_no_bits >= 0 && min_no_bits <= 32);
>> +  gold_assert(min_no_bits <= max_no_bits);
>> +  if (min_no_bits == 32)
>> +  return false;
>> +  int32_t max = (1 << (max_no_bits - 1)) - 1;
>> +  int32_t min = -(1 << (min_no_bits - 1));
>> +  int32_t as_signed = static_cast<int32_t> (bits);
>> +  return as_signed > max || as_signed < min;
>> +  }
>
> Passing both MIN_NO_BITS and MAX_NO_BITS is not the way to go.  What you
> are really testing here is that the value must fit in either a signed or
> unsigned value.  You should write it like that.  The function should not
> be named has_overflow--that overloads an existing function.  The
> function needs a comment.
>
> (Of course this relocation stuff should move to generic code anyhow, but
> that is not your problem.)
>
>
>> + case elfcpp::R_ARM_ABS8:
>> +   //FIXME: This should handles properly the dynamic linking against the shared object.
>> +   break;
>
> This comment does not make sense as written.  There should be a space
> after the "//".  It may be appropriate to warn if the symbol would
> normally require a dynamic relocation.  The same may be true in
> scan::local.
>
>> + case elfcpp::R_ARM_ABS8:
>> +   if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
>> + output_section))
>> +   reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
>> + has_thumb_bit);
>> +   break;
>
> Testing should_apply_static_reloc here only makes sense if we warn in
> scan::local and scan::global.  Otherwise we will in effect silently
> ignore the relocation.
>
> Thanks for sending the patch.
>
> Ian

Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

[Viktor Kutuzov]

Hello Ian,

No problem.

Viktor

----- Original Message ----- 
From: "Ian Lance Taylor" <iant@google.com>
To: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
Cc: <binutils@sourceware.org>
Sent: Friday, September 11, 2009 11:39 AM
Subject: Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation


Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:

> Ping. Can somebody commit this patch?

Sorry--I've been busy and have not had time to review this set of
patches.  I will get to them soon.  Sorry for the delay.

Ian

> ----- Original Message ----- 
> From: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
> To: <binutils@sourceware.org>
> Sent: Thursday, September 03, 2009 11:02 AM
> Subject: Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation
>
>
> Is this patch Ok?
>
> Best regards,
> Viktor
>
> ----- Original Message ----- 
> From: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
> To: <binutils@sourceware.org>
> Sent: Tuesday, September 01, 2009 12:52 PM
> Subject: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation
>
>
> Thanks, Ian, for the review.
>
> Please find attached the updated patch.
>
> By the way, I notice that there is a mix of spaces and tabs used in the source code for indents.
> I have done my best trying to follow the original stile.
> But it might be a formal policy I should follow?
>
> Best regards,
> Viktor
>
> * gold/arm.cc: Added R_ARM_ABS8 relocation
>
> ----- Original Message ----- 
> From: "Ian Lance Taylor" <iant@google.com>
> To: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
> Cc: <binutils@sourceware.org>
> Sent: Monday, August 31, 2009 5:56 PM
> Subject: Re: [PATCH] Gold: Added R_ARM_ABS8 relocation
>
>
> Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:
>
>> +  template<int min_no_bits, int max_no_bits>
>> +  static inline bool has_overflow(uint32_t bits) {
>> +  gold_assert(max_no_bits >= 0 && max_no_bits <= 32);
>> +  gold_assert(min_no_bits >= 0 && min_no_bits <= 32);
>> +  gold_assert(min_no_bits <= max_no_bits);
>> +  if (min_no_bits == 32)
>> +  return false;
>> +  int32_t max = (1 << (max_no_bits - 1)) - 1;
>> +  int32_t min = -(1 << (min_no_bits - 1));
>> +  int32_t as_signed = static_cast<int32_t> (bits);
>> +  return as_signed > max || as_signed < min;
>> +  }
>
> Passing both MIN_NO_BITS and MAX_NO_BITS is not the way to go.  What you
> are really testing here is that the value must fit in either a signed or
> unsigned value.  You should write it like that.  The function should not
> be named has_overflow--that overloads an existing function.  The
> function needs a comment.
>
> (Of course this relocation stuff should move to generic code anyhow, but
> that is not your problem.)
>
>
>> + case elfcpp::R_ARM_ABS8:
>> +   //FIXME: This should handles properly the dynamic linking against the shared object.
>> +   break;
>
> This comment does not make sense as written.  There should be a space
> after the "//".  It may be appropriate to warn if the symbol would
> normally require a dynamic relocation.  The same may be true in
> scan::local.
>
>> + case elfcpp::R_ARM_ABS8:
>> +   if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
>> + output_section))
>> +   reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
>> + has_thumb_bit);
>> +   break;
>
> Testing should_apply_static_reloc here only makes sense if we warn in
> scan::local and scan::global.  Otherwise we will in effect silently
> ignore the relocation.
>
> Thanks for sending the patch.
>
> Ian

Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:

> Please find attached the updated patch.
>
> By the way, I notice that there is a mix of spaces and tabs used in the source code for indents.
> I have done my best trying to follow the original stile.
> But it might be a formal policy I should follow?

There is a formal policy: tabs are at 8-space boundaries in all GNU
code.  I fixed the indentation in your patch.

I also changed the warnings to errors since gold is actually generating
incorrect output in that case.  I also avoided 1 << 31, since that value
is not representable in int32_t.

Applied as follows.

Sorry for the long delay.  Thanks for the patch.

Ian


2009-09-17  Viktor Kutuzov  <vkutuzov@accesssoftek.com>

	* arm.cc (has_signed_unsigned_overflow): New function.
	(Arm_relocate_functions::abs8): New function.
	(Target_arm::Scan::local): Handle R_ARM_ABS8.
	(Target_arm::Scan::global): Likewise.
	(Target_arm::relocate::relocate): Likewise.
	(Target_arm::Relocatable_size_for_reloc::get_size_for_reloc):
	Likewise.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: R_ARM_ABS8 --]
[-- Type: text/x-patch, Size: 4001 bytes --]

Index: arm.cc
===================================================================
RCS file: /cvs/src/src/gold/arm.cc,v
retrieving revision 1.8
diff -u -p -r1.8 arm.cc
--- arm.cc	11 Aug 2009 17:09:14 -0000	1.8
+++ arm.cc	18 Sep 2009 00:42:51 -0000
@@ -117,6 +117,23 @@ namespace utils
     return as_signed > max || as_signed < min;
   }
 
+  // Detects overflow of an NO_BITS integer stored in a uint32_t when it
+  // fits in the given number of bits as either a signed or unsigned value.
+  // For example, has_signed_unsigned_overflow<8> would check
+  // -128 <= bits <= 255
+  template<int no_bits>
+  static inline bool
+  has_signed_unsigned_overflow(uint32_t bits)
+  {
+    gold_assert(no_bits >= 2 && no_bits <= 32);
+    if (no_bits == 32)
+      return false;
+    int32_t max = static_cast<int32_t>((1U << no_bits) - 1);
+    int32_t min = -(1 << (no_bits - 1));
+    int32_t as_signed = static_cast<int32_t>(bits);
+    return as_signed > max || as_signed < min;
+  }
+
   // Select bits from A and B using bits in MASK.  For each n in [0..31],
   // the n-th bit in the result is chosen from the n-th bits of A and B.
   // A zero selects A and a one selects B.
@@ -555,6 +572,26 @@ class Arm_relocate_functions : public Re
   }
 
  public:
+
+  // R_ARM_ABS8: S + A
+  static inline typename This::Status
+  abs8(unsigned char *view,
+       const Sized_relobj<32, big_endian>* object,
+       const Symbol_value<32>* psymval, bool has_thumb_bit)
+  {
+    typedef typename elfcpp::Swap<8, big_endian>::Valtype Valtype;
+    typedef typename elfcpp::Swap<32, big_endian>::Valtype Reltype;
+    Valtype* wv = reinterpret_cast<Valtype*>(view);
+    Valtype val = elfcpp::Swap<8, big_endian>::readval(wv);
+    Reltype addend = utils::sign_extend<8>(val);
+    Reltype x = This::arm_symbol_value(object, psymval, addend, has_thumb_bit);
+    val = utils::bit_select(val, x, 0xffU);
+    elfcpp::Swap<8, big_endian>::writeval(wv, val);
+    return (utils::has_signed_unsigned_overflow<8>(x)
+	    ? This::STATUS_OVERFLOW
+	    : This::STATUS_OKAY);
+  }
+
   // R_ARM_ABS32: (S + A) | T
   static inline typename This::Status
   abs32(unsigned char *view,
@@ -1074,6 +1111,15 @@ Target_arm<big_endian>::Scan::local(cons
     case elfcpp::R_ARM_NONE:
       break;
 
+    case elfcpp::R_ARM_ABS8:
+      if (parameters->options().output_is_position_independent())
+	{
+	  // FIXME: Create a dynamic relocation for this location.
+	  gold_error(_("%s: gold bug: need dynamic ABS8 reloc"),
+		     object->name().c_str());
+	}
+      break;
+
     case elfcpp::R_ARM_ABS32:
       // If building a shared library (or a position-independent
       // executable), we need to create a dynamic relocation for
@@ -1187,6 +1233,16 @@ Target_arm<big_endian>::Scan::global(con
     case elfcpp::R_ARM_NONE:
       break;
 
+    case elfcpp::R_ARM_ABS8:
+      // Make a dynamic relocation if necessary.
+      if (gsym->needs_dynamic_reloc(Symbol::ABSOLUTE_REF))
+	{
+	  // FIXME: Create a dynamic relocation for this location.
+	  gold_error(_("%s: gold bug: need dynamic ABS8 reloc for %s"),
+		     object->name().c_str(), gsym->demangled_name().c_str());
+	}
+      break;
+
     case elfcpp::R_ARM_ABS32:
       {
 	// Make a dynamic relocation if necessary.
@@ -1595,6 +1651,13 @@ Target_arm<big_endian>::Relocate::reloca
     case elfcpp::R_ARM_NONE:
       break;
 
+    case elfcpp::R_ARM_ABS8:
+      if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
+				    output_section))
+	reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
+						    has_thumb_bit);
+      break;
+
     case elfcpp::R_ARM_ABS32:
       if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, true,
 				    output_section))
@@ -1764,6 +1827,9 @@ Target_arm<big_endian>::Relocatable_size
     case elfcpp::R_ARM_NONE:
       return 0;
 
+    case elfcpp::R_ARM_ABS8:
+      return 1;
+
     case elfcpp::R_ARM_ABS32:
     case elfcpp::R_ARM_REL32:
     case elfcpp::R_ARM_THM_CALL:

Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

[Viktor Kutuzov]

[-- Attachment #1: Type: text/plain, Size: 2597 bytes --]

Is this patch Ok?

Best regards,
Viktor
 
----- Original Message ----- 
From: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
To: <binutils@sourceware.org>
Sent: Tuesday, September 01, 2009 12:52 PM
Subject: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation


Thanks, Ian, for the review.

Please find attached the updated patch.

By the way, I notice that there is a mix of spaces and tabs used in the source code for indents.
I have done my best trying to follow the original stile.
But it might be a formal policy I should follow?

Best regards,
Viktor

* gold/arm.cc: Added R_ARM_ABS8 relocation

----- Original Message ----- 
From: "Ian Lance Taylor" <iant@google.com>
To: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
Cc: <binutils@sourceware.org>
Sent: Monday, August 31, 2009 5:56 PM
Subject: Re: [PATCH] Gold: Added R_ARM_ABS8 relocation


Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:

> +  template<int min_no_bits, int max_no_bits>
> +  static inline bool has_overflow(uint32_t bits) {
> +  gold_assert(max_no_bits >= 0 && max_no_bits <= 32);
> +  gold_assert(min_no_bits >= 0 && min_no_bits <= 32);
> +  gold_assert(min_no_bits <= max_no_bits);
> +  if (min_no_bits == 32)
> +  return false;
> +  int32_t max = (1 << (max_no_bits - 1)) - 1;
> +  int32_t min = -(1 << (min_no_bits - 1));
> +  int32_t as_signed = static_cast<int32_t> (bits);
> +  return as_signed > max || as_signed < min;
> +  }

Passing both MIN_NO_BITS and MAX_NO_BITS is not the way to go.  What you
are really testing here is that the value must fit in either a signed or
unsigned value.  You should write it like that.  The function should not
be named has_overflow--that overloads an existing function.  The
function needs a comment.

(Of course this relocation stuff should move to generic code anyhow, but
that is not your problem.)


> + case elfcpp::R_ARM_ABS8:
> +   //FIXME: This should handles properly the dynamic linking against the shared object.
> +   break;

This comment does not make sense as written.  There should be a space
after the "//".  It may be appropriate to warn if the symbol would
normally require a dynamic relocation.  The same may be true in
scan::local.

> + case elfcpp::R_ARM_ABS8:
> +   if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
> + output_section))
> +   reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
> + has_thumb_bit);
> +   break;

Testing should_apply_static_reloc here only makes sense if we warn in
scan::local and scan::global.  Otherwise we will in effect silently
ignore the relocation.

Thanks for sending the patch.

Ian

[-- Attachment #2: binutil-gold-arm-rel-abs8-02.patch --]
[-- Type: application/octet-stream, Size: 3701 bytes --]

diff -E -B -b -r -u binutils.orig/src/gold/arm.cc binutils.gold/src/gold/arm.cc
--- binutils.orig/src/gold/arm.cc	2009-08-11 10:09:14.000000000 -0700
+++ binutils.gold/src/gold/arm.cc	2009-09-01 12:26:04.000000000 -0700
@@ -117,6 +117,22 @@
     return as_signed > max || as_signed < min;
   }
 
+  // Detects overflow of an NO_BITS integer stored in a uint32_t when it
+  // fits in the given number of bits as either a signed or unsigned value.
+  // For example, has_signed_unsigned_overflow<8> would check
+  // -128 <= bits <= 255
+  template<int no_bits>
+  static inline bool
+  has_signed_unsigned_overflow(uint32_t bits) {
+    gold_assert(no_bits >= 2 && no_bits <= 32);
+  	if (no_bits == 32)
+  		return false;
+  	int32_t max = (1 << no_bits) - 1;
+  	int32_t min = -(1 << (no_bits - 1));
+    int32_t as_signed = static_cast<int32_t>(bits);
+    return as_signed > max || as_signed < min;
+  }
+
   // Select bits from A and B using bits in MASK.  For each n in [0..31],
   // the n-th bit in the result is chosen from the n-th bits of A and B.
   // A zero selects A and a one selects B.
@@ -555,6 +571,26 @@
   }
 
  public:
+
+  // R_ARM_ABS8: S + A
+  static inline typename This::Status
+  abs8(unsigned char *view,
+	const Sized_relobj<32, big_endian>* object,
+	const Symbol_value<32>* psymval, bool has_thumb_bit)
+  {
+	typedef typename elfcpp::Swap<8, big_endian>::Valtype Valtype;
+	typedef typename elfcpp::Swap<32, big_endian>::Valtype Reltype;
+	Valtype* wv = reinterpret_cast<Valtype*> (view);
+	Valtype val = elfcpp::Swap<8, big_endian>::readval(wv);
+	Reltype addend = utils::sign_extend<8>(val);
+	Reltype x = This::arm_symbol_value(object, psymval, addend, has_thumb_bit);
+	val = utils::bit_select(val, x, 0xffU);
+	elfcpp::Swap<8, big_endian>::writeval(wv, val);
+	return (utils::has_signed_unsigned_overflow<8>(x)
+		? This::STATUS_OVERFLOW
+		: This::STATUS_OKAY);
+  }
+
   // R_ARM_ABS32: (S + A) | T
   static inline typename This::Status
   abs32(unsigned char *view,
@@ -1074,6 +1110,15 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+    if (parameters->options().output_is_position_independent())
+    {
+  		// FIXME: Create a dynamic relocation for this location.
+			gold_warning(_("%s: symbol would normally require a dynamic reloc %u."),
+					 object->name().c_str(), r_type);
+    }
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       // If building a shared library (or a position-independent
       // executable), we need to create a dynamic relocation for
@@ -1187,6 +1232,16 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+		// Make a dynamic relocation if necessary.
+		if (gsym->needs_dynamic_reloc(Symbol::ABSOLUTE_REF))
+    {
+  		// FIXME: Create a dynamic relocation for this location.
+			gold_warning(_("%s: %s symbol would normally require a dynamic reloc %u."),
+					 object->name().c_str(), gsym->demangled_name().c_str(), r_type);
+    }
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       {
 	// Make a dynamic relocation if necessary.
@@ -1595,6 +1650,13 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+	  if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
+					output_section))
+		  reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
+							has_thumb_bit);
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, true,
 				    output_section))
@@ -1764,6 +1826,9 @@
     case elfcpp::R_ARM_NONE:
       return 0;
 
+  	case elfcpp::R_ARM_ABS8:
+  		return 1;
+
     case elfcpp::R_ARM_ABS32:
     case elfcpp::R_ARM_REL32:
     case elfcpp::R_ARM_THM_CALL:

Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

[Ian Lance Taylor]

Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:

> By the way, I notice that there is a mix of spaces and tabs used in the source code for indents.
> I have done my best trying to follow the original stile.
> But it might be a formal policy I should follow?

GNU style is tabs every 8 spaces, and gold should ideally use tabs for
long indents.  However, there are many cases where it uses spaces,
depending upon who wrote the code originally and how their editor was
configured, and that is acceptable.

More generally, there are some pointers to style guides in the README.
gold's style is an attempt to adapt GNU style to C++, or vice-versa.

Ian

[PATCH Take 2] Gold: Added R_ARM_ABS8 relocation

[Viktor Kutuzov]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

Thanks, Ian, for the review.

Please find attached the updated patch.

By the way, I notice that there is a mix of spaces and tabs used in the source code for indents.
I have done my best trying to follow the original stile.
But it might be a formal policy I should follow?

Best regards,
Viktor

* gold/arm.cc: Added R_ARM_ABS8 relocation

----- Original Message ----- 
From: "Ian Lance Taylor" <iant@google.com>
To: "Viktor Kutuzov" <vkutuzov@accesssoftek.com>
Cc: <binutils@sourceware.org>
Sent: Monday, August 31, 2009 5:56 PM
Subject: Re: [PATCH] Gold: Added R_ARM_ABS8 relocation


Viktor Kutuzov <vkutuzov@accesssoftek.com> writes:

> +  template<int min_no_bits, int max_no_bits>
> +  static inline bool has_overflow(uint32_t bits) {
> +  gold_assert(max_no_bits >= 0 && max_no_bits <= 32);
> +  gold_assert(min_no_bits >= 0 && min_no_bits <= 32);
> +  gold_assert(min_no_bits <= max_no_bits);
> +  if (min_no_bits == 32)
> +  return false;
> +  int32_t max = (1 << (max_no_bits - 1)) - 1;
> +  int32_t min = -(1 << (min_no_bits - 1));
> +  int32_t as_signed = static_cast<int32_t> (bits);
> +  return as_signed > max || as_signed < min;
> +  }

Passing both MIN_NO_BITS and MAX_NO_BITS is not the way to go.  What you
are really testing here is that the value must fit in either a signed or
unsigned value.  You should write it like that.  The function should not
be named has_overflow--that overloads an existing function.  The
function needs a comment.

(Of course this relocation stuff should move to generic code anyhow, but
that is not your problem.)


> + case elfcpp::R_ARM_ABS8:
> +   //FIXME: This should handles properly the dynamic linking against the shared object.
> +   break;

This comment does not make sense as written.  There should be a space
after the "//".  It may be appropriate to warn if the symbol would
normally require a dynamic relocation.  The same may be true in
scan::local.

> + case elfcpp::R_ARM_ABS8:
> +   if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
> + output_section))
> +   reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
> + has_thumb_bit);
> +   break;

Testing should_apply_static_reloc here only makes sense if we warn in
scan::local and scan::global.  Otherwise we will in effect silently
ignore the relocation.

Thanks for sending the patch.

Ian

[-- Attachment #2: binutil-gold-arm-rel-abs8-02.patch --]
[-- Type: application/octet-stream, Size: 3811 bytes --]

diff -E -B -b -r -u binutils.orig/src/gold/arm.cc binutils.gold/src/gold/arm.cc
--- binutils.orig/src/gold/arm.cc	2009-08-11 10:09:14.000000000 -0700
+++ binutils.gold/src/gold/arm.cc	2009-09-01 12:26:04.000000000 -0700
@@ -117,6 +117,22 @@
     return as_signed > max || as_signed < min;
   }
 
+  // Detects overflow of an NO_BITS integer stored in a uint32_t when it
+  // fits in the given number of bits as either a signed or unsigned value.
+  // For example, has_signed_unsigned_overflow<8> would check
+  // -128 <= bits <= 255
+  template<int no_bits>
+  static inline bool
+  has_signed_unsigned_overflow(uint32_t bits) {
+    gold_assert(no_bits >= 2 && no_bits <= 32);
+  	if (no_bits == 32)
+  		return false;
+  	int32_t max = (1 << no_bits) - 1;
+  	int32_t min = -(1 << (no_bits - 1));
+    int32_t as_signed = static_cast<int32_t>(bits);
+    return as_signed > max || as_signed < min;
+  }
+
   // Select bits from A and B using bits in MASK.  For each n in [0..31],
   // the n-th bit in the result is chosen from the n-th bits of A and B.
   // A zero selects A and a one selects B.
@@ -555,6 +571,26 @@
   }
 
  public:
+
+  // R_ARM_ABS8: S + A
+  static inline typename This::Status
+  abs8(unsigned char *view,
+	const Sized_relobj<32, big_endian>* object,
+	const Symbol_value<32>* psymval, bool has_thumb_bit)
+  {
+	typedef typename elfcpp::Swap<8, big_endian>::Valtype Valtype;
+	typedef typename elfcpp::Swap<32, big_endian>::Valtype Reltype;
+	Valtype* wv = reinterpret_cast<Valtype*> (view);
+	Valtype val = elfcpp::Swap<8, big_endian>::readval(wv);
+	Reltype addend = utils::sign_extend<8>(val);
+	Reltype x = This::arm_symbol_value(object, psymval, addend, has_thumb_bit);
+	val = utils::bit_select(val, x, 0xffU);
+	elfcpp::Swap<8, big_endian>::writeval(wv, val);
+	return (utils::has_signed_unsigned_overflow<8>(x)
+		? This::STATUS_OVERFLOW
+		: This::STATUS_OKAY);
+  }
+
   // R_ARM_ABS32: (S + A) | T
   static inline typename This::Status
   abs32(unsigned char *view,
@@ -1074,6 +1110,15 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+    if (parameters->options().output_is_position_independent())
+    {
+  		// FIXME: Create a dynamic relocation for this location.
+			gold_warning(_("%s: symbol would normally require a dynamic reloc %u."),
+					 object->name().c_str(), r_type);
+    }
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       // If building a shared library (or a position-independent
       // executable), we need to create a dynamic relocation for
@@ -1187,6 +1232,16 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+		// Make a dynamic relocation if necessary.
+		if (gsym->needs_dynamic_reloc(Symbol::ABSOLUTE_REF))
+    {
+  		// FIXME: Create a dynamic relocation for this location.
+			gold_warning(_("%s: %s symbol would normally require a dynamic reloc %u."),
+					 object->name().c_str(), gsym->demangled_name().c_str(), r_type);
+    }
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       {
 	// Make a dynamic relocation if necessary.
@@ -1595,6 +1650,13 @@
     case elfcpp::R_ARM_NONE:
       break;
 
+	case elfcpp::R_ARM_ABS8:
+	  if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, false,
+					output_section))
+		  reloc_status = Arm_relocate_functions::abs8(view, object, psymval,
+							has_thumb_bit);
+	  break;
+
     case elfcpp::R_ARM_ABS32:
       if (should_apply_static_reloc(gsym, Symbol::ABSOLUTE_REF, true,
 				    output_section))
@@ -1764,6 +1826,9 @@
     case elfcpp::R_ARM_NONE:
       return 0;
 
+  	case elfcpp::R_ARM_ABS8:
+  		return 1;
+
     case elfcpp::R_ARM_ABS32:
     case elfcpp::R_ARM_REL32:
     case elfcpp::R_ARM_THM_CALL: