[BFD,AARCH64] Properly truncate no overflow checking relocation value for load/store immediate.

Message ID 5b1b8fa9-a8b2-ff42-4a93-c8875590e27b@foss.arm.com
State New
Headers show
Series
  • [BFD,AARCH64] Properly truncate no overflow checking relocation value for load/store immediate.
Related show

Commit Message

Renlin Li April 26, 2018, 1:05 p.m.
Hi all,


In aarch64, there are relocations for the unsigned immediate offset of load/store instruction.
The size of the immediate field is 12-bit. In the scaled case, not all 12-bit will be used.

For example: R_AARCH64_LD64_GOT_LO12_NC
Set the LD/ST immediate field to bits [11:3] of relocated value. No overflow
check.

In this case, the top 3 bits of the immediate field of the load instruction should
be all 0. Only 9 bits from the value should be written into the instruction
encoding.

For relocations with over-flow check, if the value is larger than 12-bits, it
will be caught by the checking mechanism.

But for relocations without no overflow check, the value could be larger than
12 bits, after scaling, it might take more bits than the specification
specified. The value should be properly truncated.

The patch here correct the bitsize field of a few relocations, and the bitsize
field is used to properly truncate the value written into the instruction encoding.

Okay to commit?

bfd/ChangeLog:

2018-04-26  Renlin Li  <renlin.li@arm.com>

	* elfnn-aarch64.c (elfNN_aarch64_howto_table): Correct the bitsize
	field of R_AARCH64_LD64_GOT_LO12_NC, R_AARCH64_P32_LD32_GOT_LO12_NC,
	R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC,
	R_AARCH64_P32_LD32_GOTTPREL_LO12_NC,
	R_AARCH64_LDST16_ABS_LO12_NC,
	R_AARCH64_P32_TLSDESC_LD32_LO12_NC.
	R_AARCH64_P32_TLSDESC_LD32_LO12_NC.
	R_AARCH64_TLSDESC_LD64_LO12_NC.
	R_AARCH64_LDST32_ABS_LO12_NC,
	R_AARCH64_LDST64_ABS_LO12_NC,
	R_AARCH64_LDST128_ABS_LO12_NC,
	* elfxx-aarch64.c (_bfd_aarch64_elf_put_addend): Truncate the
	immediate value for load/store instruction.

ld/ChangeLog:

2018-04-26 Renlin Li  <renlin.li@arm.com>

	* testsuite/ld-aarch64/emit-relocs-115.d: Update test with new value.
	* testsuite/ld-aarch64/emit-relocs-534.d: Likewise.
	* testsuite/ld-aarch64/emit-relocs-555.d: Likewise.

Comments

Peter Smith April 26, 2018, 2:53 p.m. | #1
Hello Renlin,

I've checked that the patch works on an example I had that previously
gave different results to Gold and an in-progress implementation in
LLD. To the best of my knowledge the bitsizes in the HOWTO and the
method to truncate the result will work. One thing that I'm a bit
curious about is that relocations such as
BFD_RELOC_AARCH64_LDST64_LO12 use "value = PG_OFFSET (value +
addend);" to calculate the addend, whereas relocations like
BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC use "value = value +
addend". In the former case the truncation is done in
_bfd_aarch64_elf_resolve_relocation whereas in the latter you are
having to truncate in _bfd_aarch64_elf_put_addend; it seems like you
might be able to use PG_OFFSET for all the LO12 relocations and not
need to put an extra truncation step in? I'm no expert in the BFD
codebase, so there may be good reasons not to do that so just treat
that as an observation.

Thanks for the fix.

Peter

On 26 April 2018 at 14:05, Renlin Li <renlin.li@foss.arm.com> wrote:
> Hi all,

>

>

> In aarch64, there are relocations for the unsigned immediate offset of

> load/store instruction.

> The size of the immediate field is 12-bit. In the scaled case, not all

> 12-bit will be used.

>

> For example: R_AARCH64_LD64_GOT_LO12_NC

> Set the LD/ST immediate field to bits [11:3] of relocated value. No overflow

> check.

>

> In this case, the top 3 bits of the immediate field of the load instruction

> should

> be all 0. Only 9 bits from the value should be written into the instruction

> encoding.

>

