Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues

[Nelson Chu <nelson@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 5507 bytes --]

I think the quite easy solution is - don't use riscv_elf_append_rela to
emit dynamic IRELATIVE for R_RISCV_32/64 into iplt section.  Seems like
commit 51a8a7c2e3cc only handles the finish_dynamic_symbol, but forgot to
apply a similar fix in the relocate_section.

https://github.com/bminor/binutils-gdb/blob/master/bfd/elfnn-riscv.c#L2408
diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 604f6de4511..dca446c0495 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -2399,13 +2399,16 @@ riscv_elf_relocate_section (bfd *output_bfd,
                       2. .rela.got section in dynamic executable.
                       3. .rela.iplt section in static executable.  */
                    if (bfd_link_pic (info))
-                     sreloc = htab->elf.irelifunc;
+                     riscv_elf_append_rela (output_bfd,
htab->elf.irelifunc, &outrel);
                    else if (htab->elf.splt != NULL)
-                     sreloc = htab->elf.srelgot;
+                     riscv_elf_append_rela (output_bfd, htab->elf.srelgot,
&outrel);
                    else
-                     sreloc = htab->elf.irelplt;
-
-                   riscv_elf_append_rela (output_bfd, sreloc, &outrel);
+                     {
+                       const struct elf_backend_data *bed =
get_elf_backend_data (output_bfd);
+                       bfd_vma iplt_idx = htab->last_iplt_index--;
+                       bfd_byte *loc = htab->elf.irelplt->contents +
iplt_idx * sizeof (ElfNN_External_Rela);
+                       bed->s->swap_reloca_out (output_bfd, &outrel, loc);
+                     }

                    /* If this reloc is against an external symbol, we
                       do not want to fiddle with the addend.  Otherwise,

The above changes seem to fix the testcase you provided, but without
testing fully riscv-gnu-toolchain regressions.
Or we should find a way to handle reloc_index for iplt, and all use
riscv_elf_append_rela to emit the dynamic relocation.

Nelson

On Thu, May 16, 2024 at 3:30 PM Hau Hsu <hau.hsu@sifive.com> wrote:

>
> On May 16, 2024, at 3:07 PM, Nelson Chu <nelson@rivosinc.com> wrote:
>
> On Thu, May 16, 2024 at 2:16 PM Hau Hsu <hau.hsu@sifive.com> wrote:
>
>> Hi Nelson,
>>
>> Sorry for the late reply.
>>
>> On May 8, 2024, at 9:00 AM, Nelson Chu <nelson@rivosinc.com> wrote:
>>
>> Can you provide the testcase to show the case which I mentioned in the
>> pr13302?  Which is that an ifunc with a dynamic jump slot calls another
>> ifunc, and are not in the rel.dyn.
>>
>> I am not quite sure what's the issue you mentioned in PR13302.
>> You mean the order issue of cause by one ifunc (generates
>> JUMP_SLOT[32|64]) calls another JUMP_SLOT[32|64] ifunc?
>>
>> Since according to the testcase below, it seems no requirement to apply
>> this fix though.
>>
>> The new test case uses two methods to call ifuncs:
>> 1. Through a normal function call: PLT + GOT
>> 2. Through a global function pointer: GOT only
>>
>> Without the fix, the relocation of the first method overwrites the
>> second's.
>> The relocation section of my test case would be:
>>
>> Relocation section '.rela.plt' at offset 0x94 contains 2 entries:
>>  Offset     Info    Type                Sym. Value  Symbol's Name + Addend
>> 000110dc  0000003a R_RISCV_IRELATIVE                 100c0
>> 00000000  00000000 R_RISCV_NONE                      0
>>
>>
>> With the fix, it becomes
>>
>> Relocation section '.rela.plt' at offset 0x94 contains 2 entries:
>>  Offset     Info    Type                Sym. Value  Symbol's Name + Addend
>> 000110e0  0000003a R_RISCV_IRELATIVE                 100c0
>> 000110dc  0000003a R_RISCV_IRELATIVE                 100c0
>>
>>
> So it seems like the overwrite problem, not the order problem we were
> discussing in the pr13302...
>
>
> Yes. Sorry that I didn't explain the whole story well.
>
> This PR is originally to fix the overwrite problem, as my commit message
> says:
> > This commit resolved two issues:
> > 1. When an ifunc is referenced by a pointer, the relocation of
> >   the pointer in .rela.plt would be overwritten by normal ifunc call.
> We found the issue when building glibc testbench statically.
>
> To fix this issue, I sent a PR (
> https://sourceware.org/pipermail/binutils/2023-July/128485.html) about a
> year ago.
> The PR use the method smilier to your previous commit, i.e.
>
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=51a8a7c2e3cc0730831963651a55d23d1fae624d
> Then you suggested to check whether the relocation order is correct.
> After that I checked the X86 implementation, I sent this PR.
>
>
>
>> --- /dev/null
>>> +++ b/ld/testsuite/ld-riscv-elf/ifunc-macro.s
>>> @@ -0,0 +1,19 @@
>>> +/* Define macros to handle similar behaviors for rv32/rv64.
>>> +   Assumes macro "__64_bit__" defined for rv64.
>>> +   The macro is specifically defined for ifunc tests in
>>> ld-riscv-elf.exp. */
>>> +
>>> +.macro PTR_DATA name
>>> +.ifdef __64_bit__
>>> +       .quad   \name
>>> +.else
>>> +       .long   \name
>>> +.endif
>>> +.endm
>>> +
>>> +.macro LOAD rd, rs, offset
>>> +.ifdef __64_bit__
>>> +       ld      \rd, \offset (\rs)
>>> +.else
>>> +       lw      \rd, \offset (\rs)
>>> +.endif
>>> +.endm
>>
>>
> Btw, can we not use these macroes?
>
>
> No problem. I just want to avoid similar codes in the ifunc tests.
>
>
> Nelson
>
>
>