From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1093 invoked by alias); 3 Sep 2009 18:02:26 -0000 Received: (qmail 812 invoked by uid 22791); 3 Sep 2009 18:02:25 -0000 X-SWARE-Spam-Status: No, hits=-0.0 required=5.0 tests=AWL,BAYES_00,FAKE_REPLY_C,J_CHICKENPOX_83 X-Spam-Check-By: sourceware.org Received: from mail.accesssoftek.com (HELO mail.accesssoftek.com) (63.145.236.71) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 03 Sep 2009 18:02:19 +0000 Received: from andreic6e7fe55 (67.102.105.77) by mail.accesssoftek.com (172.16.0.71) with Microsoft SMTP Server id 8.1.393.1; Thu, 3 Sep 2009 11:01:29 -0700 Message-ID: From: Viktor Kutuzov To: Subject: Re: [PATCH Take 2] Gold: Added R_ARM_ABS8 relocation Date: Thu, 03 Sep 2009 18:02:00 -0000 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_NextPart_000_076E_01CA2C86.0072BE00" X-IsSubscribed: yes Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org X-SW-Source: 2009-09/txt/msg00101.txt.bz2 ------=_NextPart_000_076E_01CA2C86.0072BE00 Content-Type: text/plain; format=flowed; charset="iso-8859-1"; reply-type=original Content-Transfer-Encoding: 7bit Content-length: 2597 Is this patch Ok? Best regards, Viktor ----- Original Message ----- From: "Viktor Kutuzov" To: 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" To: "Viktor Kutuzov" Cc: Sent: Monday, August 31, 2009 5:56 PM Subject: Re: [PATCH] Gold: Added R_ARM_ABS8 relocation Viktor Kutuzov writes: > + template > + 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 (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 ------=_NextPart_000_076E_01CA2C86.0072BE00 Content-Type: application/octet-stream; name="binutil-gold-arm-rel-abs8-02.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="binutil-gold-arm-rel-abs8-02.patch" Content-length: 3701 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 + 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(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 (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: ------=_NextPart_000_076E_01CA2C86.0072BE00--