> For relocations with over-flow check, if the value is larger than 12-bits,

> it

> will be caught by the checking mechanism.

>

> But for relocations without no overflow check, the value could be larger

> than

> 12 bits, after scaling, it might take more bits than the specification

> specified. The value should be properly truncated.

>

> The patch here correct the bitsize field of a few relocations, and the

> bitsize

> field is used to properly truncate the value written into the instruction

> encoding.

>

> Okay to commit?

>

> bfd/ChangeLog:

>

> 2018-04-26  Renlin Li  <renlin.li@arm.com>

>

>         * elfnn-aarch64.c (elfNN_aarch64_howto_table): Correct the bitsize

>         field of R_AARCH64_LD64_GOT_LO12_NC, R_AARCH64_P32_LD32_GOT_LO12_NC,

>         R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC,

>         R_AARCH64_P32_LD32_GOTTPREL_LO12_NC,

>         R_AARCH64_LDST16_ABS_LO12_NC,

>         R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>         R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>         R_AARCH64_TLSDESC_LD64_LO12_NC.

>         R_AARCH64_LDST32_ABS_LO12_NC,

>         R_AARCH64_LDST64_ABS_LO12_NC,

>         R_AARCH64_LDST128_ABS_LO12_NC,

>         * elfxx-aarch64.c (_bfd_aarch64_elf_put_addend): Truncate the

>         immediate value for load/store instruction.

>

> ld/ChangeLog:

>

> 2018-04-26 Renlin Li  <renlin.li@arm.com>

>

>         * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new

> value.

>         * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>         * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.
Nick Clifton April 26, 2018, 3:04 p.m. | #2
Hi Renlin,

> bfd/ChangeLog:

> 

> 2018-04-26  Renlin Li  <renlin.li@arm.com>

> 

>     * elfnn-aarch64.c (elfNN_aarch64_howto_table): Correct the bitsize

>     field of R_AARCH64_LD64_GOT_LO12_NC, R_AARCH64_P32_LD32_GOT_LO12_NC,

>     R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC,

>     R_AARCH64_P32_LD32_GOTTPREL_LO12_NC,

>     R_AARCH64_LDST16_ABS_LO12_NC,

>     R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>     R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>     R_AARCH64_TLSDESC_LD64_LO12_NC.

>     R_AARCH64_LDST32_ABS_LO12_NC,

>     R_AARCH64_LDST64_ABS_LO12_NC,

>     R_AARCH64_LDST128_ABS_LO12_NC,

>     * elfxx-aarch64.c (_bfd_aarch64_elf_put_addend): Truncate the

>     immediate value for load/store instruction.

> 

> ld/ChangeLog:

> 

> 2018-04-26 Renlin Li  <renlin.li@arm.com>

> 

>     * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new value.

>     * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>     * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.


Approved - please apply.

Cheers
  Nick
Renlin Li April 27, 2018, 9:46 p.m. | #3
Hi Peter,

Thanks for the comment!
(I noticed that when I am working on the fix. But I forgot why I didn't do it there)

I update the patch by using PG_OFFSET when resolving the relocation.
Regression test Okay.

Regards,
Renlin

bfd/ChangeLog:

2018-04-27  Renlin Li  <renlin.li@arm.com>

	* elfxx-aarch64.c (_bfd_aarch64_elf_resolve_relocation): Use PG_OFFSET
	to resolve BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC,
	BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC,
	BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC,
	BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC,
	BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC,
	BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC,
	BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC,
	BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC.

ld/ChangeLog:

2018-04-27 Renlin Li  <renlin.li@arm.com>

      * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new value.
      * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.
      * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.

On 04/26/2018 03:53 PM, Peter Smith wrote:
> Hello Renlin,

> 

> I've checked that the patch works on an example I had that previously

> gave different results to Gold and an in-progress implementation in

> LLD. To the best of my knowledge the bitsizes in the HOWTO and the

> method to truncate the result will work. One thing that I'm a bit

> curious about is that relocations such as

> BFD_RELOC_AARCH64_LDST64_LO12 use "value = PG_OFFSET (value +

> addend);" to calculate the addend, whereas relocations like

> BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC use "value = value +

> addend". In the former case the truncation is done in

> _bfd_aarch64_elf_resolve_relocation whereas in the latter you are

