Re: [PATCH 1/2] LoongArch: bfd: Remove elf_seg_map condition in loongarch_elf_relax_section

[Jinyang He <hejinyang@loongson.cn>]

Hi,


I'm not fimiliar with the process of output binary. I just noted that it 
calls lang_relax_sections() before calling 
_bfd_elf_map_sections_to_segments() in ldelf_map_segments(). Thus I 
think the condition, elf_seg_map (info->output_bfd) == NULL, always be 
true when first time it calls loongarch_elf_relax_section(). Actually we 
can get right symbol value when relaxation, but ...

You reminder me that the symbol value will be reduced due to relaxation, 
which like the instruction pc is reduced. Then we cannot get right 
symbol value if the symbol in relaxable section. And I write a test,

$ cat c.s
     .section ".another.text", "x"
     nop
     la.local $a0, sym1
sym2:
     nop

     .text
sym1:
     nop
     nop
     nop
     la.local $a0, sym2

$ ./gas/as-new -mabi=lp64d -o c.o c.s
$ ./ld/ld-new c.o -o c -e 0 --section-start=.another.text=0x120000000 
-Ttext 0x120200000

Disassembly of section .another.text:

0000000120000000 <sym2-0x8>:
    120000000:    03400000     andi            $zero, $zero, 0x0
    120000004:    18ffffe4     pcaddi          $a0, 524287(0x7ffff)

0000000120000008 <sym2>:
    120000008:    03400000     andi            $zero, $zero, 0x0

Disassembly of section .text:

0000000120200000 <sym1>:
    120200000:    03400000     andi            $zero, $zero, 0x0
    120200004:    03400000     andi            $zero, $zero, 0x0
    120200008:    03400000     andi            $zero, $zero, 0x0
    12020000c:    18ffffe4     pcaddi          $a0, 524287(0x7ffff)

Due to the bug repaired by Patch2 and the bug reported by this test. And 
I also did not see that condition in other archs. I'm not sure that the 
relaxation spec2006 error is caused by that condition or those bugs.


On 2023-07-12 14:35, mengqinggang wrote:
> Hi,
>
> Without this condition, the value of 'symbol - pc' may be differences 
> between
> loongarch_elf_relax_section and  loongarch_elf_relocate_section.
> And it will cause some spec2006 test errors.
>
> 在 2023/7/11 下午4:49, Jinyang He 写道:
>> When I tried "./ld-new sth." the relax does not work because the
>> elf_seg_map (info->output_bfd) was NULL in loongarch_elf_relax_section.
>> There is no need for the segment info, we can get the address through
>> the section info. Thus, remove this condition.
>>
>> bfd/ChangeLog:
>>
>>     * elfnn-loongarch.c (loongarch_elf_relax_section): Remove
>>     elf_seg_map condition.
>>
>> ld/ChangeLog:
>>
>>     * testsuite/ld-loongarch-elf/relax.exp: Update trigger method.
>>     * testsuite/ld-loongarch-elf/relax.dd: New test.
>> ---
>>   bfd/elfnn-loongarch.c                   |  1 -
>>   ld/testsuite/ld-loongarch-elf/relax.dd  |  6 +++++
>>   ld/testsuite/ld-loongarch-elf/relax.exp | 35 +++++++------------------
>>   3 files changed, 16 insertions(+), 26 deletions(-)
>>   create mode 100644 ld/testsuite/ld-loongarch-elf/relax.dd
>>
>> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
>> index d3d8419d8..01f349a24 100644
>> --- a/bfd/elfnn-loongarch.c
>> +++ b/bfd/elfnn-loongarch.c
>> @@ -3845,7 +3845,6 @@ loongarch_elf_relax_section (bfd *abfd, 
>> asection *sec,
>>         || sec->sec_flg0
>>         || (sec->flags & SEC_RELOC) == 0
>>         || sec->reloc_count == 0
>> -      || elf_seg_map (info->output_bfd) == NULL
>>         || (info->disable_target_specific_optimizations
>>         && info->relax_pass == 0)
>>         /* The exp_seg_relro_adjust is enum phase_enum (0x4),
>> diff --git a/ld/testsuite/ld-loongarch-elf/relax.dd 
>> b/ld/testsuite/ld-loongarch-elf/relax.dd
>> new file mode 100644
>> index 000000000..e266beb8f
>> --- /dev/null
>> +++ b/ld/testsuite/ld-loongarch-elf/relax.dd
>> @@ -0,0 +1,6 @@
>> +#...
>> +.*pcaddi.*
>> +.*ld.w.*
>> +.*pcaddi.*
>> +.*ld.w.*
>> +#pass
>> diff --git a/ld/testsuite/ld-loongarch-elf/relax.exp 
>> b/ld/testsuite/ld-loongarch-elf/relax.exp
>> index 7ff876d79..d2bb967c6 100644
>> --- a/ld/testsuite/ld-loongarch-elf/relax.exp
>> +++ b/ld/testsuite/ld-loongarch-elf/relax.exp
>> @@ -21,31 +21,6 @@
>>     if [istarget loongarch64-*-*] {
>>   -  if [isbuild loongarch64-*-*] {
>> -    set testname "loongarch relax build"
>> -    set pre_builds [list \
>> -      [list \
>> -    "$testname" \
>> -    "" \
>> -    "" \
>> -    {relax.s} \
>> -    {} \
>> -    "relax" \
>> -      ] \
>> -    ]
>> -
>> -    run_cc_link_tests $pre_builds
>> -
>> -    if [file exist "tmpdir/relax"] {
>> -      set objdump_output [run_host_cmd "objdump" "-d tmpdir/relax"]
>> -      if { [ regexp ".*pcaddi.*pcaddi.*" $objdump_output] } {
>> -    pass "loongarch relax"
>> -      } {
>> -    fail "loongarch relax"
>> -      }
>> -    }
>> -  }
>> -
>>     run_ld_link_tests \
>>         [list \
>>         [list \
>> @@ -58,6 +33,16 @@ if [istarget loongarch64-*-*] {
>>             ] \
>>             "relax-align" \
>>         ] \
>> +      [list \
>> +          "relax" \
>> +          "-e 0 -Ttext 0x10000 -Tdata 0x20000" "" \
>> +          "" \
>> +          {relax.s} \
>> +          [list \
>> +          [list objdump -d relax.dd] \
>> +          ] \
>> +          "relax" \
>> +      ] \
>>         ]
>>       set objdump_flags "-s -j .data"