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

[Jinyang He <hejinyang@loongson.cn>]

On 2023-07-13 15:19, Jinyang He wrote:

> Hi,
>
>
> We should pay attention to them, the bug in relaxation. Relaxation may 
> not work correctly because of the bug1 reported by the second patch 
> and the bug2 reported in the my previous email, although it works well 
> in most of the time. Those 2 bugs are just what we've found, but there 
> may be hidden problems.
> I also tested the bug2 on RISCV and although it couldn't be resolved, 
> it reported an error.
>
> Test is,
> $ cat c.s
>   .section ".another.text", "x"
>   .fill 1, 4, 0
>   call fn2
>   call fn2
> fn1:
>   nop
>
> .text
> fn2:
>   .fill 4, 4, 0
>   call fn1
>
> $ ./gas/as-new  -march=rv32ic_zicsr -mno-arch-attr  -o c.o c.s
> $ ./ld/ld-new c.o -o c -e 0 --section-start=.another.text=0x1000000 
> -Ttext 0x1100000
> c.o: in function `fn2':
> (.text+0x10): relocation truncated to fit: R_RISCV_JAL against `fn1'
>
> So, I'll find the reason why reloc_bits_pcrel20_s2() not reports the 
> overfollow error, and add error report which I think it is more 
> important than these two patches so that we can get feedback when we 
> link.
>

Now, I find that the sign-overflow-check has bug, include 
reloc_bits_pcrel20_s2(), reloc_bits_b26() and (others, I did not test 
them). And I think the "mask" should be
     bfd_signed_vma mask = ((bfd_signed_vma)0x1 << (howto->bitsize - 
(sign ? 1 : 0)) - 1;

The test about reloc_bits_b26() is,

$ cat d.s
         .section ".another.text", "x"
         b       sym1

         .text
         nop
sym1:
         nop

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

I'll do fix and send v2 after you test spec2006. Or other responses?


Thanks!


>
> On 2023-07-13 11:13, mengqinggang wrote:
>> Hi,
>>
>> I will apply this two patches to see if there are any errors in 
>> specc2006.
>>
>> 在 2023/7/12 下午4:08, Jinyang He 写道:
>>> 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"