Harden xtensa call_primitive_last and align encode_rrr with the ISA

[pguyot]

Add an explicit error clause for the currently unreachable non-jit_state argument shape. Rename encode_rrr's op1/op2 parameters and comments to match the ISA field order; emitted bytes are unchanged.

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

[petermm]

AMP, ever pedantic - pick and choose:

PR Review: 4e42b57c3 — Harden Xtensa tail calls and align RRR naming

Verdict: changes requested

The RRR parameter rename is byte-for-byte neutral, and all current production callers use the supported [..., jit_state, ...] shape. However, the new diagnostic does not reliably satisfy the commit's hardening goal because unsupported arguments are validated only after scratch-register allocation.

Findings

[P2] Validate the argument shape before scratch-register allocation

Location: libs/jit/src/jit_xtensa.erl:668-726

call_primitive_last/3 reaches the new catch-all only after it computes ScratchMask and calls first_avail/1. first_avail/1 has no clause for a zero mask. An unsupported but type-valid argument list can consume all parameter and scratch candidates first:

Args = [ctx, a9, a7, a6, a5, 0]

For these six arguments, ParamMask reserves a10..a15, while ArgsRegsMask reserves a9, a7, a6, and a5. ScratchMask is therefore zero, and the function raises an opaque function_clause from first_avail(0) instead of the intended {unsupported_call_primitive_last_args, Args} error.

This was reproduced against the commit's compiled BEAM:

error:function_clause
[{jit_xtensa,first_avail,[0],...},
 {jit_xtensa,call_primitive_last,3,...}]

Validate in the function head before allocation or stream writes. This also reports the caller's original arguments rather than Args1, in which an offset argument may already have been replaced with a numeric stream offset.

 call_primitive_last(
     #state{
         stream_module = StreamModule,
         stream = Stream0
     } = State0,
     Primitive,
-    Args
+    [_, jit_state | _] = Args
 ) ->
     ...
     case Args1 of
         [FirstArg, jit_state | ArgsT] ->
             ...
             State2#state{
                 stream = Stream3,
                 available_regs = ?AVAILABLE_REGS_MASK,
                 used_regs = 0,
                 regs = jit_regs:unreachable(State2#state.regs)
-            };
-        _ ->
-            %% The windowed-ABI tail-call sequence above rewrites the second
-            %% argument (jit_state -> jit_state_tail_call), so it only supports
-            %% calls whose second argument is jit_state. Every current caller,
-            %% including call_primitive_with_cp for call_ext/call_fun/apply,
-            %% satisfies this; fail loudly here rather than with an opaque
-            %% case_clause should a future caller not.
-            error({unsupported_call_primitive_last_args, Args1})
-    end.
+            }
+    end;
+%% The windowed-ABI tail-call sequence rewrites the second argument
+%% (jit_state -> jit_state_tail_call), so reject every other shape before
+%% allocating registers or appending to the stream.
+call_primitive_last(_State, _Primitive, Args) ->
+    error({unsupported_call_primitive_last_args, Args}).

Add a regression test using the scratch-exhausting input, rather than a short malformed list that would happen to reach the current late catch-all:

 call_primitive_last_test() ->
     ...
     ?assertStream(xtensa, Dump, Stream).
+
+call_primitive_last_unsupported_args_test() ->
+    State = ?BACKEND:new(?JIT_VARIANT_PIC, jit_stream_binary, jit_stream_binary:new(0)),
+    Args = [ctx, a9, a7, a6, a5, 0],
+    ?assertError(
+        {unsupported_call_primitive_last_args, Args},
+        ?BACKEND:call_primitive_last(State, 0, Args)
+    ).

[P3] Use the ISA field name consistently in the SLLI comment

Location: libs/jit/src/jit_xtensa_asm.erl:253-256

The commit consistently renames bits [19:16] to op1 in the general RRR format and the encode_rrr/6 parameters, but the nearby SLLI field description still calls it only op. This is documentation-only and non-blocking, but adjusting it completes the stated naming cleanup.

-%% The encoded value split: sa_enc[4] at bits[23:20], op=1 at bits[19:16],
+%% The encoded value split: sa_enc[4] at bits[23:20], op1=1 at bits[19:16],

Verification

• Confirmed all encode_rrr/6 call arguments and emitted-byte expressions are unchanged from the parent commit; only field names/comments changed.
• Confirmed all current production call_primitive_last/3 callers pass jit_state as the second argument.
• Ran eunit:test(jit_xtensa_tests, [verbose]): 144 tests passed.
• Reproduced the P2 failure directly with the scratch-exhausting unsupported argument list above.

[petermm]

PR Review: 61aab805e — Harden Xtensa tail calls and align RRR naming

Verdict: approved

No unresolved P0–P3 findings. The force-pushed commit correctly addresses both findings from the review of obsolete commit 4e42b57c3, without changing supported code generation.

Resolved findings

P2 resolved: unsupported arguments are rejected before allocation

call_primitive_last/3 now matches [_, jit_state | _] in the supported function head. Every other shape reaches the fallback clause before parameter-mask calculation, first_avail/1, argument rewriting, or stream access:

call_primitive_last(_State, _Primitive, Args) ->
    error({unsupported_call_primitive_last_args, Args}).

This preserves the caller's original Args in the diagnostic. The new regression test uses [ctx, a9, a7, a6, a5, 0], which genuinely exhausted every available register under the old ordering: six parameters reserve a10..a15, while the explicit source registers reserve a9, a7, a6, and a5.

Supported callers and their generated instruction streams are unchanged.

P3 resolved: SLLI uses the ISA field name

The SLLI field description now identifies bits [19:16] as op1, consistently with the general RRR layout and encode_rrr/6 parameter names.

Verification

• Oracle follow-up review: merge, with no unresolved P0–P3 findings.
• git diff --check HEAD^ HEAD: passed.
• eunit:test(jit_xtensa_asm_tests, [verbose]): 73 tests passed.
• eunit:test(jit_xtensa_tests, [verbose]): 145 tests passed, including call_primitive_last_unsupported_args_test/0.

The first codegen test invocation loaded a stale pre-force-push jit_xtensa.beam and failed the new regression test with the old first_avail/1 behavior. Rebuilding the jit_emu target recompiled jit_xtensa.erl; the complete 145-test suite then passed. This was a local incremental-build artifact, not a source defect.