Bug 21523 - Relocation for R_ARM_THM_ALU_PREL_11_0 is not calculated correctly
Summary: Relocation for R_ARM_THM_ALU_PREL_11_0 is not calculated correctly
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-25 17:56 UTC by clegg89
Modified: 2017-06-07 12:54 UTC (History)
2 users (show)

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


Attachments
Test Case (5.59 KB, application/x-gzip)
2017-05-29 16:46 UTC, clegg89
Details
Patch to fix R_ARM_THM_ALU_PREL_11_0 relocation (460 bytes, patch)
2017-05-29 16:56 UTC, clegg89
Details | Diff
Patch to fix R_ARM_THM_ALU_PREL_11_0 (git pull) (461 bytes, patch)
2017-05-29 17:04 UTC, clegg89
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description clegg89 2017-05-25 17:56:56 UTC
I am using the gcc-arm-none-eabi version 6-2017-q1-update, which uses LD version 2.26.

I am attempting to link in some vendor libraries along with my own source code. The libraries were compiled with a different toolchain (IAR), but contain ARM ELF Relocatable object files. Using arm-none-eabi-objdump I am able to view the contents of the object files just fine.

For the must part I am able to link correctly, however I get a single link error:

relocation truncated to fit: R_ARM_THM_ALU_PREL_11_0

This relocation is used by the ADR instruction to provide an offset from the PC. Based on where the symbol ends up in relation to the relocation, the ADR may be either an ADD or SUB immediate instruction from PC. The link appears only to work correctly in the case of an ADD.

I traced the error down to the elf32_arm_final_link_relocate. Specifically this case statement:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/elf32-arm.c;h=1725c222696b846c29cefdea1fd414d88626839f;hb=HEAD#l10487

Here the addend and relocation value are calculated correctly:

  relocation -= Pa (input_section->output_section->vma
        + input_section->output_offset
        + rel->r_offset);

However then the value is assigned incorrectly:

  value = relocation;

This should be:

  value = abs(relocation);

It was originally written this way but changed for some reason. The commit that changed it was:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;f=bfd/elf32-arm.c;h=b6518b3871859f9eeb7653bf2f3baaa43fa0a5d0

I am not sure why the abs() was considered useless in this case. The commit message mentions warnings from clang.

Regardless, the assignment without the abs() call will cause the linker to think an overflow has occurred when one has not. If the resulting relocation is negative, and no abs() is performed, the following overflow check will trigger a false error for negative values:

  if (value >= 0x1000)
    return bfd_reloc_overflow;

Not only that, but the following check performed later will never evaluate to true:

  if (relocation < 0)
    insn |= 0xa00000;

Proposed solution would be to change back to:

  value = abs(relocation);

Please let me know if anymore information is needed. I can try to generate a simple example which demonstrates the problem if needed.

Thanks
Comment 1 clegg89 2017-05-28 15:19:54 UTC
Reported in launchpad: https://bugs.launchpad.net/gcc-arm-embedded/+bug/1693831
Comment 2 clegg89 2017-05-28 15:30:35 UTC
I have rebuilt binutils and verified that linking my project with the change described fixes my issue. Here is the patch:

diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index 1725c22..b1297d9 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -10509 +10509 @@ elf32_arm_final_link_relocate (reloc_howto_type *           howto,
-       value = relocation;
+       value = abs(relocation);

I will try to develop a simple test case which demonstrates the issue. I am referring to another bug report (https://sourceware.org/bugzilla/show_bug.cgi?id=21458) which should help in generating a suitable test case.

Is there anything else I need to work on in order to submit this patch? I am happy to provide anything necessary but I'm unclear as to the exact process.
Comment 3 clegg89 2017-05-29 16:46:09 UTC
Created attachment 10071 [details]
Test Case

The object file test.o contains two relocations, both of type R_ARM_THM_ALU_PREL_11_0, as can be seen by arm-none-eabi-objdump:

arm-none-eabi-objdump -rd test.o:
test.o:     file format elf32-littlearm


Disassembly of section .text:

00000000 <getOtherValue>:
   0:   f2af 0004       subw    r0, pc, #4
                        0: R_ARM_THM_ALU_PREL_11_0      aValue
   4:   4770            bx      lr
        ...

00000008 <aValue>:
   8:   6854 7369 6920 2073 2061 6574 7473 000a     This is a test..

00000018 <getValue>:
  18:   f2af 0004       subw    r0, pc, #4
                        18: R_ARM_THM_ALU_PREL_11_0     aValue
  1c:   4770            bx      lr

When attempting to link this object file, the first relocation (in getOtherValue) is performed without issue. However, the second relocation (in getValue) throws an error:

test.o: In function `getValue':
test:(.text+0x18): relocation truncated to fit: R_ARM_THM_ALU_PREL_11_0 against symbol `aValue' defined in .text section in test.o

Details on how to build the project can be found in README.md
Comment 4 clegg89 2017-05-29 16:56:13 UTC
Created attachment 10072 [details]
Patch to fix R_ARM_THM_ALU_PREL_11_0 relocation
Comment 5 clegg89 2017-05-29 17:04:42 UTC
Created attachment 10073 [details]
Patch to fix R_ARM_THM_ALU_PREL_11_0 (git pull)
Comment 6 Sourceware Commits 2017-05-30 14:09:09 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 8c65b54f18c03ffb844e1cbaa3b46b43444ff9e7
Author: Casey Smith <clegg89@gmail.com>
Date:   Tue May 30 15:07:56 2017 +0100

    Fix calculation of R_ARM_RHM_ALU_PREL_11_0 relocation when used with a SUB instruction.
    
    	PR ld/21523
    	* elf32-arm.c (elf32_arm_final_link_relocate): Install an absolute
    	value when processing the R_ARM_THM_ALU_PREL_11_0 reloc.
Comment 7 Nick Clifton 2017-05-30 14:13:08 UTC
Hi Casey,

  Thanks very much for reporting this bug and providing a patch.

  I am not sure why clang was complaining about the abs() invocation
  but it obviously is needed, so I am happy to put it back.  Patch
  applied.

Cheers
  Nick
Comment 8 clegg89 2017-05-31 13:21:06 UTC
(In reply to Nick Clifton from comment #7)
> Hi Casey,
> 
>   Thanks very much for reporting this bug and providing a patch.
> 
>   I am not sure why clang was complaining about the abs() invocation
>   but it obviously is needed, so I am happy to put it back.  Patch
>   applied.
> 
> Cheers
>   Nick

Hi Nick,

Thanks for getting this in.

Someone on the GNU ARM Embedded Toolchain project mentioned I should ask for this to be backported to 2.28 so that it makes it into the 2017q2 release for the GNU ARM Embedded Toolchain. I'm not sure how difficult that is or what it involves, but if it's possible I would definitely appreciate it.

Thanks again!
Comment 9 Nick Clifton 2017-05-31 14:48:43 UTC
Hi Casey,

> https://sourceware.org/bugzilla/show_bug.cgi?id=21523

> Someone on the GNU ARM Embedded Toolchain project mentioned I should ask for
> this to be backported to 2.28 so that it makes it into the 2017q2 release for
> the GNU ARM Embedded Toolchain. I'm not sure how difficult that is or what it
> involves, but if it's possible I would definitely appreciate it.

It should be simple - send an email to Tristan Gringold, the binutils release
manager (gingold@adacore.com) and ask him if it is OK to apply the patch to the
branch.  He will probably say yes.  Assuming that he does you can go ahead and
check it in.  Or - if you do not have write permission on the binutils repository,
ping me and I will do it for you.

Cheers
  Nick
Comment 10 clegg89 2017-06-06 17:13:31 UTC
(In reply to Nick Clifton from comment #9)
> Hi Casey,
> 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=21523
> 
> > Someone on the GNU ARM Embedded Toolchain project mentioned I should ask for
> > this to be backported to 2.28 so that it makes it into the 2017q2 release for
> > the GNU ARM Embedded Toolchain. I'm not sure how difficult that is or what it
> > involves, but if it's possible I would definitely appreciate it.
> 
> It should be simple - send an email to Tristan Gringold, the binutils release
> manager (gingold@adacore.com) and ask him if it is OK to apply the patch to
> the
> branch.  He will probably say yes.  Assuming that he does you can go ahead
> and
> check it in.  Or - if you do not have write permission on the binutils
> repository,
> ping me and I will do it for you.
> 
> Cheers
>   Nick

Hi Nick,

I messaged Tristan and he said to go ahead and backport it. I do not have write access so if you're able to cherry-pick the commit that would be much appreciated.

Thanks,
Casey
Comment 11 Sourceware Commits 2017-06-07 12:54:40 UTC
The binutils-2_28-branch branch has been updated by Nick Clifton <nickc@sourceware.org>:

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

commit 1898f468867e1f997a0cc10ad61647a3b623c4b7
Author: Casey Smith <clegg89@gmail.com>
Date:   Wed Jun 7 13:52:58 2017 +0100

    Import a fix from upstream that allows the ARM ADR pseudo-instruction to work when generating a SUB instruction.
    
    	2017-05-30  Casey Smith  <clegg89@gmail.com>
    
    	PR ld/21523
    	* elf32-arm.c (elf32_arm_final_link_relocate): Install an absolute
    	value when processing the R_ARM_THM_ALU_PREL_11_0 reloc.