Skip to content

Commit d066709

Browse files
pguyotpguyot
committed
Fix JIT bs_get_binary2 with compile-time integer size
In OP_BS_GET_BINARY2, `Size` from decode_compact_term is a tagged term integer ((N bsl 4) bor 0xF). The code used `Size bsl 4` which shifts the already-tagged value further left, producing a garbage value. The correct operation is `Size bsr 4` to untag and get the raw byte count, matching what the non-JIT interpreter does with term_to_int(). This bug caused bs_get_binary2 with a compile-time literal size to always fail the bounds check, making patterns like <<Head:N/binary, Rest/binary>> (with literal N) unmatchable. Also add a test for the atom all case using assembly and fix subq on x86_64 with large integers following what other backends do. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
1 parent 6f66fa4 commit d066709

7 files changed

Lines changed: 214 additions & 4 deletions

File tree

‎libs/jit/src/jit.erl‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1348,10 +1348,12 @@ first_pass(<<?OP_BS_GET_BINARY2, Rest0/binary>>, MMod, MSt0, State0) ->
13481348
{MSt11, SizeReg};
13491349
is_integer(Size) ->
13501350
% SizeReg is binary size
1351-
% SizeVal is a constant
1352-
MSt11 = MMod:sub(MSt10, SizeReg, Size bsl 4),
1351+
% Size is a tagged integer: (N bsl 4) bor 0xF
1352+
% SizeBytes is the raw byte count
1353+
SizeBytes = Size bsr 4,
1354+
MSt11 = MMod:sub(MSt10, SizeReg, SizeBytes),
13531355
MSt12 = cond_jump_to_label({{free, SizeReg}, '<', BSOffsetReg1}, Fail, MMod, MSt11),
1354-
{MSt12, Size bsl 4};
1356+
{MSt12, SizeBytes};
13551357
true ->
13561358
{MSt11, SizeValReg} = MMod:move_to_native_register(MSt10, Size),
13571359
MSt12 = MMod:if_else_block(

‎libs/jit/src/jit_x86_64_asm.erl‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,16 @@ addq(SrcReg, DestReg) when is_atom(SrcReg), is_atom(DestReg) ->
483483
{REX_B, MODRM_RM} = x86_64_x_reg(DestReg),
484484
<<?X86_64_REX(1, REX_R, 0, REX_B), 16#01, 3:2, MODRM_REG:3, MODRM_RM:3>>.
485485

486+
subq(Imm, Reg) when ?IS_SINT8_T(Imm), is_atom(Reg) ->
487+
case x86_64_x_reg(Reg) of
488+
{0, Index} -> <<16#48, 16#83, (16#E8 + Index), Imm>>;
489+
{1, Index} -> <<16#49, 16#83, (16#E8 + Index), Imm>>
490+
end;
491+
subq(Imm, rax) when ?IS_SINT32_T(Imm) ->
492+
<<16#48, 16#2D, Imm:32/little>>;
493+
subq(Imm, Reg) when ?IS_SINT32_T(Imm), is_atom(Reg) ->
494+
{REX_B, MODRM_RM} = x86_64_x_reg(Reg),
495+
<<?X86_64_REX(1, 0, 0, REX_B), 16#81, 3:2, 5:3, MODRM_RM:3, Imm:32/little>>;
486496
subq(RegA, RegB) when is_atom(RegA), is_atom(RegB) ->
487497
{REX_R, MODRM_REG} = x86_64_x_reg(RegA),
488498
{REX_B, MODRM_RM} = x86_64_x_reg(RegB),

‎tests/erlang_tests/CMakeLists.txt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -540,6 +540,7 @@ compile_erlang(test_function_exported)
540540
compile_erlang(test_list_to_tuple)
541541

542542
compile_erlang(bs_context_byte_size)
543+
compile_erlang(bs_get_binary_fixed_size)
543544
compile_erlang(bs_context_to_binary_with_offset)
544545
compile_erlang(bs_restore2_start_offset)
545546
compile_erlang(test_refc_binaries)
@@ -613,6 +614,9 @@ compile_assembler(test_op_bs_start_match_asm)
613614
compile_erlang(test_op_bs_create_bin)
614615
compile_assembler(test_op_bs_create_bin_asm)
615616

617+
compile_erlang(bs_get_binary_fixed_size)
618+
compile_assembler(bs_get_binary2_all_asm)
619+
616620
compile_erlang(bigint)
617621
compile_erlang(bigint_stress)
618622

@@ -624,6 +628,7 @@ compile_erlang(test_lists_keyfind)
624628
if(Erlang_VERSION VERSION_GREATER_EQUAL "23")
625629
set(OTP23_OR_GREATER_TESTS
626630
test_op_bs_start_match_asm.beam
631+
bs_get_binary2_all_asm.beam
627632
)
628633
else()
629634
set(OTP23_OR_GREATER_TESTS)
@@ -1074,6 +1079,7 @@ set(erlang_test_beams
10741079
test_list_to_tuple.beam
10751080

10761081
bs_context_byte_size.beam
1082+
bs_get_binary_fixed_size.beam
10771083
bs_context_to_binary_with_offset.beam
10781084
bs_restore2_start_offset.beam
10791085
bs_append_extra_words.beam

‎tests/erlang_tests/bs_get_binary2_all_asm.S‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
%
2+
% This file is part of AtomVM.
3+
%
4+
% Copyright 2026 Paul Guyot <pguyot@kallisys.net>
5+
%
6+
% Licensed under the Apache License, Version 2.0 (the "License");
7+
% you may not use this file except in compliance with the License.
8+
% You may obtain a copy of the License at
9+
%
10+
% http://www.apache.org/licenses/LICENSE-2.0
11+
%
12+
% Unless required by applicable law or agreed to in writing, software
13+
% distributed under the License is distributed on an "AS IS" BASIS,
14+
% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
% See the License for the specific language governing permissions and
16+
% limitations under the License.
17+
%
18+
% SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later
19+
%
20+
21+
%% BEAM assembly file to test bs_get_binary2 opcode with {atom, all}.
22+
%% Modern OTP compilers never generate bs_get_binary2 with all -- they use
23+
%% bs_get_tail or bs_skip_bits2 instead. This file exercises the JIT code
24+
%% path at jit.erl line 1347 (Size =:= ?ALL_ATOM).
25+
26+
{module, bs_get_binary2_all_asm}.
27+
28+
{exports, [
29+
{extract_all_after_skip1, 1}
30+
]}.
31+
32+
{attributes, []}.
33+
34+
{labels, 5}.
35+
36+
%% extract_all_after_skip1(Bin) ->
37+
%% Skip 1 byte, then get the remaining bytes using
38+
%% bs_get_binary2 with {atom, all}.
39+
%% Returns the extracted binary, or the atom 'error' on failure.
40+
{function, extract_all_after_skip1, 1, 2}.
41+
{label, 1}.
42+
{func_info, {atom, bs_get_binary2_all_asm}, {atom, extract_all_after_skip1}, 1}.
43+
{label, 2}.
44+
{bs_start_match4, {atom, no_fail}, 1, {x, 0}, {x, 0}}.
45+
{test, bs_skip_bits2,
46+
{f, 3},
47+
[{x, 0}, {integer, 8}, 1, {field_flags, [unsigned, big]}]}.
48+
{test, bs_get_binary2,
49+
{f, 3},
50+
1,
51+
[{x, 0}, {atom, all}, 8, {field_flags, [unsigned, big]}],
52+
{x, 0}}.
53+
return.
54+
{label, 3}.
55+
{move, {atom, error}, {x, 0}}.
56+
return.

‎tests/erlang_tests/bs_get_binary_fixed_size.erl‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
%
2+
% This file is part of AtomVM.
3+
%
4+
% Copyright 2026 Paul Guyot <pguyot@kallisys.net>
5+
%
6+
% Licensed under the Apache License, Version 2.0 (the "License");
7+
% you may not use this file except in compliance with the License.
8+
% You may obtain a copy of the License at
9+
%
10+
% http://www.apache.org/licenses/LICENSE-2.0
11+
%
12+
% Unless required by applicable law or agreed to in writing, software
13+
% distributed under the License is distributed on an "AS IS" BASIS,
14+
% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
% See the License for the specific language governing permissions and
16+
% limitations under the License.
17+
%
18+
% SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later
19+
%
20+
21+
-module(bs_get_binary_fixed_size).
22+
23+
%% Force the compiler to use bs_get_binary2 opcode instead of the
24+
%% newer bs_match opcode (OTP 25+). On older OTP this is ignored.
25+
%% The no_bs_match option was removed in OTP 29.
26+
-ifdef(OTP_RELEASE).
27+
-if(?OTP_RELEASE =< 28).
28+
-compile([no_bs_match]).
29+
-endif.
30+
-endif.
31+
32+
-export([start/0]).
33+
34+
start() ->
35+
Bin = id(<<1, 2, 3, 4, 5, 6, 7, 8>>),
36+
ok = test_match_4(Bin),
37+
ok = test_match_3(Bin),
38+
%% bs_get_binary2_all_asm uses bs_start_match4 which requires OTP 23+
39+
HasAsmModule =
40+
case erlang:system_info(machine) of
41+
"BEAM" ->
42+
erlang:system_info(otp_release) >= "23";
43+
"ATOM" ->
44+
?OTP_RELEASE >= 23
45+
end,
46+
ok =
47+
if
48+
HasAsmModule -> test_match_all(Bin);
49+
true -> ok
50+
end,
51+
ok = test_match_fail(Bin),
52+
0.
53+
54+
test_match_4(Bin) ->
55+
<<Head:4/binary, Rest/binary>> = Bin,
56+
4 = byte_size(Head),
57+
4 = byte_size(Rest),
58+
<<1, 2, 3, 4>> = Head,
59+
<<5, 6, 7, 8>> = Rest,
60+
ok.
61+
62+
test_match_3(Bin) ->
63+
<<Head:3/binary, Rest/binary>> = Bin,
64+
3 = byte_size(Head),
65+
5 = byte_size(Rest),
66+
<<1, 2, 3>> = Head,
67+
<<4, 5, 6, 7, 8>> = Rest,
68+
ok.
69+
70+
test_match_all(Bin) ->
71+
%% Uses bs_get_binary2 with {atom, all} via hand-written BEAM assembly,
72+
%% since the compiler generates bs_skip_bits2/bs_get_tail instead.
73+
<<2, 3, 4, 5, 6, 7, 8>> = bs_get_binary2_all_asm:extract_all_after_skip1(Bin),
74+
7 = byte_size(bs_get_binary2_all_asm:extract_all_after_skip1(Bin)),
75+
ok.
76+
77+
test_match_fail(Bin) ->
78+
case Bin of
79+
<<_Head:9/binary, _Rest/binary>> ->
80+
error;
81+
_ ->
82+
ok
83+
end.
84+
85+
id(X) -> X.

‎tests/libs/jit/jit_x86_64_asm_tests.erl‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -914,9 +914,59 @@ andb_test_() ->
914914

915915
subq_test_() ->
916916
[
917+
% 8-bit immediates
918+
?_assertAsmEqual(
919+
<<16#48, 16#83, 16#E8, 16#01>>, "subq $0x1, %rax", jit_x86_64_asm:subq(1, rax)
920+
),
921+
?_assertAsmEqual(
922+
<<16#48, 16#83, 16#E9, 16#01>>, "subq $0x1, %rcx", jit_x86_64_asm:subq(1, rcx)
923+
),
924+
?_assertAsmEqual(
925+
<<16#48, 16#83, 16#EA, 16#01>>, "subq $0x1, %rdx", jit_x86_64_asm:subq(1, rdx)
926+
),
927+
?_assertAsmEqual(
928+
<<16#48, 16#83, 16#EE, 16#01>>, "subq $0x1, %rsi", jit_x86_64_asm:subq(1, rsi)
929+
),
930+
?_assertAsmEqual(
931+
<<16#48, 16#83, 16#EF, 16#01>>, "subq $0x1, %rdi", jit_x86_64_asm:subq(1, rdi)
932+
),
933+
?_assertAsmEqual(
934+
<<16#49, 16#83, 16#E8, 16#01>>, "subq $0x1, %r8", jit_x86_64_asm:subq(1, r8)
935+
),
936+
?_assertAsmEqual(
937+
<<16#49, 16#83, 16#E9, 16#01>>, "subq $0x1, %r9", jit_x86_64_asm:subq(1, r9)
938+
),
939+
?_assertAsmEqual(
940+
<<16#49, 16#83, 16#EA, 16#01>>, "subq $0x1, %r10", jit_x86_64_asm:subq(1, r10)
941+
),
942+
?_assertAsmEqual(
943+
<<16#49, 16#83, 16#EB, 16#01>>, "subq $0x1, %r11", jit_x86_64_asm:subq(1, r11)
944+
),
945+
% Register to register
917946
?_assertAsmEqual(<<16#48, 16#29, 16#c1>>, "subq %rax, %rcx", jit_x86_64_asm:subq(rax, rcx)),
918947
?_assertAsmEqual(<<16#49, 16#29, 16#c2>>, "subq %rax, %r10", jit_x86_64_asm:subq(rax, r10)),
919-
?_assertAsmEqual(<<16#4c, 16#29, 16#c1>>, "subq %r8, %rcx", jit_x86_64_asm:subq(r8, rcx))
948+
?_assertAsmEqual(<<16#4c, 16#29, 16#c1>>, "subq %r8, %rcx", jit_x86_64_asm:subq(r8, rcx)),
949+
% 32-bit immediates
950+
?_assertAsmEqual(
951+
<<16#48, 16#2D, 16#78, 16#56, 16#34, 16#12>>,
952+
"subq $0x12345678,%rax",
953+
jit_x86_64_asm:subq(16#12345678, rax)
954+
),
955+
?_assertAsmEqual(
956+
<<16#48, 16#81, 16#EE, 16#78, 16#56, 16#34, 16#12>>,
957+
"subq $0x12345678,%rsi",
958+
jit_x86_64_asm:subq(16#12345678, rsi)
959+
),
960+
?_assertAsmEqual(
961+
<<16#49, 16#81, 16#EB, 16#78, 16#56, 16#34, 16#12>>,
962+
"subq $0x12345678,%r11",
963+
jit_x86_64_asm:subq(16#12345678, r11)
964+
),
965+
?_assertAsmEqual(
966+
<<16#48, 16#2D, 16#88, 16#A9, 16#CB, 16#ED>>,
967+
"subq $-0x12345678,%rax",
968+
jit_x86_64_asm:subq(-16#12345678, rax)
969+
)
920970
].
921971

922972
decl_test_() ->

‎tests/test.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -524,6 +524,7 @@ struct Test tests[] = {
524524
TEST_CASE_EXPECTED(test_list_to_tuple, 69),
525525

526526
TEST_CASE(bs_context_byte_size),
527+
TEST_CASE(bs_get_binary_fixed_size),
527528
TEST_CASE_EXPECTED(bs_context_to_binary_with_offset, 42),
528529
TEST_CASE_EXPECTED(bs_restore2_start_offset, 823),
529530

0 commit comments

Comments
 (0)