[PATCH] RISC-V: Avoid relocation for a function symbol self-referenced with -mno-relax
Xiao Zeng
zengxiao@eswincomputing.com
Tue Jan 7 03:03:52 GMT 2025
2025-01-07 10:46 Nelson Chu <nelson@rivosinc.com> wrote:
>
>On Tue, Jan 7, 2025 at 9:54 AM Xiao Zeng <zengxiao@eswincomputing.com>
>wrote:
>
>> Refer to commit dff565fcca8137954d6ad571ef39f6aec5c0429c. When relaxation
>> is disabled, the assembler no longer needs to generate relocation
>> information for references to this symbol within its body. This commit
>> enhances such scenarios.
>>
>> gas/ChangeLog:
>>
>> * config/tc-riscv.c (md_apply_fix): Avoid relocation for a function
>> symbol self-referenced with -mno-relax.
>> * testsuite/gas/riscv/fixup-local-norelax.d: Updated.
>> * testsuite/gas/riscv/fixup-function-symbol-norelax.d: New test.
>> * testsuite/gas/riscv/fixup-function-symbol-relax.d: New test.
>> * testsuite/gas/riscv/fixup-function-symbol.s: New test.
>>
>> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com>
>> ---
>> gas/config/tc-riscv.c | 4 +-
>> .../gas/riscv/fixup-function-symbol-norelax.d | 26 ++++++++++
>> .../gas/riscv/fixup-function-symbol-relax.d | 50 +++++++++++++++++++
>> .../gas/riscv/fixup-function-symbol.s | 13 +++++
>> gas/testsuite/gas/riscv/fixup-local-norelax.d | 12 ++---
>> 5 files changed, 94 insertions(+), 11 deletions(-)
>> create mode 100644 gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> create mode 100644 gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> create mode 100644 gas/testsuite/gas/riscv/fixup-function-symbol.s
>>
>> diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
>> index a915c8b4995..7255a2337f6 100644
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -4800,7 +4800,7 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg)
>> Fill in a tentative value to improve objdump readability for
>> -mrelax,
>> and set fx_done for -mno-relax. */
>> if (fixP->fx_addsy
>> - && S_IS_LOCAL (fixP->fx_addsy)
>> + && (S_IS_LOCAL (fixP->fx_addsy) || !riscv_opts.relax)
>> && S_GET_SEGMENT (fixP->fx_addsy) == seg)
>> {
>> bfd_vma target = S_GET_VALUE (fixP->fx_addsy) + *valP;
>> @@ -4835,7 +4835,7 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg)
>> riscv_pcrel_hi_fixup *entry = htab_find (riscv_pcrel_hi_fixup_hash,
>> &search);
>> if (entry && entry->symbol
>> - && S_IS_LOCAL (entry->symbol)
>> + && (S_IS_LOCAL (entry->symbol) || !riscv_opts.relax)
>> && S_GET_SEGMENT (entry->symbol) == seg)
>> {
>> bfd_vma target = entry->target;
>>
>
>The original idea was to only optimize local symbols, since global/weak
>symbols may be preemptive in the link time, so we cannot resolve them in
>the assembler time.
Your point is correct, but considering this special case: When relaxation is
disabled, the assembler no longer needs to generate relocation information
for references to this symbol within its body. Regardless of whether it's a
global symbol, the relocation can be completed during the assembly phase,
without needing to defer it to the linking (ld) phase.
It is optimized specifically for this special scenario.
>
>
>> diff --git a/gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> b/gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> new file mode 100644
>> index 00000000000..5a69b1cc6fd
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-function-symbol-norelax.d
>> @@ -0,0 +1,26 @@
>> +#as: -march=rv64i -mno-relax
>> +#source: fixup-function-symbol.s
>> +#objdump: -dr -Mno-aliases
>> +
>> +.*:[ ]+file format .*
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +0+0000 <localFuncSym>:
>> +[ ]+0:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+4:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 0
>> <localFuncSym>
>> +[ ]+8:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+c:[ ]+ff853503[ ]+ld[ ]+a0,-8\(a0\) # 0 <localFuncSym>
>> +[ ]+10:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+14:[ ]+fea53823[ ]+sd[ ]+a0,-16\(a0\) # 0 <localFuncSym>
>> +[ ]+18:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> +
>> +0+001c <globalFuncSym>:
>> +[ ]+1c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+20:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 1c
>> <globalFuncSym>
>> +[ ]+24:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+28:[ ]+ff853503[ ]+ld[ ]+a0,-8\(a0\) # 1c <globalFuncSym>
>> +[ ]+2c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+30:[ ]+fea53823[ ]+sd[ ]+a0,-16\(a0\) # 1c <globalFuncSym>
>> +[ ]+34:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> \ No newline at end of file
>> diff --git a/gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> b/gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> new file mode 100644
>> index 00000000000..70381534720
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-function-symbol-relax.d
>> @@ -0,0 +1,50 @@
>> +#as: -march=rv64i -mrelax
>> +#source: fixup-function-symbol.s
>> +#objdump: -dr -Mno-aliases
>> +
>> +.*:[ ]+file format .*
>> +
>> +
>> +Disassembly of section .text:
>> +
>> +0+0000 <localFuncSym>:
>> +[ ]+0:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+0:[ ]+R_RISCV_PCREL_HI20[ ]+localFuncSym.*
>> +[ ]+0:[ ]+R_RISCV_RELAX.*
>> +[ ]+4:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 0
>> <localFuncSym>
>> +[ ]+4:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+4:[ ]+R_RISCV_RELAX.*
>> +[ ]+8:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+8:[ ]+R_RISCV_PCREL_HI20[ ]+localFuncSym.*
>> +[ ]+8:[ ]+R_RISCV_RELAX.*
>> +[ ]+c:[ ]+00053503[ ]+ld[ ]+a0,0\(a0\) # 8
>> <localFuncSym\+0x8>
>> +[ ]+c:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+c:[ ]+R_RISCV_RELAX.*
>> +[ ]+10:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+10:[ ]+R_RISCV_PCREL_HI20[ ]+localFuncSym.*
>> +[ ]+10:[ ]+R_RISCV_RELAX.*
>> +[ ]+14:[ ]+00a53023[ ]+sd[ ]+a0,0\(a0\) # 10
>> <localFuncSym\+0x10>
>> +[ ]+14:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> +[ ]+14:[ ]+R_RISCV_RELAX.*
>> +[ ]+18:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> +
>> +0+001c <globalFuncSym>:
>> +[ ]+1c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+1c:[ ]+R_RISCV_PCREL_HI20[ ]+globalFuncSym.*
>> +[ ]+1c:[ ]+R_RISCV_RELAX.*
>> +[ ]+20:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 1c
>> <globalFuncSym>
>> +[ ]+20:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+20:[ ]+R_RISCV_RELAX.*
>> +[ ]+24:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+24:[ ]+R_RISCV_PCREL_HI20[ ]+globalFuncSym.*
>> +[ ]+24:[ ]+R_RISCV_RELAX.*
>> +[ ]+28:[ ]+00053503[ ]+ld[ ]+a0,0\(a0\) # 24
>> <globalFuncSym\+0x8>
>> +[ ]+28:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+28:[ ]+R_RISCV_RELAX.*
>> +[ ]+2c:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> +[ ]+2c:[ ]+R_RISCV_PCREL_HI20[ ]+globalFuncSym.*
>> +[ ]+2c:[ ]+R_RISCV_RELAX.*
>> +[ ]+30:[ ]+00a53023[ ]+sd[ ]+a0,0\(a0\) # 2c
>> <globalFuncSym\+0x10>
>> +[ ]+30:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> +[ ]+30:[ ]+R_RISCV_RELAX.*
>> +[ ]+34:[ ]+00008067[ ]+jalr[ ]+zero,0\(ra\)
>> \ No newline at end of file
>> diff --git a/gas/testsuite/gas/riscv/fixup-function-symbol.s
>> b/gas/testsuite/gas/riscv/fixup-function-symbol.s
>> new file mode 100644
>> index 00000000000..d93b29e17e7
>> --- /dev/null
>> +++ b/gas/testsuite/gas/riscv/fixup-function-symbol.s
>> @@ -0,0 +1,13 @@
>> + .local localFuncSym
>> +localFuncSym:
>> + la a0, localFuncSym
>> + ld a0, localFuncSym
>> + sd a0, localFuncSym, a0
>> + ret
>> +
>> + .global globalFuncSym
>> +globalFuncSym:
>> + la a0, globalFuncSym
>> + ld a0, globalFuncSym
>> + sd a0, globalFuncSym, a0
>> + ret
>>
>
>I don't know the difference between this testcase and fixup-local.s. Even
>so, we should merge them into one as possible as we can if they are trying
>to resolve the same issue...
Yes, there is indeed no significant difference between them. I just used more
informative code to express the intent of this patch, in order to avoid
breaking the previous use cases.
>
>Nelson
>
>
>> diff --git a/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> index 8b29860893c..bc0b6714265 100644
>> --- a/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> +++ b/gas/testsuite/gas/riscv/fixup-local-norelax.d
>> @@ -27,17 +27,11 @@ Disassembly of section .text:
>> [ ]+2c:[ ]+00a51023[ ]+sh[ ]+a0,0\(a0\) # 28 <foo\+0x28>
>> [ ]+2c:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> [ ]+30:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> -[ ]+30:[ ]+R_RISCV_PCREL_HI20[ ]+foo.*
>> -[ ]+34:[ ]+00050513[ ]+addi[ ]+a0,a0,0 # 30 <foo\+0x30>
>> -[ ]+34:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+34:[ ]+fd050513[ ]+addi[ ]+a0,a0,-48 # 0 <foo>
>
> [ ]+38:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> -[ ]+38:[ ]+R_RISCV_PCREL_HI20[ ]+foo.*
>> -[ ]+3c:[ ]+00051503[ ]+lh[ ]+a0,0\(a0\) # 38 <foo\+0x38>
>> -[ ]+3c:[ ]+R_RISCV_PCREL_LO12_I[ ]+.L0.*
>> +[ ]+3c:[ ]+fc851503[ ]+lh[ ]+a0,-56\(a0\) # 0 <foo>
>> [ ]+40:[ ]+00000517[ ]+auipc[ ]+a0,0x0
>> -[ ]+40:[ ]+R_RISCV_PCREL_HI20[ ]+foo.*
>> -[ ]+44:[ ]+00a51023[ ]+sh[ ]+a0,0\(a0\) # 40 <foo\+0x40>
>> -[ ]+44:[ ]+R_RISCV_PCREL_LO12_S[ ]+.L0.*
>> +[ ]+44:[ ]+fca51023[ ]+sh[ ]+a0,-64\(a0\) # 0 <foo>
>> [ ]+48:[ ]+00000517[ ]+auipc[ ]a0,0x0
>> [ ]+4c:[ ]+01052503[ ]+lw[ ]+a0,16\(a0\) # 58 <foo\+0x58>
>> [ ]+50:[ ]+00000517[ ]+auipc[ ]a0,0x0
>> --
>> 2.17.1
>>
>>
Of course, if Nelson approves this patch, I will reorganize the test cases as per
your suggestion and submit the v2 patch.
By the way, this patch has passed the regression tests in the riscv64 toolchain.
Thanks
Xiao Zeng
More information about the Binutils
mailing list