> having to truncate in _bfd_aarch64_elf_put_addend; it seems like you

> might be able to use PG_OFFSET for all the LO12 relocations and not

> need to put an extra truncation step in? I'm no expert in the BFD

> codebase, so there may be good reasons not to do that so just treat

> that as an observation.

> 

> Thanks for the fix.

> 

> Peter

> 

> On 26 April 2018 at 14:05, Renlin Li <renlin.li@foss.arm.com> wrote:

>> Hi all,

>>

>>

>> In aarch64, there are relocations for the unsigned immediate offset of

>> load/store instruction.

>> The size of the immediate field is 12-bit. In the scaled case, not all

>> 12-bit will be used.

>>

>> For example: R_AARCH64_LD64_GOT_LO12_NC

>> Set the LD/ST immediate field to bits [11:3] of relocated value. No overflow

>> check.

>>

>> In this case, the top 3 bits of the immediate field of the load instruction

>> should

>> be all 0. Only 9 bits from the value should be written into the instruction

>> encoding.

>>

>> For relocations with over-flow check, if the value is larger than 12-bits,

>> it

>> will be caught by the checking mechanism.

>>

>> But for relocations without no overflow check, the value could be larger

>> than

>> 12 bits, after scaling, it might take more bits than the specification

>> specified. The value should be properly truncated.

>>

>> The patch here correct the bitsize field of a few relocations, and the

>> bitsize

>> field is used to properly truncate the value written into the instruction

>> encoding.

>>

>> Okay to commit?

>>

>> bfd/ChangeLog:

>>

>> 2018-04-26  Renlin Li  <renlin.li@arm.com>

>>

>>          * elfnn-aarch64.c (elfNN_aarch64_howto_table): Correct the bitsize

>>          field of R_AARCH64_LD64_GOT_LO12_NC, R_AARCH64_P32_LD32_GOT_LO12_NC,

>>          R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC,

>>          R_AARCH64_P32_LD32_GOTTPREL_LO12_NC,

>>          R_AARCH64_LDST16_ABS_LO12_NC,

>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>>          R_AARCH64_TLSDESC_LD64_LO12_NC.

>>          R_AARCH64_LDST32_ABS_LO12_NC,

>>          R_AARCH64_LDST64_ABS_LO12_NC,

>>          R_AARCH64_LDST128_ABS_LO12_NC,

>>          * elfxx-aarch64.c (_bfd_aarch64_elf_put_addend): Truncate the

>>          immediate value for load/store instruction.

>>

>> ld/ChangeLog:

>>

>> 2018-04-26 Renlin Li  <renlin.li@arm.com>

>>

>>          * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new

>> value.

>>          * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>>          * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.
commit a7b3fde0b637089dadeb37b0c5f7de27162f67e0
Author: Renlin Li <renlin.li@arm.com>
Date:   Fri Apr 27 10:48:18 2018 +0100

    [BFD][AARCH64]Properly truncate no overflow checking relocation value for load/store immediate.
    
    bfd/ChangeLog:
    
    2018-04-27  Renlin Li  <renlin.li@arm.com>
    
    	* elfxx-aarch64.c (_bfd_aarch64_elf_resolve_relocation): Use PG_OFFSET
    	to resolve BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC,
    	BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC,
    	BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC,
    	BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC,
    	BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC,
    	BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC,
    	BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC,
    	BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC.
    
    ld/ChangeLog:
    
    2018-04-27 Renlin Li  <renlin.li@arm.com>
    
        * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new value.
        * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.
        * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.

