[v3,0/5] LoongArch tls le model linker relaxation support.

Message ID 20231215100633.65397-1-changjiachen@stu.xupt.edu.cn
Headers
Series LoongArch tls le model linker relaxation support. |

Message

changjiachen Dec. 15, 2023, 10:06 a.m. UTC
  This is the v3 version of patches to support loongarch linker tls le model relax.

Changes from v2:

* Some problems in the v2 patch are answered as follows.

Question: 

use ".reloc.,R_LARCH_TLS_LE_ADD_R,sym" to generate relocation 
or %le_add_r(sym) to generate relocation.

Reply: 

First, after a test, the R_LARCH_TLS_LE_ADD_R can be generated using ".reloc.,R_LARCH_TLS_LE_ADD_R,sym", 
or "%le_add_r(sym)". However,".reloc" generates R_LARCH_TLS_LE_ADD_R relocation directly, and it is not 
easy to add "R_LARCH_RELAX" relocation. "%le_add_r(sym)" Adds the R_LARCH_TLS_LE_ADD_R and R_LARCH_RELAX 
relocation commands, which is easier to implement.

Of course, there is another way to generate ".reloc.,R_LARCH_TLS_LE_ADD_R,sym" and 
".reloc.,R_LARCH_RELAX,sym" directly in gcc. However, this implementation causes the 
-mrelax/-mno-relax option to be set in both gcc and gas, which can be confusing.

One problem with this is that add.d $r12,$r12,$r2 and add.d $r12,$r12,$r2, 
%le_add_r(sym) are too similar, so I have to add comments in loongarch_fix_opcodes[]. 
The goal is to make it as clear as possible to developers.

* modified code format in loongarch_relax_tls_le(),use loongarch_relax_delete_bytes() 
instead of R_LARCH_DELETE to implement the delete instruction operation.

* modified R_LARCH_TLS_LE_ADD_R type_name:"tls_le_add_r"-->"le_add_r".

* modify comment information.

* some comments added to "add.d" in loongarch_opcode loongarch_fix_opcodes[].

* remove some unnecessary content from the ld/testsuite/ld-loongarch test case.




Changes from v1:

* Modified v1-0000-cover-letter.patch part of the explanatory content.

Before Modify:

example: __thread int a = 1;

old insn sequence:

lu12i.w $r12,%le_hi20_r(a)
ori     $r12,$r12,%le_lo12_r(a)
add.d   $r12,$r12,$r2,%le_add_r(a)
li.w  	$r13,$r0,1
stptr.w $r13,$r12,0

new insn sequence:

lu12i.w $r12,%le_hi20_r(a)
add.d   $r12,$r12,$r2,%le_add_r(a)
li.w  	$r13,$r0,1
st.w    $r13,$r12,%le_lo12_r(a)

After Modify:

example: __thread int a = 1;

old insn sequence(at the O0 optimization level):

lu12i.w $r12,%le_hi20(a)
ori     $r12,$r12,%le_lo12(a)
add.d   $r12,$r12,$r2
addi.w  $r13,$r0,1
stptr.w $r13,$r12,0

new insn sequence(at the O0 optimization level):

lu12i.w $r12,%le_hi20_r(a)
add.d   $r12,$r12,$r2,%le_add_r(a)
addi.w  $r13,$r0,1
st.w    $r13,$r12,%le_lo12_r(a)


