Repository navigation
cranelift: handle interrupt_poll in inliner and emit stack map at faulting load PC - #14947
Shoryamishra61 wants to merge 2 commits into
Conversation
…lting load PC - Treat interrupt_poll as a call-like non-inlined instruction in the inliner. - Inhibit inlining of callees with interrupt_poll into non-tail callers. - Fixup exception tables only for actual callable targets, skipping interrupt_poll. - Mark InterruptPoll as a safepoint on x64 and aarch64. - Emit user stack maps at the faulting load PC. - Insert architecture-specific nop when consecutive call and poll stack maps share the same PC. - Generalize write.rs to display user stack maps for all safepoints. Fixes bytecodealliance#14926
github-actions
Bot
added
cranelift
| /// `interrupt_poll` is only valid in functions that use the `tail` calling | ||
| /// convention (see the verifier), so inlining a callee that contains one into a | ||
| /// function with any other convention would produce invalid CLIF. | ||
| fn can_inline(caller: &ir::Function, callee: &ir::Function) -> bool { |
There was a problem hiding this comment.
IIRC we've already got inlining heuristics elsewhere in Wasmtime which would cover things like this. Subjectively I'd say it's ok to drop this and leave this up to callers, and if the heuristics from Wasmtime are ever extracted we could perhaps consolidate with this.
| // `interrupt_poll` is classified as `is_call()` because it | ||
| // "continues execution elsewhere" (see its definition), but | ||
| // it is not an inlineable function call — it has no `FuncRef` | ||
| // and no external callee body. When a function containing a | ||
| // poll is inlined by `inline_one`, the instruction is cloned | ||
| // into the caller verbatim by the generic instruction-copying | ||
| // loop; no special action is needed here. |
There was a problem hiding this comment.
It's ok to trim down verbose LLM comments like this. Basically this just needs to convey "InterruptPoll returns is_call(), but it doesn't participate in inlining"
| // in release builds where the buffer's ordering assertion is | ||
| // compiled out. | ||
| if stack_map.is_some() && sink.last_user_stack_map_pc() == Some(sink.cur_offset()) { | ||
| sink.put4(0xd503201f); |
There was a problem hiding this comment.
IIRC we've already got infrastructure in other places for whether or not metadata applies to after an instruction or that instruction itself (or something like that). For example I think MachDebugTagPos serves that purpose. Personally I think it would be better to improve stack map lookup to handle that as opposed to injecting otherwise-not-useful nops.
My intuition here is that this is probably the wrong decision, in hindsight: yes it can "do a call" in a sense, if a signal handler runs, but that's completely transparent. The "can call" opcode characteristic is really more relevant for things like inlining, as we see here, and in most ways the opcode is not a call (with args, with a callee, etc). So we should probably flip the condition here and then work through the implications, IMHO. (I will take a more detailed look next week when I am fully back at the keyboard; I just wanted to leave this thought now as the discussion is open) |
|
Looking at this a little more, it remains my opinion that (Also cc @erikrose) |
|
Yes, we should hold loosely my choice to put |
|
@erikrose would you mind taking on the fix here, since the original creator of this PR hasn't responded for a week? |
|
Apologies for the delayed response, @cfallin and @alexcrichton . I've been occupied with other commitments over the past week and wasn't able to follow up on the review discussions. I'm still interested in seeing this through and addressing the feedback. I understand the concern about treating If the work hasn't already been handed over, I'd appreciate the opportunity to make these revisions. If @erikrose has already started, I'm happy to coordinate to avoid duplicating effort. Thanks for the detailed feedback and your patience. |
|
Ah, no worries @Shoryamishra61, I just wanted to make sure things were moving forward and didn't want to put any pressure on an external contributor (you don't owe us anything!) -- if you want to keep going on this, please do. I guess if you feel like you need to hand it off at any point, it would make sense to pass off to Erik. Either way we (as Cranelift maintainers) do want to see it resolved before declaring the MMU-interruption work done, since this is a bug, but that's not on you to ensure :-) |
|
Pushed commit \920636a214\ addressing @cfallin's review:
|
github-actions
Bot
added
the
cranelift:meta
Fixes #14926
Why this change is needed
When inlining functions that contain
interrupt_pollor compiling functions where aninterrupt_pollpossesses user stack maps, Cranelift encountered several issues:interrupt_pollis a safepoint at which stack maps must be emitted at the faulting load instruction PC for signal handler inspection.interrupt_polldoes not have a callee, arguments, or calling convention (signal handlers are transparent to control flow). It is therefore classified without.call(), but retains safepoint handling (is_safepoint()).interrupt_pollis not classified as a call, the inliner copies it directly in the instruction-copying loop, and stack map entries are cleanly copied usingopcode.is_safepoint().InterruptPollas a safepoint (is_safepoint()), emitting user stack maps at the address of the faulting load without artificial NOP padding.Implementation Highlights
cranelift/codegen/meta/src/shared/instructions.rs):.call()classification frominterrupt_poll.cranelift/codegen/src/inline.rs):opcode.is_safepoint()for copying user stack map entries, while reserving call/try_call exception table fixups strictly for actual call instructions.cranelift/codegen/src/isa/{x64, aarch64}/inst/):is_safepoint()returnstrueforInst::InterruptPoll.cranelift/filetests/):AI Tool Usage Disclosure
In compliance with the Bytecode Alliance AI Tool Use Policy: An AI assistant was used locally to assist with code inspection, running formatting/syntax checks, and drafting this summary. All code changes were reviewed and verified.