Bug 16202 - ABS8 and ABS16 get wrong addend on ARM-ELF (big endian)
Summary: ABS8 and ABS16 get wrong addend on ARM-ELF (big endian)
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-22 02:49 UTC by ma.jiang
Modified: 2014-01-13 16:13 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Proposed patch (369 bytes, patch)
2014-01-03 14:34 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ma.jiang 2013-11-22 02:49:36 UTC
on ARM-ELF , in function elf32_arm_final_link_relocate, addend is get from :
      addend = bfd_get_32 (input_bfd, hit_data) & howto->src_mask;
.There is a little problem, becasue howto->scr_mask is a constant that designed for little endian.
for example , in the testcase in gas testsuit:
.syntax unified
        .byte   x -128
        .byte   x +255
        .short  y -32768
        .short  y +65535

the first ABS8 reloc for x should get a addend -128.But on big endian ARM, "bfd_get_32 (input_bfd, hit_data)" get a  0x80ff8000, and with a howto->src_mask=0xff, the final addend is 0.

in the ABS8 branch, the addend is used directly.
    case R_ARM_ABS8:
      value += addend;

      /* There is no way to tell whether the user intended to use a signed or
	 unsigned addend.  When checking for overflow we accept either,
	 as specified by the AAELF.  */
      if ((long) value > 0xff || (long) value < -0x80)
	return bfd_reloc_overflow;

      bfd_put_8 (input_bfd, value, hit_data);
      return bfd_reloc_ok;

Finally, a 0 is put into the object file, which of course is totally wrong.
ABS16 has the same problem.
============================================================================
Fix for this problem is quite strait-forward. IN ABS8/ABS16 branch, we can fetch addend once more using correct bfd_get_8/bfd_get_16, as following codes :

    case R_ARM_ABS8:
      addend = bfd_get_8 (input_bfd, hit_data); /*fectch addend again with bfd_get_8 */

      value += addend;

      /* There is no way to tell whether the user intended to use a signed or
	 unsigned addend.  When checking for overflow we accept either,
	 as specified by the AAELF.  */
      if ((long) value > 0xff || (long) value < -0x80)
	return bfd_reloc_overflow;

      bfd_put_8 (input_bfd, value, hit_data);
      return bfd_reloc_ok;
.
Comment 1 Nick Clifton 2014-01-03 14:34:55 UTC
Created attachment 7337 [details]
Proposed patch
Comment 2 Nick Clifton 2014-01-03 14:37:05 UTC
Hi Ma,

  I do not think that refetching the addend in the ABS8 and ABS16 cases is the right thing to do.  There could be other relocations that are affected by the same problem.  Instead I think that the correct thing to do is to fetch the addend using the proper bfd_get_XX macro in the first place.

  Please could you try out the uploaded patch and let me know if it works for you ?

Cheers
  Nick
Comment 3 ma.jiang 2014-01-06 01:43:20 UTC
(In reply to Nick Clifton from comment #2)
> Hi Ma,

  I do not think that refetching the addend in the ABS8 and ABS16
> cases is the right thing to do.  There could be other relocations that are
> affected by the same problem.  Instead I think that the correct thing to do
> is to fetch the addend using the proper bfd_get_XX macro in the first place.
> Please could you try out the uploaded patch and let me know if it works for
> you ?

Cheers
  Nick

==============================================================
Hi Nick,
      Thanks for the reply. 
I have carefully read the proposed patch.
Comment 4 ma.jiang 2014-01-06 06:53:59 UTC
(In reply to Nick Clifton from comment #1)
> Created attachment 7337 [details]
Proposed patch

===============================================
This patch can solve the abs8 problem.In fact,I have considered a simliar fix. The reason that I choose to fix the ABS8 branch not the top one, is That I found many branches(eg, R_ARM_THM_ABS5) had already refetched the addend. I think it's more safe to follow their way--fix the problem and make sure not to throw new problems.Changing the top addend may touch many other branches, I could not make sure all of them were ok with the change. 

Today, after reading all branches of elf32_arm_final_link_relocate again, I think it is ok to change the top addend. But still there are something need to discuss.
First, some relocs require to read more bytes than its bitsize shows.For example,R_ARM_ABS12 is set to the first byte of the instruction, and its bitsize is 12. So, using bfd_get_16 to get addend is not right. R_ARM_ABS12 is ok, as it seems only used by vxworks,and does not get addend from instructions(globals->use_rel=0). For other similar relocs such as R_ARM_ALU_PCREL7_0,R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS, addend is refeteched in their branches. So, This is just a hidden danger for now.But we should pay attention.
Second, some relocs (eg,R_ARM_THM_JUMP6) refecthed the addend. They will not need the refetch process any more if the proposed patch applied. It will be better if the patch do some clean for this, I think.
 
At last ,I think the Root Cause of this problem is the reloc_howto mechanism. This mechanism failed to provide a clear abstraction for relocations, as it exposed too many details. As a result, almost every reloc process branch has to do some special things.In fact, the only thing that caller should know and set is the relocation type. The reloc_howto mechanism should automaticlly finished remaining dirty work such as adjusting the mask for big endian target, extracting the addend, computing the reloc result, and Writing back results. Adding some new callbacks for reloc_howto_struct can do the job, this is not very hard theoretically, but the real amount of work could be horrible.
Comment 5 cvs-commit@gcc.gnu.org 2014-01-13 16:10:49 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  fd0fd00cded9d641ee55a09edcb62366213357a6 (commit)
      from  f8de51293246a17166da9a2744c1eb5ab956d67b (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=fd0fd00cded9d641ee55a09edcb62366213357a6

commit fd0fd00cded9d641ee55a09edcb62366213357a6
Author: Ma Jiang <ma.jiang@zte.com.cn>
Date:   Mon Jan 13 16:06:28 2014 +0000

    2014-01-13  Ma Jiang  <ma.jiang@zte.com.cn>
    
    	PR ld/16202
    	* elf32-arm.c (elf32_arm_final_link_relocate): Refetch addends for
    	R_ARM_ABS8 and R_ARM_ABS16.

-----------------------------------------------------------------------

Summary of changes:
 bfd/ChangeLog   |    6 ++++++
 bfd/elf32-arm.c |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)
Comment 6 Nick Clifton 2014-01-13 16:13:34 UTC
Hi Ma,

  OK, I agree - just updating ABS8 and ABS16 is the safer change.  So I have checked in the patch you suggested, along with one minor change - the addend is only refetched when using REL relocs.

Cheers
  Nick