diff --git a/bfd/elfxx-aarch64.c b/bfd/elfxx-aarch64.c
index 08e7cd9..59fdc5b 100644
--- a/bfd/elfxx-aarch64.c
+++ b/bfd/elfxx-aarch64.c
@@ -452,26 +452,18 @@ _bfd_aarch64_elf_resolve_relocation (bfd_reloc_code_real_type r_type,
     case BFD_RELOC_AARCH64_TLSLD_ADD_DTPREL_LO12:
     case BFD_RELOC_AARCH64_TLSLD_ADD_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_MOVW_DTPREL_G0:
     case BFD_RELOC_AARCH64_TLSLD_MOVW_DTPREL_G0_NC:
     case BFD_RELOC_AARCH64_TLSLD_MOVW_DTPREL_G1:
     case BFD_RELOC_AARCH64_TLSLD_MOVW_DTPREL_G1_NC:
     case BFD_RELOC_AARCH64_TLSLD_MOVW_DTPREL_G2:
     case BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC:
       value = value + addend;
       break;
 
@@ -521,7 +513,15 @@ _bfd_aarch64_elf_resolve_relocation (bfd_reloc_code_real_type r_type,
     case BFD_RELOC_AARCH64_TLSGD_ADD_LO12_NC:
     case BFD_RELOC_AARCH64_TLSIE_LD32_GOTTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC:
       value = PG_OFFSET (value + addend);
       break;
 
diff --git a/ld/testsuite/ld-aarch64/emit-relocs-115.d b/ld/testsuite/ld-aarch64/emit-relocs-115.d
index f436d32..95a6e31 100644
--- a/ld/testsuite/ld-aarch64/emit-relocs-115.d
+++ b/ld/testsuite/ld-aarch64/emit-relocs-115.d
@@ -6,5 +6,5 @@
 00010000 <.text>:
    10000:	798019d6 	ldrsh	x22, \[x14, #12\]
 			10000: R_AARCH64_P32_TLSLE_LDST16_TPREL_LO12_NC	v2
-   10004:	79a72a28 	ldrsh	x8, \[x17, #5012\]
+   10004:	79872a28 	ldrsh	x8, \[x17, #916\]
 			10004: R_AARCH64_P32_TLSLE_LDST16_TPREL_LO12_NC	v3
diff --git a/ld/testsuite/ld-aarch64/emit-relocs-534.d b/ld/testsuite/ld-aarch64/emit-relocs-534.d
index 121fdc4..fe59b23 100644
--- a/ld/testsuite/ld-aarch64/emit-relocs-534.d
+++ b/ld/testsuite/ld-aarch64/emit-relocs-534.d
@@ -5,5 +5,5 @@
 0000000000010000 <.text>:
    10000:	798009d6 	ldrsh	x22, \[x14, #4\]
 			10000: R_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC	v2
-   10004:	79a71a28 	ldrsh	x8, \[x17, #5004\]
+   10004:	79871a28 	ldrsh	x8, \[x17, #908\]
 			10004: R_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC	v3
diff --git a/ld/testsuite/ld-aarch64/emit-relocs-555.d b/ld/testsuite/ld-aarch64/emit-relocs-555.d
index e866b60..1e33998 100644
--- a/ld/testsuite/ld-aarch64/emit-relocs-555.d
+++ b/ld/testsuite/ld-aarch64/emit-relocs-555.d
@@ -5,5 +5,5 @@
 0000000000010000 <.text>:
    10000:	798029d6 	ldrsh	x22, \[x14, #20\]
 			10000: R_AARCH64_TLSLE_LDST16_TPREL_LO12_NC	v2
-   10004:	79a73a28 	ldrsh	x8, \[x17, #5020\]
+   10004:	79873a28 	ldrsh	x8, \[x17, #924\]
 			10004: R_AARCH64_TLSLE_LDST16_TPREL_LO12_NC	v3
Renlin Li June 20, 2018, 8:48 a.m. | #4
Ping~



On 04/27/2018 10:46 PM, Renlin Li wrote:
> Hi Peter,

> 

> Thanks for the comment!

> (I noticed that when I am working on the fix. But I forgot why I didn't do it there)

> 

> I update the patch by using PG_OFFSET when resolving the relocation.

> Regression test Okay.

> 

> Regards,

> Renlin

> 

> bfd/ChangeLog:

> 

> 2018-04-27  Renlin Li  <renlin.li@arm.com>

> 

>      * elfxx-aarch64.c (_bfd_aarch64_elf_resolve_relocation): Use PG_OFFSET

>      to resolve BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC,

>      BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC,

>      BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC,

>      BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC,

>      BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC,

>      BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC,

>      BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC,

>      BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC.

> 

> ld/ChangeLog:

> 

> 2018-04-27 Renlin Li  <renlin.li@arm.com>

> 

>       * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new value.

>       * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>       * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.

> 

> On 04/26/2018 03:53 PM, Peter Smith wrote:

>> Hello Renlin,

>>

>> I've checked that the patch works on an example I had that previously

>> gave different results to Gold and an in-progress implementation in

>> LLD. To the best of my knowledge the bitsizes in the HOWTO and the

>> method to truncate the result will work. One thing that I'm a bit

>> curious about is that relocations such as

>> BFD_RELOC_AARCH64_LDST64_LO12 use "value = PG_OFFSET (value +

>> addend);" to calculate the addend, whereas relocations like

>> BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC use "value = value +

>> addend". In the former case the truncation is done in

>> _bfd_aarch64_elf_resolve_relocation whereas in the latter you are

>> having to truncate in _bfd_aarch64_elf_put_addend; it seems like you

>> might be able to use PG_OFFSET for all the LO12 relocations and not

>> need to put an extra truncation step in? I'm no expert in the BFD

>> codebase, so there may be good reasons not to do that so just treat

>> that as an observation.

>>

>> Thanks for the fix.

>>

>> Peter

>>

>> On 26 April 2018 at 14:05, Renlin Li <renlin.li@foss.arm.com> wrote:

>>> Hi all,

>>>

>>>

>>> In aarch64, there are relocations for the unsigned immediate offset of

>>> load/store instruction.

>>> The size of the immediate field is 12-bit. In the scaled case, not all

>>> 12-bit will be used.

>>>

>>> For example: R_AARCH64_LD64_GOT_LO12_NC

>>> Set the LD/ST immediate field to bits [11:3] of relocated value. No overflow

>>> check.

>>>

>>> In this case, the top 3 bits of the immediate field of the load instruction

>>> should

>>> be all 0. Only 9 bits from the value should be written into the instruction

>>> encoding.

>>>

>>> For relocations with over-flow check, if the value is larger than 12-bits,

>>> it

>>> will be caught by the checking mechanism.

>>>

>>> But for relocations without no overflow check, the value could be larger

>>> than

>>> 12 bits, after scaling, it might take more bits than the specification

>>> specified. The value should be properly truncated.

>>>

>>> The patch here correct the bitsize field of a few relocations, and the

>>> bitsize

>>> field is used to properly truncate the value written into the instruction

>>> encoding.

>>>

>>> Okay to commit?

>>>

>>> bfd/ChangeLog:

>>>

>>> 2018-04-26  Renlin Li  <renlin.li@arm.com>

>>>

>>>          * elfnn-aarch64.c (elfNN_aarch64_howto_table): Correct the bitsize

>>>          field of R_AARCH64_LD64_GOT_LO12_NC, R_AARCH64_P32_LD32_GOT_LO12_NC,

>>>          R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC,

>>>          R_AARCH64_P32_LD32_GOTTPREL_LO12_NC,

>>>          R_AARCH64_LDST16_ABS_LO12_NC,

>>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>>>          R_AARCH64_TLSDESC_LD64_LO12_NC.

>>>          R_AARCH64_LDST32_ABS_LO12_NC,

>>>          R_AARCH64_LDST64_ABS_LO12_NC,

>>>          R_AARCH64_LDST128_ABS_LO12_NC,

>>>          * elfxx-aarch64.c (_bfd_aarch64_elf_put_addend): Truncate the

>>>          immediate value for load/store instruction.

>>>

>>> ld/ChangeLog:

>>>

>>> 2018-04-26 Renlin Li  <renlin.li@arm.com>

>>>

>>>          * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new

>>> value.

>>>          * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>>>          * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.

>
Peter Smith June 20, 2018, 9:20 a.m. | #5
Hello Renlin,

No objections. Apologies if you were waiting for me I'd assumed that
you were able to update based on your prior approval.

Peter

On 20 June 2018 at 09:48, Renlin Li <renlin.li@foss.arm.com> wrote:
> Ping~

>

>

>

>

> On 04/27/2018 10:46 PM, Renlin Li wrote:

>>

>> Hi Peter,

>>

>> Thanks for the comment!

>> (I noticed that when I am working on the fix. But I forgot why I didn't do

>> it there)

>>

>> I update the patch by using PG_OFFSET when resolving the relocation.

>> Regression test Okay.

>>

>> Regards,

>> Renlin

>>

>> bfd/ChangeLog:

>>

>> 2018-04-27  Renlin Li  <renlin.li@arm.com>

>>

>>      * elfxx-aarch64.c (_bfd_aarch64_elf_resolve_relocation): Use

>> PG_OFFSET

>>      to resolve BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC.

>>

>> ld/ChangeLog:

>>

>> 2018-04-27 Renlin Li  <renlin.li@arm.com>

>>

>>       * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new

>> value.

>>       * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>>       * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.

>>

>> On 04/26/2018 03:53 PM, Peter Smith wrote:

>>>

>>> Hello Renlin,

>>>

>>> I've checked that the patch works on an example I had that previously

>>> gave different results to Gold and an in-progress implementation in

>>> LLD. To the best of my knowledge the bitsizes in the HOWTO and the

>>> method to truncate the result will work. One thing that I'm a bit

>>> curious about is that relocations such as

>>> BFD_RELOC_AARCH64_LDST64_LO12 use "value = PG_OFFSET (value +

>>> addend);" to calculate the addend, whereas relocations like

>>> BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC use "value = value +

>>> addend". In the former case the truncation is done in

>>> _bfd_aarch64_elf_resolve_relocation whereas in the latter you are

>>> having to truncate in _bfd_aarch64_elf_put_addend; it seems like you

>>> might be able to use PG_OFFSET for all the LO12 relocations and not

>>> need to put an extra truncation step in? I'm no expert in the BFD

>>> codebase, so there may be good reasons not to do that so just treat

>>> that as an observation.

>>>

>>> Thanks for the fix.

>>>

>>> Peter

>>>

>>> On 26 April 2018 at 14:05, Renlin Li <renlin.li@foss.arm.com> wrote:

>>>>

>>>> Hi all,

>>>>

>>>>

>>>> In aarch64, there are relocations for the unsigned immediate offset of

>>>> load/store instruction.

>>>> The size of the immediate field is 12-bit. In the scaled case, not all

>>>> 12-bit will be used.

>>>>

>>>> For example: R_AARCH64_LD64_GOT_LO12_NC

>>>> Set the LD/ST immediate field to bits [11:3] of relocated value. No

>>>> overflow

>>>> check.

>>>>

>>>> In this case, the top 3 bits of the immediate field of the load

>>>> instruction

>>>> should

>>>> be all 0. Only 9 bits from the value should be written into the

>>>> instruction

>>>> encoding.

>>>>

>>>> For relocations with over-flow check, if the value is larger than

>>>> 12-bits,

>>>> it

>>>> will be caught by the checking mechanism.

>>>>

>>>> But for relocations without no overflow check, the value could be larger

>>>> than

>>>> 12 bits, after scaling, it might take more bits than the specification

>>>> specified. The value should be properly truncated.

>>>>

>>>> The patch here correct the bitsize field of a few relocations, and the

>>>> bitsize

>>>> field is used to properly truncate the value written into the

>>>> instruction

>>>> encoding.

>>>>

>>>> Okay to commit?

>>>>

>>>> bfd/ChangeLog:

>>>>

>>>> 2018-04-26  Renlin Li  <renlin.li@arm.com>

>>>>

>>>>          * elfnn-aarch64.c (elfNN_aarch64_howto_table): Correct the

>>>> bitsize

>>>>          field of R_AARCH64_LD64_GOT_LO12_NC,

>>>> R_AARCH64_P32_LD32_GOT_LO12_NC,

>>>>          R_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC,

>>>>          R_AARCH64_P32_LD32_GOTTPREL_LO12_NC,

>>>>          R_AARCH64_LDST16_ABS_LO12_NC,

>>>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>>>>          R_AARCH64_P32_TLSDESC_LD32_LO12_NC.

>>>>          R_AARCH64_TLSDESC_LD64_LO12_NC.

>>>>          R_AARCH64_LDST32_ABS_LO12_NC,

>>>>          R_AARCH64_LDST64_ABS_LO12_NC,

>>>>          R_AARCH64_LDST128_ABS_LO12_NC,

>>>>          * elfxx-aarch64.c (_bfd_aarch64_elf_put_addend): Truncate the

>>>>          immediate value for load/store instruction.

>>>>

>>>> ld/ChangeLog:

>>>>

>>>> 2018-04-26 Renlin Li  <renlin.li@arm.com>

>>>>

>>>>          * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new

>>>> value.

>>>>          * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>>>>          * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.

>>

>>

>
Nick Clifton June 20, 2018, 10:41 a.m. | #6
Hi Renlin,

>> bfd/ChangeLog:

>>

>> 2018-04-27  Renlin Li  <renlin.li@arm.com>

>>

>>      * elfxx-aarch64.c (_bfd_aarch64_elf_resolve_relocation): Use PG_OFFSET

>>      to resolve BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC,

>>      BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC.

>>

>> ld/ChangeLog:

>>

>> 2018-04-27 Renlin Li  <renlin.li@arm.com>

>>

>>       * testsuite/ld-aarch64/emit-relocs-115.d: Update test with new value.

>>       * testsuite/ld-aarch64/emit-relocs-534.d: Likewise.

>>       * testsuite/ld-aarch64/emit-relocs-555.d: Likewise.


Approved - please apply.

Cheers
  Nick

Patch

diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index 3ccca779ff79cb83d3f2fc7de50bb258a55f94bb..d71aae59e1bb6ca0cc6c1e31d4f77d3e730b30a1 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -892,7 +892,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO (AARCH64_R (LDST16_ABS_LO12_NC),	/* type */
 	 1,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 11,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -907,7 +907,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO (AARCH64_R (LDST32_ABS_LO12_NC),	/* type */
 	 2,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 10,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -922,7 +922,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO (AARCH64_R (LDST64_ABS_LO12_NC),	/* type */
 	 3,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 9,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -937,7 +937,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO (AARCH64_R (LDST128_ABS_LO12_NC),	/* type */
 	 4,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 8,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -984,7 +984,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO64 (AARCH64_R (LD64_GOT_LO12_NC),	/* type */
 	 3,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 9,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -999,7 +999,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO32 (AARCH64_R (LD32_GOT_LO12_NC),	/* type */
 	 2,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 10,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -1179,7 +1179,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO64 (AARCH64_R (TLSIE_LD64_GOTTPREL_LO12_NC),	/* type */
 	 3,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 9,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -1193,7 +1193,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO32 (AARCH64_R (TLSIE_LD32_GOTTPREL_LO12_NC),	/* type */
 	 2,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 10,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -1811,7 +1811,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO64 (AARCH64_R (TLSDESC_LD64_LO12),	/* type */
 	 3,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 9,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
@@ -1826,7 +1826,7 @@  static reloc_howto_type elfNN_aarch64_howto_table[] =
   HOWTO32 (AARCH64_R (TLSDESC_LD32_LO12_NC),	/* type */
 	 2,			/* rightshift */
 	 2,			/* size (0 = byte, 1 = short, 2 = long) */
-	 12,			/* bitsize */
+	 10,			/* bitsize */
 	 FALSE,			/* pc_relative */
 	 0,			/* bitpos */
 	 complain_overflow_dont,	/* complain_on_overflow */
diff --git a/bfd/elfxx-aarch64.c b/bfd/elfxx-aarch64.c
index 08e7cd9684cf4621ac6c2843d8bc82a3d21b4a6a..7da1fdaf70870bc9b9d744ba5fbf1a15ea892dd0 100644
--- a/bfd/elfxx-aarch64.c
+++ b/bfd/elfxx-aarch64.c
@@ -263,36 +263,40 @@  _bfd_aarch64_elf_put_addend (bfd *abfd,
       contents = reencode_add_imm (contents, addend);
       break;
 
-    case BFD_RELOC_AARCH64_LD32_GOTPAGE_LO14:
     case BFD_RELOC_AARCH64_LD32_GOT_LO12_NC:
-    case BFD_RELOC_AARCH64_LD64_GOTOFF_LO15:
-    case BFD_RELOC_AARCH64_LD64_GOTPAGE_LO15:
     case BFD_RELOC_AARCH64_LD64_GOT_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSDESC_LD32_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSIE_LD32_GOTTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC:
+    case BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC:
+      /* Properly truncate the value according to the bitsize.  */
+      addend &= ((1 << howto->bitsize) - 1);
+      /* Fall through.  */
+
+    case BFD_RELOC_AARCH64_LD32_GOTPAGE_LO14:
+    case BFD_RELOC_AARCH64_LD64_GOTPAGE_LO15:
+    case BFD_RELOC_AARCH64_LD64_GOTOFF_LO15:
     case BFD_RELOC_AARCH64_LDST128_LO12:
     case BFD_RELOC_AARCH64_LDST16_LO12:
     case BFD_RELOC_AARCH64_LDST32_LO12:
     case BFD_RELOC_AARCH64_LDST64_LO12:
     case BFD_RELOC_AARCH64_LDST8_LO12:
-    case BFD_RELOC_AARCH64_TLSDESC_LD32_LO12_NC:
     case BFD_RELOC_AARCH64_TLSDESC_LD64_LO12:
-    case BFD_RELOC_AARCH64_TLSIE_LD32_GOTTPREL_LO12_NC:
-    case BFD_RELOC_AARCH64_TLSIE_LD64_GOTTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST32_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST64_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLD_LDST8_DTPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST16_TPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST32_TPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST64_TPREL_LO12_NC:
     case BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12:
-    case BFD_RELOC_AARCH64_TLSLE_LDST8_TPREL_LO12_NC:
       if (old_addend & ((1 << howto->rightshift) - 1))
 	return bfd_reloc_overflow;
       /* Used for ldr*|str* rt, [rn, #uimm12] to provide the low order
diff --git a/ld/testsuite/ld-aarch64/emit-relocs-115.d b/ld/testsuite/ld-aarch64/emit-relocs-115.d
index f436d32c61fca5abf7b1b59fa822fbc086dc9722..95a6e317448513a8afa0739adf3533de6fe2f90f 100644
--- a/ld/testsuite/ld-aarch64/emit-relocs-115.d
+++ b/ld/testsuite/ld-aarch64/emit-relocs-115.d
@@ -6,5 +6,5 @@ 
 00010000 <.text>:
    10000:	798019d6 	ldrsh	x22, \[x14, #12\]
 			10000: R_AARCH64_P32_TLSLE_LDST16_TPREL_LO12_NC	v2
-   10004:	79a72a28 	ldrsh	x8, \[x17, #5012\]
+   10004:	79872a28 	ldrsh	x8, \[x17, #916\]
 			10004: R_AARCH64_P32_TLSLE_LDST16_TPREL_LO12_NC	v3
diff --git a/ld/testsuite/ld-aarch64/emit-relocs-534.d b/ld/testsuite/ld-aarch64/emit-relocs-534.d
index 121fdc4c0b2330475fff04ebd32dc66ba293382d..fe59b23182d1adff2786a6e7559a38c1c1230940 100644
--- a/ld/testsuite/ld-aarch64/emit-relocs-534.d
+++ b/ld/testsuite/ld-aarch64/emit-relocs-534.d
@@ -5,5 +5,5 @@ 
 0000000000010000 <.text>:
    10000:	798009d6 	ldrsh	x22, \[x14, #4\]
 			10000: R_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC	v2
-   10004:	79a71a28 	ldrsh	x8, \[x17, #5004\]
+   10004:	79871a28 	ldrsh	x8, \[x17, #908\]
 			10004: R_AARCH64_TLSLD_LDST16_DTPREL_LO12_NC	v3
diff --git a/ld/testsuite/ld-aarch64/emit-relocs-555.d b/ld/testsuite/ld-aarch64/emit-relocs-555.d
index e866b6034c4405edaf4459f2b0aa22697458075a..1e33998e3579b674e707c659facc3e5b3f988f77 100644
--- a/ld/testsuite/ld-aarch64/emit-relocs-555.d
+++ b/ld/testsuite/ld-aarch64/emit-relocs-555.d
@@ -5,5 +5,5 @@ 
 0000000000010000 <.text>:
    10000:	798029d6 	ldrsh	x22, \[x14, #20\]
 			10000: R_AARCH64_TLSLE_LDST16_TPREL_LO12_NC	v2
-   10004:	79a73a28 	ldrsh	x8, \[x17, #5020\]
+   10004:	79873a28 	ldrsh	x8, \[x17, #924\]
 			10004: R_AARCH64_TLSLE_LDST16_TPREL_LO12_NC	v3