RISC-V: Avoid relocation for a function symbol self-referenced with -mno-relax
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
Commit Message
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
Comments
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.
> 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...
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
>
>
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
On Tue, Jan 7, 2025 at 11:03 AM Xiao Zeng <zengxiao@eswincomputing.com>
wrote:
> 2025-01-07 10:46 Nelson Chu <nelson@rivosinc.com> wrote:
> >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.
>
Even the symbol seems referenced within its body in assembler, if it is
preemptive, then it may be changed in the linker...
Nelson
2025-01-07 12:02 Nelson Chu <nelson@rivosinc.com> wrote:
>
>On Tue, Jan 7, 2025 at 11:03 AM Xiao Zeng <zengxiao@eswincomputing.com>
>wrote:
>
>> 2025-01-07 10:46 Nelson Chu <nelson@rivosinc.com> wrote:
>> >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.
>>
>
>Even the symbol seems referenced within its body in assembler, if it is
>preemptive, then it may be changed in the linker...
Obviously, I didn't consider this point. If possible, could you kindly ask
Nelson to provide an example? I would greatly appreciate it.
>
>Nelson
Thanks
Xiao Zeng
On 07.01.2025 04:03, Xiao Zeng wrote:
> 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:
>>> --- 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.
Yet where in your change is the check that the reference is a _self_ reference?
How would you even reliably recognize such? Consider the case of one function-
like construct falling through to another one, with the latter having such a
self reference. If the latter is preempted, for it the omitted relocation
doesn't matter because it won't be invoked from elsewhere. Whereas if at the
same time the former isn't preempted, the references still need resolving
correctly to whatever overrode the latter symbol.
Jan
2025-01-07 16:07 Jan Beulich <jbeulich@suse.com> wrote:
>
>On 07.01.2025 04:03, Xiao Zeng wrote:
>> 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:
>>>> --- 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.
>
>Yet where in your change is the check that the reference is a _self_ reference?
>How would you even reliably recognize such? Consider the case of one function-
>like construct falling through to another one, with the latter having such a
>self reference. If the latter is preempted, for it the omitted relocation
>doesn't matter because it won't be invoked from elsewhere. Whereas if at the
>same time the former isn't preempted, the references still need resolving
>correctly to whatever overrode the latter symbol.
>
>Jan
Thank you, Jan. There are indeed some cases I hadn't considered.
Your response has been very helpful to me.
Thanks
Xiao Zeng
@@ -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;
new file mode 100644
@@ -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
new file mode 100644
@@ -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
new file mode 100644
@@ -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
@@ -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