changjiachen (5):
  LoongArch: bfd: Add support for tls le relax.
  LoongArch: include: Add support for tls le relax.
  LoongArch: opcodes: Add support for tls le relax.
  LoongArch: gas: Add support for tls le relax.
  LoongArch: ld: Add support for tls le relax.

 bfd/bfd-in2.h                                 |   4 +
 bfd/elfnn-loongarch.c                         |  76 +++++++++
 bfd/elfxx-loongarch.c                         |  50 ++++++
 bfd/libbfd.h                                  |   3 +
 bfd/reloc.c                                   |   6 +
 gas/config/tc-loongarch.c                     |  12 +-
 gas/testsuite/gas/loongarch/reloc.d           |  18 +++
 gas/testsuite/gas/loongarch/reloc.s           |  11 ++
 include/elf/loongarch.h                       |  12 ++
 ld/testsuite/ld-loongarch-elf/old-tls-le.s    |  19 +++
 .../relax-bound-check-tls-le.s                |  48 ++++++
 ld/testsuite/ld-loongarch-elf/relax-tls-le.s  |  17 ++
 ld/testsuite/ld-loongarch-elf/relax.exp       | 151 +++++++++++++++++-
 .../tls-relax-compatible-check-new.s          |  29 ++++
 .../tls-relax-compatible-check-old.s          |  27 ++++
 opcodes/loongarch-opc.c                       |   3 +-
 16 files changed, 482 insertions(+), 4 deletions(-)
 create mode 100644 ld/testsuite/ld-loongarch-elf/old-tls-le.s
 create mode 100644 ld/testsuite/ld-loongarch-elf/relax-bound-check-tls-le.s
 create mode 100644 ld/testsuite/ld-loongarch-elf/relax-tls-le.s
 create mode 100644 ld/testsuite/ld-loongarch-elf/tls-relax-compatible-check-new.s
 create mode 100644 ld/testsuite/ld-loongarch-elf/tls-relax-compatible-check-old.s
  

Comments

Xi Ruoyao Dec. 15, 2023, 12:34 p.m. UTC | #1
On Fri, 2023-12-15 at 18:06 +0800, changjiachen wrote:
> First, after a test, the R_LARCH_TLS_LE_ADD_R can be generated using ".reloc.,R_LARCH_TLS_LE_ADD_R,sym", 
> or "%le_add_r(sym)". However,".reloc" generates R_LARCH_TLS_LE_ADD_R relocation directly, and it is not 
> easy to add "R_LARCH_RELAX" relocation. "%le_add_r(sym)" Adds the R_LARCH_TLS_LE_ADD_R and R_LARCH_RELAX 
> relocation commands, which is easier to implement.
>
> Of course, there is another way to generate ".reloc.,R_LARCH_TLS_LE_ADD_R,sym" and 
> ".reloc.,R_LARCH_RELAX,sym" directly in gcc. However, this implementation causes the 
> -mrelax/-mno-relax option to be set in both gcc and gas, which can be confusing.

GCC 14 already have -mrelax/-mno-relax options.  This is not confusing
at all.

> One problem with this is that add.d $r12,$r12,$r2 and add.d $r12,$r12,$r2, 
> %le_add_r(sym) are too similar, so I have to add comments in loongarch_fix_opcodes[]. 
> The goal is to make it as clear as possible to developers.

No, normal developers (not Binutils developers) should not be mandated
to read Binutils code to understand something.  OTOH they should read
the documentation of GAS.  So make it clear somewhere in the doc.

And I've told you if you must go with an additional operand in add.d,
you should reject things like:

add.d $a0,$a0,$a0,8

but now:

$ cat t.s
add.d $a0,$a0,$a0,8
$ gas/as-new t.s
$ binutils/objdump -d a.out

a.out:     file format elf64-loongarch


Disassembly of section .text:

0000000000000000 <.text>:
   0:	0010b084 	add.d       	$a0, $a0, $t0

Now is it clear that this behavior is unacceptable?

Is it too difficult or unreasonable to make it an error with message
"the fourth operand of the add.d instruction must be %le_add_r"?!  Or
didn't I make it clear?
  
