Thanks for the review! Will send a v2 to fix the attribute handling and the missing length attributes.
For the indirect calls to returns_twice / indirect_return. I looked into how aarch64 does it, and it passes fntype through CUMULATIVE_ARGS and reads it back in a late RTL pass. That's a bit more work to get right for RISC-V, I'd like to get this patch in first and address it in a separate follow-up patch. On Thu, Jun 11, 2026 at 8:01 AM Jeffrey Law <[email protected]> wrote: > A few things stand out. > > ### 1. `indirect_return` attribute handling looks wrong/incomplete > You register it as: > > ```c > {"indirect_return", 0, 0, false, true, true, true, NULL, NULL}, > ``` > > and then check it via: > > ```c > tree fntype = TREE_TYPE (decl); > lookup_attribute ("indirect_return", TYPE_ATTRIBUTES (fntype)) > ``` > > That means this is treated as a **type** attribute, not a decl > attribute. That may be OK if intentional, but then: > > - is that really the desired user model? > - does it propagate correctly through function pointer types, > redeclarations, builtins, etc? > - should calls through function pointers also be covered? > > > > > > On 6/9/2026 9:55 PM, Monk Chiang wrote: > > Add call-site LPAD insertion for two cases: > > > > 1. setjmp / __attribute__((returns_twice)) calls, which may return a > > second time via longjmp. > > 2. A new "indirect_return" attribute for functions that may return to > > an unexpected address. > > > > Detection uses riscv_call_needs_lpad_p() at expand time. When needed, > > call_internal_cfi / call_value_internal_cfi emit .p2align 2, > > .option push/norelax/norvc, the call, .option pop, and lpad 0 as a > > single insn. This prevents the assembler (c.jal) or linker (jal > > relaxation) from shifting the return address off the lpad. > > > > gcc/ChangeLog: > > > > * config/riscv/riscv-protos.h (riscv_call_needs_lpad_p): Declare. > > * config/riscv/riscv.cc (riscv_gnu_attributes): Register new > > indirect_return attribute for function types. > > (riscv_call_needs_lpad_p): New function. > > * config/riscv/riscv.md (call_internal_cfi): New insn pattern. > > (call_value_internal_cfi): Likewise for call-with-return-value. > > (define_expand "call"): Emit call_internal_cfi when > > riscv_call_needs_lpad_p returns true. > > (define_expand "call_value"): Likewise. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.target/riscv/zicfilp-indirect-return.c: New test. > > * gcc.target/riscv/zicfilp-setjmp.c: New test. > > > > > > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md > > index 2055e5c4a9d..cb0153ab0af 100644 > > --- a/gcc/config/riscv/riscv.md > > +++ b/gcc/config/riscv/riscv.md > > > > @@ -4206,6 +4210,36 @@ > > call\t%0@plt" > > [(set_attr "type" "call")]) > > > > +;; Zicfilp-protected call: .option push/pop guards prevent c.jal > compression > > +;; and jal linker relaxation from moving the return address off the > lpad. > > +;; .p2align 2 ensures the lpad is 4-byte aligned. > > +(define_insn "call_internal_cfi" > > + [(call (mem:SI (match_operand 0 "call_insn_operand" "l,S,U")) > > + (match_operand 1 "" "")) > > + (clobber (reg:SI RETURN_ADDR_REGNUM))] > > + "TARGET_ZICFILP" > > + { > > + output_asm_insn (".p2align\t2", operands); > > + output_asm_insn (".option push", operands); > > + output_asm_insn (".option norelax", operands); > > + output_asm_insn (".option norvc", operands); > > + switch (which_alternative) > > + { > > + case 0: > > + output_asm_insn ("jalr\t%0", operands); > > + break; > > + case 1: > > + output_asm_insn ("call\t%0", operands); > > + break; > > + default: > > + output_asm_insn ("call\t%0@plt", operands); > > + break; > > + } > > + output_asm_insn (".option pop", operands); > > + return "lpad\t0"; > > + } > > + [(set_attr "type" "call")]) > Given this outputs multiple instructions I think you need an attribute > specifying the length (in bytes) of the sequence. > > Thanks for putting a comment on this stuff; one of the things that I've > found problematical with the kcfi work from Kees & friends is they > haven't ever explained why they need the call sequence to be output as > an atomic blob. Your comment makes it clear. > > > > > > @@ -4230,6 +4269,34 @@ > > call\t%1@plt" > > [(set_attr "type" "call")]) > > > > +(define_insn "call_value_internal_cfi" > > + [(set (match_operand 0 "" "") > > + (call (mem:SI (match_operand 1 "call_insn_operand" "l,S,U")) > > + (match_operand 2 "" ""))) > > + (clobber (reg:SI RETURN_ADDR_REGNUM))] > > + "TARGET_ZICFILP" > You should likely add the same comment to this define_insn. This also > needs a length attribute. > > At a high level, if we have a pointer to a function and the signature > includes the returns-twice type attribute, it would seem like it won't > be handled properly. Of course there's probably other things that break > if we call a returns-twice function indirectly. Mostly I wanted to > confirm that you didn't really expect/want to cover that scenario? > > Generally OK. Mostly want to get the length attributes added, comment > copied and probably just a "yea, not handling indirect calls to returns > twice functions" and we're good to go. > > jeff >
