* [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P @ 2024-05-06 4:45 Hau Hsu 2024-05-06 4:45 ` [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues Hau Hsu 2024-05-08 1:03 ` [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P Nelson Chu 0 siblings, 2 replies; 11+ messages in thread From: Hau Hsu @ 2024-05-06 4:45 UTC (permalink / raw) To: binutils, kito.cheng, hau.hsu Add POINTER_LOCAL_IFUNC_P which returns TRUE for pointer reference to local IFUNC symbol. Add PLT_LOCAL_IFUNC_P which returns TRUE for PLT reference to local IFUNC symbol. Copied from x86 commit: cf1070f1a1ca1f8be1cd88aa6ece55a25e6a887b --- bfd/elfnn-riscv.c | 11 ++--------- bfd/elfxx-riscv.h | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c index 95abf2d10e2..3a30b7a4bd9 100644 --- a/bfd/elfnn-riscv.c +++ b/bfd/elfnn-riscv.c @@ -2373,9 +2373,7 @@ riscv_elf_relocate_section (bfd *output_bfd, outrel.r_offset += input_section->output_section->vma + input_section->output_offset; - if (h->dynindx == -1 - || h->forced_local - || bfd_link_executable (info)) + if (POINTER_LOCAL_IFUNC_P (info, h)) { info->callbacks->minfo (_("Local IFUNC function `%s' in %pB\n"), @@ -3272,12 +3270,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, bfd_put_NN (output_bfd, sec_addr (plt), loc); rela.r_offset = got_address; - - if (h->dynindx == -1 - || ((bfd_link_executable (info) - || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT) - && h->def_regular - && h->type == STT_GNU_IFUNC)) + if (PLT_LOCAL_IFUNC_P (info, h)) { info->callbacks->minfo (_("Local IFUNC function `%s' in %pB\n"), h->root.root.string, diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h index 49be71746b9..9efac27864c 100644 --- a/bfd/elfxx-riscv.h +++ b/bfd/elfxx-riscv.h @@ -27,6 +27,20 @@ #define RISCV_UNKNOWN_VERSION -1 +/* TRUE if this is a pointer reference to a local IFUNC. */ +#define POINTER_LOCAL_IFUNC_P(INFO, H) \ + ((H)->dynindx == -1 \ + || (H)->forced_local \ + || bfd_link_executable (INFO)) + +/* TRUE if this is a PLT reference to a local IFUNC. */ +#define PLT_LOCAL_IFUNC_P(INFO, H) \ + ((H)->dynindx == -1 \ + || ((bfd_link_executable (INFO) \ + || ELF_ST_VISIBILITY ((H)->other) != STV_DEFAULT) \ + && (H)->def_regular \ + && (H)->type == STT_GNU_IFUNC)) + struct riscv_elf_params { /* Whether to relax code sequences to GP-relative addressing. */ -- 2.37.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-06 4:45 [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P Hau Hsu @ 2024-05-06 4:45 ` Hau Hsu 2024-05-08 1:00 ` Nelson Chu 2024-05-08 1:03 ` [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P Nelson Chu 1 sibling, 1 reply; 11+ messages in thread From: Hau Hsu @ 2024-05-06 4:45 UTC (permalink / raw) To: binutils, kito.cheng, hau.hsu 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. 2. R_RISCV_IRELATIVE should come last. See https://sourceware.org/bugzilla/show_bug.cgi?id=13302 This patch fixes above issues with the implementation similar to x86. That is, by adding two variables to record the next relocation index for R_RISCV_IRELATIVE and R_RISCV_JUMP_SLOT. A previous commit partially fixed the overwrite issue: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=51a8a7c2e3cc0730831963651a55d23d1fae624d Details below: There are three ways to access a local (non-preemptible) ifunc: (1) Through PLT + GOT, i.e. use normal function call. (2) Through GOT, i.e. use a local function pointer. (3) Through a global function pointer. For (1) and (2), a R_RISCV_IRELATIVE is created for GOT entry. For (3), a R_RISCV_IRELATIVE is created for the global pointer. The relocations overwrite issue is that, the R_RISCV_IRELATIVE created for (1) overwrite the relocations for (2) and (3). The previous commit partially fixed the overwrite issue for (2), but not for (3). --- bfd/elfnn-riscv.c | 65 +++++++++---------- ld/testsuite/ld-riscv-elf/ifunc-macro.s | 19 ++++++ .../ifunc-plt-got-overwrite-02-exe.rd | 4 ++ .../ifunc-plt-got-overwrite-02-pic.rd | 11 ++++ .../ifunc-plt-got-overwrite-02-pie.rd | 7 ++ .../ld-riscv-elf/ifunc-plt-got-overwrite-02.d | 16 +++++ .../ld-riscv-elf/ifunc-plt-got-overwrite-02.s | 34 ++++++++++ ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 7 ++ ld/testsuite/ld-riscv-elf/variant_cc-now.d | 4 +- ld/testsuite/ld-riscv-elf/variant_cc-shared.d | 4 +- 10 files changed, 132 insertions(+), 39 deletions(-) create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-macro.s create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c index 3a30b7a4bd9..d9243648d24 100644 --- a/bfd/elfnn-riscv.c +++ b/bfd/elfnn-riscv.c @@ -224,8 +224,12 @@ struct riscv_elf_link_hash_table htab_t loc_hash_table; void * loc_hash_memory; - /* The index of the last unused .rel.iplt slot. */ - bfd_vma last_iplt_index; + /* The index of the next R_RISCV_JUMP_SLOT entry in .rela.plt. */ + bfd_vma next_jump_slot_index; + + /* R_RISCV_IRELATIVE entry in .rela.plt comes last. + Use this to record the index of the next R_RISCV_IRELATIVE entry. */ + bfd_vma next_irelative_index; /* The data segment phase, don't relax the section when it is exp_seg_relro_adjust. */ @@ -1273,6 +1277,7 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, void *inf) /* We also need to make an entry in the .rela.plt section. */ htab->elf.srelplt->size += sizeof (ElfNN_External_Rela); + htab->elf.srelplt->reloc_count ++; /* If this symbol is not defined in a regular file, and we are not generating a shared library, then set the symbol to this @@ -1622,10 +1627,14 @@ riscv_elf_late_size_sections (bfd *output_bfd, struct bfd_link_info *info) local ifunc symbols. */ htab_traverse (htab->loc_hash_table, allocate_local_ifunc_dynrelocs, info); - /* Used to resolve the dynamic relocs overwite problems when - generating static executable. */ - if (htab->elf.irelplt) - htab->last_iplt_index = htab->elf.irelplt->reloc_count - 1; + + htab->next_jump_slot_index = 0; + + if (htab->elf.srelplt) + htab->next_irelative_index = htab->elf.srelplt->reloc_count - 1; + else if (htab->elf.irelplt) + htab->next_irelative_index = htab->elf.irelplt->reloc_count - 1; + if (htab->elf.sgotplt) { @@ -3201,15 +3210,14 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, if (h->plt.offset != (bfd_vma) -1) { - /* We've decided to create a PLT entry for this symbol. */ + /* We've decided to create a PLT entry for this symbol. + Add a PLT entry, a GOT entry and a relocation entry. */ bfd_byte *loc; bfd_vma i, header_address, plt_idx, got_offset, got_address; uint32_t plt_entry[PLT_ENTRY_INSNS]; Elf_Internal_Rela rela; asection *plt, *gotplt, *relplt; - /* When building a static executable, use .iplt, .igot.plt and - .rela.iplt sections for STT_GNU_IFUNC symbols. */ if (htab->elf.splt != NULL) { plt = htab->elf.splt; @@ -3218,6 +3226,8 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, } else { + /* When building a static executable, use .iplt, .igot.plt and + .rela.iplt sections for STT_GNU_IFUNC symbols. */ plt = htab->elf.iplt; gotplt = htab->elf.igotplt; relplt = htab->elf.irelplt; @@ -3238,7 +3248,8 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, header_address = sec_addr (plt); /* Calculate the index of the entry and the offset of .got.plt entry. - For static executables, we don't reserve anything. */ + The index of .got.plt is the same as .plt (exclude the header entries), + since this section is dedicated for plt. */ if (plt == htab->elf.splt) { plt_idx = (h->plt.offset - PLT_HEADER_SIZE) / PLT_ENTRY_SIZE; @@ -3246,6 +3257,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, } else { + /* For static executables, we don't need to reserve the header entry. */ plt_idx = h->plt.offset / PLT_ENTRY_SIZE; got_offset = plt_idx * GOT_ENTRY_SIZE; } @@ -3269,6 +3281,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, loc = gotplt->contents + (got_address - sec_addr (gotplt)); bfd_put_NN (output_bfd, sec_addr (plt), loc); + /* Fill in the relocation entry. */ rela.r_offset = got_address; if (PLT_LOCAL_IFUNC_P (info, h)) { @@ -3283,17 +3296,20 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, rela.r_addend = h->root.u.def.value + sec->output_section->vma + sec->output_offset; + bfd_vma rela_idx = htab->next_irelative_index--; + loc = relplt->contents + rela_idx * sizeof (ElfNN_External_Rela); + bed->s->swap_reloca_out (output_bfd, &rela, loc); } else { /* Fill in the entry in the .rela.plt section. */ rela.r_info = ELFNN_R_INFO (h->dynindx, R_RISCV_JUMP_SLOT); rela.r_addend = 0; + bfd_vma rela_idx = htab->next_jump_slot_index++; + loc = relplt->contents + rela_idx * sizeof (ElfNN_External_Rela); + bed->s->swap_reloca_out (output_bfd, &rela, loc); } - loc = relplt->contents + plt_idx * sizeof (ElfNN_External_Rela); - bed->s->swap_reloca_out (output_bfd, &rela, loc); - if (!h->def_regular) { /* Mark the symbol as undefined, rather than as defined in @@ -3315,7 +3331,6 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, asection *sgot; asection *srela; Elf_Internal_Rela rela; - bool use_elf_append_rela = true; /* This symbol has an entry in the GOT. Set it up. */ @@ -3338,10 +3353,6 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, /* Use .rela.iplt section to store .got relocations in static executable. */ srela = htab->elf.irelplt; - - /* Do not use riscv_elf_append_rela to add dynamic - relocs. */ - use_elf_append_rela = false; } if (SYMBOL_REFERENCES_LOCAL (info, h)) @@ -3417,23 +3428,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, bfd_put_NN (output_bfd, 0, sgot->contents + (h->got.offset & ~(bfd_vma) 1)); - if (use_elf_append_rela) - riscv_elf_append_rela (output_bfd, srela, &rela); - else - { - /* Use riscv_elf_append_rela to add the dynamic relocs into - .rela.iplt may cause the overwrite problems. Since we insert - the relocs for PLT didn't handle the reloc_index of .rela.iplt, - but the riscv_elf_append_rela adds the relocs to the place - that are calculated from the reloc_index (in seqential). - - One solution is that add these dynamic relocs (GOT IFUNC) - from the last of .rela.iplt section. */ - bfd_vma iplt_idx = htab->last_iplt_index--; - bfd_byte *loc = srela->contents - + iplt_idx * sizeof (ElfNN_External_Rela); - bed->s->swap_reloca_out (output_bfd, &rela, loc); - } + riscv_elf_append_rela (output_bfd, srela, &rela); } if (h->needs_copy) diff --git a/ld/testsuite/ld-riscv-elf/ifunc-macro.s b/ld/testsuite/ld-riscv-elf/ifunc-macro.s new file mode 100644 index 00000000000..88935a55814 --- /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 diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd new file mode 100644 index 00000000000..0de47a4009f --- /dev/null +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd @@ -0,0 +1,4 @@ +Relocation section '.rela.plt' at .* +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd new file mode 100644 index 00000000000..d7ecd4a4a69 --- /dev/null +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd @@ -0,0 +1,11 @@ +Relocation section '.rela.got' at .* +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_(32|64)[ ]+foo1\(\)[ ]+foo1 \+ 0 +#... +Relocation section '.rela.ifunc' at .* +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_(32|64)[ ]+foo2\(\)[ ]+foo2 \+ 0 +#... +Relocation section '.rela.plt' at .* +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+foo1\(\)[ ]+foo1 \+ 0 diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd new file mode 100644 index 00000000000..532cbf8a86a --- /dev/null +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd @@ -0,0 +1,7 @@ +Relocation section '.rela.ifunc' at .* +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* + +Relocation section '.rela.plt' at .* +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d new file mode 100644 index 00000000000..3e33ac619d4 --- /dev/null +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d @@ -0,0 +1,16 @@ +#... +Disassembly of section .plt: +#... +0+[0-9a-f]+ <(\*ABS\*\+0x[0-9a-f]+@plt|foo@plt|.plt)>: +#... +Disassembly of section .text: +#... +0+[0-9a-f]+ <foo_resolver>: +.*:[ ]+[0-9a-f]+[ ]+ret + +0+[0-9a-f]+ <_start>: +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+.* +.*:[ ]+[0-9a-f]+[ ]+(lw|ld)[ ]+.*<(.*)> +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+.* +.*:[ ]+[0-9a-f]+[ ]+jalr[ ]+.*<(.*plt.*)> +.*:[ ]+[0-9a-f]+[ ]+ret diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s new file mode 100644 index 00000000000..ff6d171591c --- /dev/null +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s @@ -0,0 +1,34 @@ +/* There are 2 ifuncs: foo1 and foo2. + foo1 is accessed by a function call. + foo2 is referenced by a global pointer (foo2_addr). */ + .include "ifunc-macro.s" + .globl foo2_addr + .section .data + .type foo2_addr, @object +foo2_addr: + PTR_DATA foo2 + + .text + .type foo_resolver, @function +foo_resolver: + ret + .size foo_resolver, .-foo_resolver + + .globl foo1 + .type foo1, %gnu_indirect_function + .set foo1, foo_resolver + + .globl foo2 + .type foo2, %gnu_indirect_function + .set foo2, foo_resolver + + .globl _start + .type _start, @function +_start: +.L1: + auipc x1, %got_pcrel_hi (foo1) + LOAD x1, x1, %pcrel_lo (.L1) + call foo1 + + ret + .size _start, .-_start diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp index a1dd0e5e37e..10011445683 100644 --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp @@ -284,6 +284,13 @@ if [istarget "riscv*-*-*"] { run_dump_test_ifunc "ifunc-plt-got-overwrite" rv64 pie run_dump_test_ifunc "ifunc-plt-got-overwrite" rv64 pic + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 exe + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 pie + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 pic + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 exe + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 pie + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 pic + # TODO: Make the following tests work under RV32. if [istarget "riscv32-*-*"] { return diff --git a/ld/testsuite/ld-riscv-elf/variant_cc-now.d b/ld/testsuite/ld-riscv-elf/variant_cc-now.d index 9453554a159..3ed5ab3cae0 100644 --- a/ld/testsuite/ld-riscv-elf/variant_cc-now.d +++ b/ld/testsuite/ld-riscv-elf/variant_cc-now.d @@ -7,11 +7,11 @@ Relocation section '.rela.plt' at .* #... [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+nocc_global_default_undef \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+cc_global_default_undef \+ 0 +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+cc_global_default_def \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+nocc_global_default_def \+ 0 -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 diff --git a/ld/testsuite/ld-riscv-elf/variant_cc-shared.d b/ld/testsuite/ld-riscv-elf/variant_cc-shared.d index ffb69a392f2..6b73cf1de20 100644 --- a/ld/testsuite/ld-riscv-elf/variant_cc-shared.d +++ b/ld/testsuite/ld-riscv-elf/variant_cc-shared.d @@ -7,11 +7,11 @@ Relocation section '.rela.plt' at .* #... [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+nocc_global_default_undef \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+cc_global_default_undef \+ 0 +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+cc_global_default_def \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+nocc_global_default_def \+ 0 -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 -- 2.37.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-06 4:45 ` [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues Hau Hsu @ 2024-05-08 1:00 ` Nelson Chu 2024-05-16 6:16 ` Hau Hsu 0 siblings, 1 reply; 11+ messages in thread From: Nelson Chu @ 2024-05-08 1:00 UTC (permalink / raw) To: Hau Hsu; +Cc: binutils, kito.cheng [-- Attachment #1: Type: text/plain, Size: 18886 bytes --] 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. Since according to the testcase below, it seems no requirement to apply this fix though. Thanks Nelson On Mon, May 6, 2024 at 12:46 PM Hau Hsu <hau.hsu@sifive.com> wrote: > 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. > 2. R_RISCV_IRELATIVE should come last. > See https://sourceware.org/bugzilla/show_bug.cgi?id=13302 > > This patch fixes above issues with the implementation similar to x86. > That is, by adding two variables to record the next relocation index for > R_RISCV_IRELATIVE and R_RISCV_JUMP_SLOT. > > A previous commit partially fixed the overwrite issue: > > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=51a8a7c2e3cc0730831963651a55d23d1fae624d > > Details below: > > There are three ways to access a local (non-preemptible) ifunc: > (1) Through PLT + GOT, i.e. use normal function call. > (2) Through GOT, i.e. use a local function pointer. > (3) Through a global function pointer. > > For (1) and (2), a R_RISCV_IRELATIVE is created for GOT entry. > For (3), a R_RISCV_IRELATIVE is created for the global pointer. > The relocations overwrite issue is that, the R_RISCV_IRELATIVE created > for (1) overwrite the relocations for (2) and (3). > The previous commit partially fixed the overwrite issue for (2), but not > for (3). > --- > bfd/elfnn-riscv.c | 65 +++++++++---------- > ld/testsuite/ld-riscv-elf/ifunc-macro.s | 19 ++++++ > .../ifunc-plt-got-overwrite-02-exe.rd | 4 ++ > .../ifunc-plt-got-overwrite-02-pic.rd | 11 ++++ > .../ifunc-plt-got-overwrite-02-pie.rd | 7 ++ > .../ld-riscv-elf/ifunc-plt-got-overwrite-02.d | 16 +++++ > .../ld-riscv-elf/ifunc-plt-got-overwrite-02.s | 34 ++++++++++ > ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 7 ++ > ld/testsuite/ld-riscv-elf/variant_cc-now.d | 4 +- > ld/testsuite/ld-riscv-elf/variant_cc-shared.d | 4 +- > 10 files changed, 132 insertions(+), 39 deletions(-) > create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-macro.s > create mode 100644 > ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd > create mode 100644 > ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd > create mode 100644 > ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd > create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d > create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s > > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c > index 3a30b7a4bd9..d9243648d24 100644 > --- a/bfd/elfnn-riscv.c > +++ b/bfd/elfnn-riscv.c > @@ -224,8 +224,12 @@ struct riscv_elf_link_hash_table > htab_t loc_hash_table; > void * loc_hash_memory; > > - /* The index of the last unused .rel.iplt slot. */ > - bfd_vma last_iplt_index; > + /* The index of the next R_RISCV_JUMP_SLOT entry in .rela.plt. */ > + bfd_vma next_jump_slot_index; > + > + /* R_RISCV_IRELATIVE entry in .rela.plt comes last. > + Use this to record the index of the next R_RISCV_IRELATIVE entry. */ > + bfd_vma next_irelative_index; > > /* The data segment phase, don't relax the section > when it is exp_seg_relro_adjust. */ > @@ -1273,6 +1277,7 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, > void *inf) > > /* We also need to make an entry in the .rela.plt section. */ > htab->elf.srelplt->size += sizeof (ElfNN_External_Rela); > + htab->elf.srelplt->reloc_count ++; > > /* If this symbol is not defined in a regular file, and we are > not generating a shared library, then set the symbol to this > @@ -1622,10 +1627,14 @@ riscv_elf_late_size_sections (bfd *output_bfd, > struct bfd_link_info *info) > local ifunc symbols. */ > htab_traverse (htab->loc_hash_table, allocate_local_ifunc_dynrelocs, > info); > > - /* Used to resolve the dynamic relocs overwite problems when > - generating static executable. */ > - if (htab->elf.irelplt) > - htab->last_iplt_index = htab->elf.irelplt->reloc_count - 1; > + > + htab->next_jump_slot_index = 0; > + > + if (htab->elf.srelplt) > + htab->next_irelative_index = htab->elf.srelplt->reloc_count - 1; > + else if (htab->elf.irelplt) > + htab->next_irelative_index = htab->elf.irelplt->reloc_count - 1; > + > > if (htab->elf.sgotplt) > { > @@ -3201,15 +3210,14 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > > if (h->plt.offset != (bfd_vma) -1) > { > - /* We've decided to create a PLT entry for this symbol. */ > + /* We've decided to create a PLT entry for this symbol. > + Add a PLT entry, a GOT entry and a relocation entry. */ > bfd_byte *loc; > bfd_vma i, header_address, plt_idx, got_offset, got_address; > uint32_t plt_entry[PLT_ENTRY_INSNS]; > Elf_Internal_Rela rela; > asection *plt, *gotplt, *relplt; > > - /* When building a static executable, use .iplt, .igot.plt and > - .rela.iplt sections for STT_GNU_IFUNC symbols. */ > if (htab->elf.splt != NULL) > { > plt = htab->elf.splt; > @@ -3218,6 +3226,8 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > } > else > { > + /* When building a static executable, use .iplt, .igot.plt and > + .rela.iplt sections for STT_GNU_IFUNC symbols. */ > plt = htab->elf.iplt; > gotplt = htab->elf.igotplt; > relplt = htab->elf.irelplt; > @@ -3238,7 +3248,8 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > header_address = sec_addr (plt); > > /* Calculate the index of the entry and the offset of .got.plt > entry. > - For static executables, we don't reserve anything. */ > + The index of .got.plt is the same as .plt (exclude the header > entries), > + since this section is dedicated for plt. */ > if (plt == htab->elf.splt) > { > plt_idx = (h->plt.offset - PLT_HEADER_SIZE) / PLT_ENTRY_SIZE; > @@ -3246,6 +3257,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > } > else > { > + /* For static executables, we don't need to reserve the header > entry. */ > plt_idx = h->plt.offset / PLT_ENTRY_SIZE; > got_offset = plt_idx * GOT_ENTRY_SIZE; > } > @@ -3269,6 +3281,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > loc = gotplt->contents + (got_address - sec_addr (gotplt)); > bfd_put_NN (output_bfd, sec_addr (plt), loc); > > + /* Fill in the relocation entry. */ > rela.r_offset = got_address; > if (PLT_LOCAL_IFUNC_P (info, h)) > { > @@ -3283,17 +3296,20 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > rela.r_addend = h->root.u.def.value > + sec->output_section->vma > + sec->output_offset; > + bfd_vma rela_idx = htab->next_irelative_index--; > + loc = relplt->contents + rela_idx * sizeof > (ElfNN_External_Rela); > + bed->s->swap_reloca_out (output_bfd, &rela, loc); > } > else > { > /* Fill in the entry in the .rela.plt section. */ > rela.r_info = ELFNN_R_INFO (h->dynindx, R_RISCV_JUMP_SLOT); > rela.r_addend = 0; > + bfd_vma rela_idx = htab->next_jump_slot_index++; > + loc = relplt->contents + rela_idx * sizeof > (ElfNN_External_Rela); > + bed->s->swap_reloca_out (output_bfd, &rela, loc); > } > > - loc = relplt->contents + plt_idx * sizeof (ElfNN_External_Rela); > - bed->s->swap_reloca_out (output_bfd, &rela, loc); > - > if (!h->def_regular) > { > /* Mark the symbol as undefined, rather than as defined in > @@ -3315,7 +3331,6 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > asection *sgot; > asection *srela; > Elf_Internal_Rela rela; > - bool use_elf_append_rela = true; > > /* This symbol has an entry in the GOT. Set it up. */ > > @@ -3338,10 +3353,6 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > /* Use .rela.iplt section to store .got relocations > in static executable. */ > srela = htab->elf.irelplt; > - > - /* Do not use riscv_elf_append_rela to add dynamic > - relocs. */ > - use_elf_append_rela = false; > } > > if (SYMBOL_REFERENCES_LOCAL (info, h)) > @@ -3417,23 +3428,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > bfd_put_NN (output_bfd, 0, > sgot->contents + (h->got.offset & ~(bfd_vma) 1)); > > - if (use_elf_append_rela) > - riscv_elf_append_rela (output_bfd, srela, &rela); > - else > - { > - /* Use riscv_elf_append_rela to add the dynamic relocs into > - .rela.iplt may cause the overwrite problems. Since we insert > - the relocs for PLT didn't handle the reloc_index of > .rela.iplt, > - but the riscv_elf_append_rela adds the relocs to the place > - that are calculated from the reloc_index (in seqential). > - > - One solution is that add these dynamic relocs (GOT IFUNC) > - from the last of .rela.iplt section. */ > - bfd_vma iplt_idx = htab->last_iplt_index--; > - bfd_byte *loc = srela->contents > - + iplt_idx * sizeof (ElfNN_External_Rela); > - bed->s->swap_reloca_out (output_bfd, &rela, loc); > - } > + riscv_elf_append_rela (output_bfd, srela, &rela); > } > > if (h->needs_copy) > diff --git a/ld/testsuite/ld-riscv-elf/ifunc-macro.s > b/ld/testsuite/ld-riscv-elf/ifunc-macro.s > new file mode 100644 > index 00000000000..88935a55814 > --- /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 > diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd > b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd > new file mode 100644 > index 00000000000..0de47a4009f > --- /dev/null > +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd > @@ -0,0 +1,4 @@ > +Relocation section '.rela.plt' at .* > +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* > diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd > b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd > new file mode 100644 > index 00000000000..d7ecd4a4a69 > --- /dev/null > +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd > @@ -0,0 +1,11 @@ > +Relocation section '.rela.got' at .* > +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_(32|64)[ ]+foo1\(\)[ > ]+foo1 \+ 0 > +#... > +Relocation section '.rela.ifunc' at .* > +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_(32|64)[ ]+foo2\(\)[ > ]+foo2 \+ 0 > +#... > +Relocation section '.rela.plt' at .* > +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+foo1\(\)[ > ]+foo1 \+ 0 > diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd > b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd > new file mode 100644 > index 00000000000..532cbf8a86a > --- /dev/null > +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd > @@ -0,0 +1,7 @@ > +Relocation section '.rela.ifunc' at .* > +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* > + > +Relocation section '.rela.plt' at .* > +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* > diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d > b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d > new file mode 100644 > index 00000000000..3e33ac619d4 > --- /dev/null > +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d > @@ -0,0 +1,16 @@ > +#... > +Disassembly of section .plt: > +#... > +0+[0-9a-f]+ <(\*ABS\*\+0x[0-9a-f]+@plt|foo@plt|.plt)>: > +#... > +Disassembly of section .text: > +#... > +0+[0-9a-f]+ <foo_resolver>: > +.*:[ ]+[0-9a-f]+[ ]+ret > + > +0+[0-9a-f]+ <_start>: > +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+.* > +.*:[ ]+[0-9a-f]+[ ]+(lw|ld)[ ]+.*<(.*)> > +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+.* > +.*:[ ]+[0-9a-f]+[ ]+jalr[ ]+.*<(.*plt.*)> > +.*:[ ]+[0-9a-f]+[ ]+ret > diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s > b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s > new file mode 100644 > index 00000000000..ff6d171591c > --- /dev/null > +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s > @@ -0,0 +1,34 @@ > +/* There are 2 ifuncs: foo1 and foo2. > + foo1 is accessed by a function call. > + foo2 is referenced by a global pointer (foo2_addr). */ > + .include "ifunc-macro.s" > + .globl foo2_addr > + .section .data > + .type foo2_addr, @object > +foo2_addr: > + PTR_DATA foo2 > + > + .text > + .type foo_resolver, @function > +foo_resolver: > + ret > + .size foo_resolver, .-foo_resolver > + > + .globl foo1 > + .type foo1, %gnu_indirect_function > + .set foo1, foo_resolver > + > + .globl foo2 > + .type foo2, %gnu_indirect_function > + .set foo2, foo_resolver > + > + .globl _start > + .type _start, @function > +_start: > +.L1: > + auipc x1, %got_pcrel_hi (foo1) > + LOAD x1, x1, %pcrel_lo (.L1) > + call foo1 > + > + ret > + .size _start, .-_start > diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp > b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp > index a1dd0e5e37e..10011445683 100644 > --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp > +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp > @@ -284,6 +284,13 @@ if [istarget "riscv*-*-*"] { > run_dump_test_ifunc "ifunc-plt-got-overwrite" rv64 pie > run_dump_test_ifunc "ifunc-plt-got-overwrite" rv64 pic > > + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 exe > + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 pie > + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 pic > + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 exe > + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 pie > + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 pic > + > # TODO: Make the following tests work under RV32. > if [istarget "riscv32-*-*"] { > return > diff --git a/ld/testsuite/ld-riscv-elf/variant_cc-now.d > b/ld/testsuite/ld-riscv-elf/variant_cc-now.d > index 9453554a159..3ed5ab3cae0 100644 > --- a/ld/testsuite/ld-riscv-elf/variant_cc-now.d > +++ b/ld/testsuite/ld-riscv-elf/variant_cc-now.d > @@ -7,11 +7,11 @@ Relocation section '.rela.plt' at .* > #... > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ > ]+nocc_global_default_undef \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ > ]+cc_global_default_undef \+ 0 > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ > ]+cc_global_default_def \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ > ]+nocc_global_default_def \+ 0 > -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 > -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 > diff --git a/ld/testsuite/ld-riscv-elf/variant_cc-shared.d > b/ld/testsuite/ld-riscv-elf/variant_cc-shared.d > index ffb69a392f2..6b73cf1de20 100644 > --- a/ld/testsuite/ld-riscv-elf/variant_cc-shared.d > +++ b/ld/testsuite/ld-riscv-elf/variant_cc-shared.d > @@ -7,11 +7,11 @@ Relocation section '.rela.plt' at .* > #... > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ > ]+nocc_global_default_undef \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ > ]+cc_global_default_undef \+ 0 > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 > +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ > ]+cc_global_default_def \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ > ]+nocc_global_default_def \+ 0 > -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 > -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ > ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 > [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 > -- > 2.37.1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-08 1:00 ` Nelson Chu @ 2024-05-16 6:16 ` Hau Hsu 2024-05-16 7:07 ` Nelson Chu 0 siblings, 1 reply; 11+ messages in thread From: Hau Hsu @ 2024-05-16 6:16 UTC (permalink / raw) To: Nelson Chu; +Cc: Binutils, Kito Cheng [-- Attachment #1: Type: text/plain, Size: 20432 bytes --] 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 > > Thanks > Nelson > > On Mon, May 6, 2024 at 12:46 PM Hau Hsu <hau.hsu@sifive.com <mailto:hau.hsu@sifive.com>> wrote: >> 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. >> 2. R_RISCV_IRELATIVE should come last. >> See https://sourceware.org/bugzilla/show_bug.cgi?id=13302 >> >> This patch fixes above issues with the implementation similar to x86. >> That is, by adding two variables to record the next relocation index for >> R_RISCV_IRELATIVE and R_RISCV_JUMP_SLOT. >> >> A previous commit partially fixed the overwrite issue: >> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=51a8a7c2e3cc0730831963651a55d23d1fae624d >> >> Details below: >> >> There are three ways to access a local (non-preemptible) ifunc: >> (1) Through PLT + GOT, i.e. use normal function call. >> (2) Through GOT, i.e. use a local function pointer. >> (3) Through a global function pointer. >> >> For (1) and (2), a R_RISCV_IRELATIVE is created for GOT entry. >> For (3), a R_RISCV_IRELATIVE is created for the global pointer. >> The relocations overwrite issue is that, the R_RISCV_IRELATIVE created >> for (1) overwrite the relocations for (2) and (3). >> The previous commit partially fixed the overwrite issue for (2), but not >> for (3). >> --- >> bfd/elfnn-riscv.c | 65 +++++++++---------- >> ld/testsuite/ld-riscv-elf/ifunc-macro.s | 19 ++++++ >> .../ifunc-plt-got-overwrite-02-exe.rd | 4 ++ >> .../ifunc-plt-got-overwrite-02-pic.rd | 11 ++++ >> .../ifunc-plt-got-overwrite-02-pie.rd | 7 ++ >> .../ld-riscv-elf/ifunc-plt-got-overwrite-02.d | 16 +++++ >> .../ld-riscv-elf/ifunc-plt-got-overwrite-02.s | 34 ++++++++++ >> ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 7 ++ >> ld/testsuite/ld-riscv-elf/variant_cc-now.d | 4 +- >> ld/testsuite/ld-riscv-elf/variant_cc-shared.d | 4 +- >> 10 files changed, 132 insertions(+), 39 deletions(-) >> create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-macro.s >> create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd >> create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd >> create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd >> create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d >> create mode 100644 ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s >> >> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c >> index 3a30b7a4bd9..d9243648d24 100644 >> --- a/bfd/elfnn-riscv.c >> +++ b/bfd/elfnn-riscv.c >> @@ -224,8 +224,12 @@ struct riscv_elf_link_hash_table >> htab_t loc_hash_table; >> void * loc_hash_memory; >> >> - /* The index of the last unused .rel.iplt slot. */ >> - bfd_vma last_iplt_index; >> + /* The index of the next R_RISCV_JUMP_SLOT entry in .rela.plt. */ >> + bfd_vma next_jump_slot_index; >> + >> + /* R_RISCV_IRELATIVE entry in .rela.plt comes last. >> + Use this to record the index of the next R_RISCV_IRELATIVE entry. */ >> + bfd_vma next_irelative_index; >> >> /* The data segment phase, don't relax the section >> when it is exp_seg_relro_adjust. */ >> @@ -1273,6 +1277,7 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, void *inf) >> >> /* We also need to make an entry in the .rela.plt section. */ >> htab->elf.srelplt->size += sizeof (ElfNN_External_Rela); >> + htab->elf.srelplt->reloc_count ++; >> >> /* If this symbol is not defined in a regular file, and we are >> not generating a shared library, then set the symbol to this >> @@ -1622,10 +1627,14 @@ riscv_elf_late_size_sections (bfd *output_bfd, struct bfd_link_info *info) >> local ifunc symbols. */ >> htab_traverse (htab->loc_hash_table, allocate_local_ifunc_dynrelocs, info); >> >> - /* Used to resolve the dynamic relocs overwite problems when >> - generating static executable. */ >> - if (htab->elf.irelplt) >> - htab->last_iplt_index = htab->elf.irelplt->reloc_count - 1; >> + >> + htab->next_jump_slot_index = 0; >> + >> + if (htab->elf.srelplt) >> + htab->next_irelative_index = htab->elf.srelplt->reloc_count - 1; >> + else if (htab->elf.irelplt) >> + htab->next_irelative_index = htab->elf.irelplt->reloc_count - 1; >> + >> >> if (htab->elf.sgotplt) >> { >> @@ -3201,15 +3210,14 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> >> if (h->plt.offset != (bfd_vma) -1) >> { >> - /* We've decided to create a PLT entry for this symbol. */ >> + /* We've decided to create a PLT entry for this symbol. >> + Add a PLT entry, a GOT entry and a relocation entry. */ >> bfd_byte *loc; >> bfd_vma i, header_address, plt_idx, got_offset, got_address; >> uint32_t plt_entry[PLT_ENTRY_INSNS]; >> Elf_Internal_Rela rela; >> asection *plt, *gotplt, *relplt; >> >> - /* When building a static executable, use .iplt, .igot.plt and >> - .rela.iplt sections for STT_GNU_IFUNC symbols. */ >> if (htab->elf.splt != NULL) >> { >> plt = htab->elf.splt; >> @@ -3218,6 +3226,8 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> } >> else >> { >> + /* When building a static executable, use .iplt, .igot.plt and >> + .rela.iplt sections for STT_GNU_IFUNC symbols. */ >> plt = htab->elf.iplt; >> gotplt = htab->elf.igotplt; >> relplt = htab->elf.irelplt; >> @@ -3238,7 +3248,8 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> header_address = sec_addr (plt); >> >> /* Calculate the index of the entry and the offset of .got.plt entry. >> - For static executables, we don't reserve anything. */ >> + The index of .got.plt is the same as .plt (exclude the header entries), >> + since this section is dedicated for plt. */ >> if (plt == htab->elf.splt) >> { >> plt_idx = (h->plt.offset - PLT_HEADER_SIZE) / PLT_ENTRY_SIZE; >> @@ -3246,6 +3257,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> } >> else >> { >> + /* For static executables, we don't need to reserve the header entry. */ >> plt_idx = h->plt.offset / PLT_ENTRY_SIZE; >> got_offset = plt_idx * GOT_ENTRY_SIZE; >> } >> @@ -3269,6 +3281,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> loc = gotplt->contents + (got_address - sec_addr (gotplt)); >> bfd_put_NN (output_bfd, sec_addr (plt), loc); >> >> + /* Fill in the relocation entry. */ >> rela.r_offset = got_address; >> if (PLT_LOCAL_IFUNC_P (info, h)) >> { >> @@ -3283,17 +3296,20 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> rela.r_addend = h->root.u.def.value >> + sec->output_section->vma >> + sec->output_offset; >> + bfd_vma rela_idx = htab->next_irelative_index--; >> + loc = relplt->contents + rela_idx * sizeof (ElfNN_External_Rela); >> + bed->s->swap_reloca_out (output_bfd, &rela, loc); >> } >> else >> { >> /* Fill in the entry in the .rela.plt section. */ >> rela.r_info = ELFNN_R_INFO (h->dynindx, R_RISCV_JUMP_SLOT); >> rela.r_addend = 0; >> + bfd_vma rela_idx = htab->next_jump_slot_index++; >> + loc = relplt->contents + rela_idx * sizeof (ElfNN_External_Rela); >> + bed->s->swap_reloca_out (output_bfd, &rela, loc); >> } >> >> - loc = relplt->contents + plt_idx * sizeof (ElfNN_External_Rela); >> - bed->s->swap_reloca_out (output_bfd, &rela, loc); >> - >> if (!h->def_regular) >> { >> /* Mark the symbol as undefined, rather than as defined in >> @@ -3315,7 +3331,6 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> asection *sgot; >> asection *srela; >> Elf_Internal_Rela rela; >> - bool use_elf_append_rela = true; >> >> /* This symbol has an entry in the GOT. Set it up. */ >> >> @@ -3338,10 +3353,6 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> /* Use .rela.iplt section to store .got relocations >> in static executable. */ >> srela = htab->elf.irelplt; >> - >> - /* Do not use riscv_elf_append_rela to add dynamic >> - relocs. */ >> - use_elf_append_rela = false; >> } >> >> if (SYMBOL_REFERENCES_LOCAL (info, h)) >> @@ -3417,23 +3428,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, >> bfd_put_NN (output_bfd, 0, >> sgot->contents + (h->got.offset & ~(bfd_vma) 1)); >> >> - if (use_elf_append_rela) >> - riscv_elf_append_rela (output_bfd, srela, &rela); >> - else >> - { >> - /* Use riscv_elf_append_rela to add the dynamic relocs into >> - .rela.iplt may cause the overwrite problems. Since we insert >> - the relocs for PLT didn't handle the reloc_index of .rela.iplt, >> - but the riscv_elf_append_rela adds the relocs to the place >> - that are calculated from the reloc_index (in seqential). >> - >> - One solution is that add these dynamic relocs (GOT IFUNC) >> - from the last of .rela.iplt section. */ >> - bfd_vma iplt_idx = htab->last_iplt_index--; >> - bfd_byte *loc = srela->contents >> - + iplt_idx * sizeof (ElfNN_External_Rela); >> - bed->s->swap_reloca_out (output_bfd, &rela, loc); >> - } >> + riscv_elf_append_rela (output_bfd, srela, &rela); >> } >> >> if (h->needs_copy) >> diff --git a/ld/testsuite/ld-riscv-elf/ifunc-macro.s b/ld/testsuite/ld-riscv-elf/ifunc-macro.s >> new file mode 100644 >> index 00000000000..88935a55814 >> --- /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 >> diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd >> new file mode 100644 >> index 00000000000..0de47a4009f >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-exe.rd >> @@ -0,0 +1,4 @@ >> +Relocation section '.rela.plt' at .* >> +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* >> diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd >> new file mode 100644 >> index 00000000000..d7ecd4a4a69 >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pic.rd >> @@ -0,0 +1,11 @@ >> +Relocation section '.rela.got' at .* >> +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_(32|64)[ ]+foo1\(\)[ ]+foo1 \+ 0 >> +#... >> +Relocation section '.rela.ifunc' at .* >> +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_(32|64)[ ]+foo2\(\)[ ]+foo2 \+ 0 >> +#... >> +Relocation section '.rela.plt' at .* >> +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+foo1\(\)[ ]+foo1 \+ 0 >> diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd >> new file mode 100644 >> index 00000000000..532cbf8a86a >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02-pie.rd >> @@ -0,0 +1,7 @@ >> +Relocation section '.rela.ifunc' at .* >> +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* >> + >> +Relocation section '.rela.plt' at .* >> +[ ]+Offset[ ]+Info[ ]+Type[ ]+.* >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+[0-9a-f]* >> diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d >> new file mode 100644 >> index 00000000000..3e33ac619d4 >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.d >> @@ -0,0 +1,16 @@ >> +#... >> +Disassembly of section .plt: >> +#... >> +0+[0-9a-f]+ <(\*ABS\*\+0x[0-9a-f]+@plt|foo@plt|.plt)>: >> +#... >> +Disassembly of section .text: >> +#... >> +0+[0-9a-f]+ <foo_resolver>: >> +.*:[ ]+[0-9a-f]+[ ]+ret >> + >> +0+[0-9a-f]+ <_start>: >> +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+.* >> +.*:[ ]+[0-9a-f]+[ ]+(lw|ld)[ ]+.*<(.*)> >> +.*:[ ]+[0-9a-f]+[ ]+auipc[ ]+.* >> +.*:[ ]+[0-9a-f]+[ ]+jalr[ ]+.*<(.*plt.*)> >> +.*:[ ]+[0-9a-f]+[ ]+ret >> diff --git a/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s >> new file mode 100644 >> index 00000000000..ff6d171591c >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/ifunc-plt-got-overwrite-02.s >> @@ -0,0 +1,34 @@ >> +/* There are 2 ifuncs: foo1 and foo2. >> + foo1 is accessed by a function call. >> + foo2 is referenced by a global pointer (foo2_addr). */ >> + .include "ifunc-macro.s" >> + .globl foo2_addr >> + .section .data >> + .type foo2_addr, @object >> +foo2_addr: >> + PTR_DATA foo2 >> + >> + .text >> + .type foo_resolver, @function >> +foo_resolver: >> + ret >> + .size foo_resolver, .-foo_resolver >> + >> + .globl foo1 >> + .type foo1, %gnu_indirect_function >> + .set foo1, foo_resolver >> + >> + .globl foo2 >> + .type foo2, %gnu_indirect_function >> + .set foo2, foo_resolver >> + >> + .globl _start >> + .type _start, @function >> +_start: >> +.L1: >> + auipc x1, %got_pcrel_hi (foo1) >> + LOAD x1, x1, %pcrel_lo (.L1) >> + call foo1 >> + >> + ret >> + .size _start, .-_start >> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp >> index a1dd0e5e37e..10011445683 100644 >> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp >> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp >> @@ -284,6 +284,13 @@ if [istarget "riscv*-*-*"] { >> run_dump_test_ifunc "ifunc-plt-got-overwrite" rv64 pie >> run_dump_test_ifunc "ifunc-plt-got-overwrite" rv64 pic >> >> + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 exe >> + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 pie >> + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv32 pic >> + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 exe >> + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 pie >> + run_dump_test_ifunc "ifunc-plt-got-overwrite-02" rv64 pic >> + >> # TODO: Make the following tests work under RV32. >> if [istarget "riscv32-*-*"] { >> return >> diff --git a/ld/testsuite/ld-riscv-elf/variant_cc-now.d b/ld/testsuite/ld-riscv-elf/variant_cc-now.d >> index 9453554a159..3ed5ab3cae0 100644 >> --- a/ld/testsuite/ld-riscv-elf/variant_cc-now.d >> +++ b/ld/testsuite/ld-riscv-elf/variant_cc-now.d >> @@ -7,11 +7,11 @@ Relocation section '.rela.plt' at .* >> #... >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+nocc_global_default_undef \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+cc_global_default_undef \+ 0 >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+cc_global_default_def \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+nocc_global_default_def \+ 0 >> -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 >> -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 >> diff --git a/ld/testsuite/ld-riscv-elf/variant_cc-shared.d b/ld/testsuite/ld-riscv-elf/variant_cc-shared.d >> index ffb69a392f2..6b73cf1de20 100644 >> --- a/ld/testsuite/ld-riscv-elf/variant_cc-shared.d >> +++ b/ld/testsuite/ld-riscv-elf/variant_cc-shared.d >> @@ -7,11 +7,11 @@ Relocation section '.rela.plt' at .* >> #... >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+nocc_global_default_undef \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+0000[ ]+cc_global_default_undef \+ 0 >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 >> +[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+cc_global_default_def \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+0+8000[ ]+nocc_global_default_def \+ 0 >> -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+cc_global_default_ifunc\(\)[ ]+cc_global_default_ifunc \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 >> -[0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_JUMP_SLOT[ ]+nocc_global_default_ifunc\(\)[ ]+nocc_global_default_ifunc \+ 0 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8000 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 >> [0-9a-f]+[ ]+[0-9a-f]+[ ]+R_RISCV_IRELATIVE[ ]+8050 >> -- >> 2.37.1 >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-16 6:16 ` Hau Hsu @ 2024-05-16 7:07 ` Nelson Chu 2024-05-16 7:30 ` Hau Hsu 0 siblings, 1 reply; 11+ messages in thread From: Nelson Chu @ 2024-05-16 7:07 UTC (permalink / raw) To: Hau Hsu; +Cc: Binutils, Kito Cheng [-- Attachment #1: Type: text/plain, Size: 2312 bytes --] 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... > --- /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? Nelson ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-16 7:07 ` Nelson Chu @ 2024-05-16 7:30 ` Hau Hsu 2024-05-16 8:28 ` Nelson Chu 0 siblings, 1 reply; 11+ messages in thread From: Hau Hsu @ 2024-05-16 7:30 UTC (permalink / raw) To: Nelson Chu; +Cc: Binutils, Kito Cheng [-- Attachment #1: Type: text/plain, Size: 3389 bytes --] > 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 <mailto: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 <mailto: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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-16 7:30 ` Hau Hsu @ 2024-05-16 8:28 ` Nelson Chu 2024-05-17 2:32 ` Hau Hsu 0 siblings, 1 reply; 11+ messages in thread From: Nelson Chu @ 2024-05-16 8:28 UTC (permalink / raw) To: Hau Hsu; +Cc: Binutils, Kito Cheng [-- 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 > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-16 8:28 ` Nelson Chu @ 2024-05-17 2:32 ` Hau Hsu 2024-05-23 3:53 ` Hau Hsu 0 siblings, 1 reply; 11+ messages in thread From: Hau Hsu @ 2024-05-17 2:32 UTC (permalink / raw) To: Nelson Chu; +Cc: Binutils, Kito Cheng [-- Attachment #1: Type: text/plain, Size: 5957 bytes --] Let me run more toolchain integrated tests. Thanks! Hau > On May 16, 2024, at 4:28 PM, Nelson Chu <nelson@rivosinc.com> wrote: > > 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 <mailto:hau.hsu@sifive.com>> wrote: >> >>> On May 16, 2024, at 3:07 PM, Nelson Chu <nelson@rivosinc.com <mailto:nelson@rivosinc.com>> wrote: >>> >>> On Thu, May 16, 2024 at 2:16 PM Hau Hsu <hau.hsu@sifive.com <mailto: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 <mailto: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 >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-17 2:32 ` Hau Hsu @ 2024-05-23 3:53 ` Hau Hsu 2024-05-23 6:43 ` Nelson Chu 0 siblings, 1 reply; 11+ messages in thread From: Hau Hsu @ 2024-05-23 3:53 UTC (permalink / raw) To: Nelson Chu; +Cc: Binutils, Kito Cheng [-- Attachment #1: Type: text/plain, Size: 6914 bytes --] I used riscv-gnu-toolchain to run gcc test suite (linux) and got no regressions: ========= Summary of gcc testsuite ========= | # of unexpected case / # of unique unexpected case | gcc | g++ | gfortran | rv64imafdc/ lp64d/ medlow | 0 / 0 | 0 / 0 | 0 / 0 | >> 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. It seems my change works. But I can try to modify code as suggested. Hau Hsu > On May 17, 2024, at 10:32 AM, Hau Hsu <hau.hsu@sifive.com> wrote: > > Let me run more toolchain integrated tests. > Thanks! > > Hau > > > >> On May 16, 2024, at 4:28 PM, Nelson Chu <nelson@rivosinc.com> wrote: >> >> 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 <mailto:hau.hsu@sifive.com>> wrote: >>> >>>> On May 16, 2024, at 3:07 PM, Nelson Chu <nelson@rivosinc.com <mailto:nelson@rivosinc.com>> wrote: >>>> >>>> On Thu, May 16, 2024 at 2:16 PM Hau Hsu <hau.hsu@sifive.com <mailto: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 <mailto: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 >>> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues 2024-05-23 3:53 ` Hau Hsu @ 2024-05-23 6:43 ` Nelson Chu 0 siblings, 0 replies; 11+ messages in thread From: Nelson Chu @ 2024-05-23 6:43 UTC (permalink / raw) To: Hau Hsu; +Cc: Binutils, Kito Cheng [-- Attachment #1: Type: text/plain, Size: 6939 bytes --] Your code will break what we discussed in the pr13302 - "riscv inserts the dynamic relocations in the order of plt and got entry offset" On Thu, May 23, 2024 at 11:54 AM Hau Hsu <hau.hsu@sifive.com> wrote: > I used riscv-gnu-toolchain to run gcc test suite (linux) and got no > regressions: > > ========= Summary of gcc testsuite ========= > | # of unexpected case / # of unique > unexpected case > | gcc | g++ | gfortran | > rv64imafdc/ lp64d/ medlow | 0 / 0 | 0 / 0 | 0 / 0 | > > > 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. > > > It seems my change works. But I can try to modify code as suggested. > > > Hau Hsu > > > > > On May 17, 2024, at 10:32 AM, Hau Hsu <hau.hsu@sifive.com> wrote: > > Let me run more toolchain integrated tests. > Thanks! > > Hau > > > > On May 16, 2024, at 4:28 PM, Nelson Chu <nelson@rivosinc.com> wrote: > > 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 >> >> >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P 2024-05-06 4:45 [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P Hau Hsu 2024-05-06 4:45 ` [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues Hau Hsu @ 2024-05-08 1:03 ` Nelson Chu 1 sibling, 0 replies; 11+ messages in thread From: Nelson Chu @ 2024-05-08 1:03 UTC (permalink / raw) To: Hau Hsu; +Cc: binutils, kito.cheng [-- Attachment #1: Type: text/plain, Size: 2754 bytes --] I think it's fine to keep the old code since it's the same as aarch64 code. Nelson On Mon, May 6, 2024 at 12:46 PM Hau Hsu <hau.hsu@sifive.com> wrote: > Add POINTER_LOCAL_IFUNC_P which returns TRUE for pointer reference to > local IFUNC symbol. > Add PLT_LOCAL_IFUNC_P which returns TRUE for PLT reference to local > IFUNC symbol. > > Copied from x86 commit: cf1070f1a1ca1f8be1cd88aa6ece55a25e6a887b > --- > bfd/elfnn-riscv.c | 11 ++--------- > bfd/elfxx-riscv.h | 14 ++++++++++++++ > 2 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c > index 95abf2d10e2..3a30b7a4bd9 100644 > --- a/bfd/elfnn-riscv.c > +++ b/bfd/elfnn-riscv.c > @@ -2373,9 +2373,7 @@ riscv_elf_relocate_section (bfd *output_bfd, > outrel.r_offset += input_section->output_section->vma > + input_section->output_offset; > > - if (h->dynindx == -1 > - || h->forced_local > - || bfd_link_executable (info)) > + if (POINTER_LOCAL_IFUNC_P (info, h)) > { > info->callbacks->minfo > (_("Local IFUNC function `%s' in %pB\n"), > @@ -3272,12 +3270,7 @@ riscv_elf_finish_dynamic_symbol (bfd *output_bfd, > bfd_put_NN (output_bfd, sec_addr (plt), loc); > > rela.r_offset = got_address; > - > - if (h->dynindx == -1 > - || ((bfd_link_executable (info) > - || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT) > - && h->def_regular > - && h->type == STT_GNU_IFUNC)) > + if (PLT_LOCAL_IFUNC_P (info, h)) > { > info->callbacks->minfo (_("Local IFUNC function `%s' in %pB\n"), > h->root.root.string, > diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h > index 49be71746b9..9efac27864c 100644 > --- a/bfd/elfxx-riscv.h > +++ b/bfd/elfxx-riscv.h > @@ -27,6 +27,20 @@ > > #define RISCV_UNKNOWN_VERSION -1 > > +/* TRUE if this is a pointer reference to a local IFUNC. */ > +#define POINTER_LOCAL_IFUNC_P(INFO, H) \ > + ((H)->dynindx == -1 \ > + || (H)->forced_local \ > + || bfd_link_executable (INFO)) > + > +/* TRUE if this is a PLT reference to a local IFUNC. */ > +#define PLT_LOCAL_IFUNC_P(INFO, H) \ > + ((H)->dynindx == -1 \ > + || ((bfd_link_executable (INFO) \ > + || ELF_ST_VISIBILITY ((H)->other) != STV_DEFAULT) \ > + && (H)->def_regular \ > + && (H)->type == STT_GNU_IFUNC)) > + > struct riscv_elf_params > { > /* Whether to relax code sequences to GP-relative addressing. */ > -- > 2.37.1 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-05-23 6:43 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-06 4:45 [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P Hau Hsu 2024-05-06 4:45 ` [PATCH 2/2] riscv: Fix R_RISCV_IRELATIVE overwrite and order issues Hau Hsu 2024-05-08 1:00 ` Nelson Chu 2024-05-16 6:16 ` Hau Hsu 2024-05-16 7:07 ` Nelson Chu 2024-05-16 7:30 ` Hau Hsu 2024-05-16 8:28 ` Nelson Chu 2024-05-17 2:32 ` Hau Hsu 2024-05-23 3:53 ` Hau Hsu 2024-05-23 6:43 ` Nelson Chu 2024-05-08 1:03 ` [PATCH 1/2] riscv: Add POINTER_LOCAL_IFUNC_P/PLT_LOCAL_IFUNC_P Nelson Chu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).