changjiachen Dec. 19, 2023, 5:33 a.m. UTC | #2
From: Xi Ruoyao <xry111@xry111.site>
Date: 2023-12-15 20:34:50
To:  changjiachen <changjiachen@stu.xupt.edu.cn>,binutils@sourceware.org
Cc:  xuchenghua@loongson.cn,chenglulu@loongson.cn,liuzhensong@loongson.cn,i.swmail@xen0n.name,maskray@google.com,cailulu@loongson.cn,luweining@loongson.cn,wanglei@loongson.cn,hejinyang@loongson.cn,Lazy_Linux@126.com,mengqinggang@loongson.cn
Subject: Re: [PATCH v3 0/5] LoongArch tls le model linker relaxation support.>On Fri, 2023-12-15 at 18:06 +0800, changjiachen wrote:
>> First, after a test, the R_LARCH_TLS_LE_ADD_R can be generated using ".reloc.,R_LARCH_TLS_LE_ADD_R,sym", 
>> or "%le_add_r(sym)". However,".reloc" generates R_LARCH_TLS_LE_ADD_R relocation directly, and it is not 
>> easy to add "R_LARCH_RELAX" relocation. "%le_add_r(sym)" Adds the R_LARCH_TLS_LE_ADD_R and R_LARCH_RELAX 
>> relocation commands, which is easier to implement.
>>
>> Of course, there is another way to generate ".reloc.,R_LARCH_TLS_LE_ADD_R,sym" and 
>> ".reloc.,R_LARCH_RELAX,sym" directly in gcc. However, this implementation causes the 
>> -mrelax/-mno-relax option to be set in both gcc and gas, which can be confusing.
>
>GCC 14 already have -mrelax/-mno-relax options.  This is not confusing
>at all.
>
>> One problem with this is that add.d $r12,$r12,$r2 and add.d $r12,$r12,$r2, 
>> %le_add_r(sym) are too similar, so I have to add comments in loongarch_fix_opcodes[]. 
>> The goal is to make it as clear as possible to developers.
>
>No, normal developers (not Binutils developers) should not be mandated
>to read Binutils code to understand something.  OTOH they should read
>the documentation of GAS.  So make it clear somewhere in the doc.
>
>And I've told you if you must go with an additional operand in add.d,
>you should reject things like:
>
>add.d $a0,$a0,$a0,8
>
>but now:
>
>$ cat t.s
>add.d $a0,$a0,$a0,8
>$ gas/as-new t.s
>$ binutils/objdump -d a.out
>
>a.out:     file format elf64-loongarch
>
>
>Disassembly of section .text:
>
>0000000000000000 <.text>:
>   0:	0010b084 	add.d       	$a0, $a0, $t0
>
>Now is it clear that this behavior is unacceptable?
>
>Is it too difficult or unreasonable to make it an error with message
>"the fourth operand of the add.d instruction must be %le_add_r"?!  Or
>didn't I make it clear?
Hello, I understand what you mean. For the fourth operand of tls add.d instruction, 
I have made the following modifications.


For the case of "add.d $a0,$a0,$a0,8", that is, when the fourth operand of the tls add.d 
instruction is not %le_add_r(sym), the modified assembler throws a "no match insn add.d $a0,$a0,$a0,8".


example:
a.s
add.d $a0,$a0,$a0,8
$ gas/as-new a.s
a.s: Assembler messages:
a.s:1: error:no match insn: add.d	$a0,$a0,$a0,8



Can such modification solve the problem you raised? 
I will add this change to v4 patch if possible.
I am very much looking forward to your suggestions again.








>
>-- 
>Xi Ruoyao <xry111@xry111.site>
>School of Aerospace Science and Technology, Xidian University
  
Xi Ruoyao Dec. 19, 2023, 10:21 a.m. UTC | #3
On Tue, 2023-12-19 at 13:33 +0800, 常佳琛 wrote:
> For the case of "add.d $a0,$a0,$a0,8", that is, when the fourth operand of the tls add.d 
> instruction is not %le_add_r(sym), the modified assembler throws a "no match insn add.d $a0,$a0,$a0,8".
> 
> example:
> a.s
> add.d $a0,$a0,$a0,8
> $ gas/as-new a.s
> a.s: Assembler messages: a.s:1: error:no match insn: add.d $a0,$a0,$a0,8

Ok to me.  Thanks.