public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation
@ 2009-09-11 17:50 Viktor Kutuzov
  2009-09-11 18:39 ` Ian Lance Taylor
  0 siblings, 1 reply; 7+ messages in thread
From: Viktor Kutuzov @ 2009-09-11 17:50 UTC (permalink / raw)
  To: binutils

[-- 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:

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation
@ 2009-09-03 18:02 Viktor Kutuzov
  2009-09-18  0:48 ` Ian Lance Taylor
  0 siblings, 1 reply; 7+ messages in thread
From: Viktor Kutuzov @ 2009-09-03 18:02 UTC (permalink / raw)
  To: binutils

[-- 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:

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [PATCH] gold: add cast to gold_unreachable to workaround gcc giving   invalid "no return statement" warnings
@ 2009-08-13 15:09 Mikolaj Zalewski
  2009-08-31 21:49 ` [PATCH] Gold: Added R_ARM_ABS8 relocation Viktor Kutuzov
  0 siblings, 1 reply; 7+ messages in thread
From: Mikolaj Zalewski @ 2009-08-13 15:09 UTC (permalink / raw)
  To: binutils

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

  I couldn't compile gold today because of the "no return statement"
warning when using gold_unreachable. At
http://gcc.gnu.org/ml/gcc-bugs/2007-11/msg01605.html I've found a
workaround that should work for gcc >= 3.4. Such a patch that affects
only gold_unreachable is ok?

2009-08-13  Mikolaj Zalewski  <mikolajz@google.com>

	* gold.h (gold_unreachable): Add a cast.

[-- Attachment #2: 0001-gold-add-cast-to-gold_unreachable-to-workaround-gcc.patch --]
[-- Type: text/x-diff, Size: 1041 bytes --]

From 2c4c78358538c7b260a2d23a66add87e28d9a1ad Mon Sep 17 00:00:00 2001
From: Mikolaj Zalewski <mikolajz@google.com>
Date: Thu, 13 Aug 2009 16:56:44 +0200
Subject: [PATCH] gold: add cast to gold_unreachable to workaround gcc giving invalid "no return statement" warnings

---
 gold/gold.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/gold/gold.h b/gold/gold.h
index 1319699..3c9971d 100644
--- a/gold/gold.h
+++ b/gold/gold.h
@@ -255,9 +255,11 @@ gold_nomem() ATTRIBUTE_NORETURN;
 
 // This macro and function are used in cases which can not arise if
 // the code is written correctly.
+// Note: the cast of __FUNCTION__ is needed to workaround gcc bug
+// http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30988 causing invalid warnings.
 
 #define gold_unreachable() \
-  (gold::do_gold_unreachable(__FILE__, __LINE__, __FUNCTION__))
+  (gold::do_gold_unreachable(__FILE__, __LINE__, (const char *)__FUNCTION__))
 
 extern void do_gold_unreachable(const char*, int, const char*)
   ATTRIBUTE_NORETURN;
-- 
1.5.4.3


^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-09-18  0:48 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-11 17:50 [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation Viktor Kutuzov
2009-09-11 18:39 ` Ian Lance Taylor
2009-09-11 18:46   ` Viktor Kutuzov
  -- strict thread matches above, loose matches on Subject: below --
2009-09-03 18:02 Viktor Kutuzov
2009-09-18  0:48 ` Ian Lance Taylor
2009-08-13 15:09 [PATCH] gold: add cast to gold_unreachable to workaround gcc giving invalid "no return statement" warnings Mikolaj Zalewski
2009-08-31 21:49 ` [PATCH] Gold: Added R_ARM_ABS8 relocation Viktor Kutuzov
2009-09-01  0:56   ` Ian Lance Taylor
2009-09-01 19:53     ` [PATCH Take 2] " Viktor Kutuzov
2009-09-01 21:24       ` Ian Lance Taylor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).