Skip to content

Fix rem bigint operand dispatch on 32-bit#2381

Open
bettio wants to merge 1 commit into
atomvm:release-0.7from
bettio:fix-rem-bif
Open

Fix rem bigint operand dispatch on 32-bit#2381
bettio wants to merge 1 commit into
atomvm:release-0.7from
bettio:fix-rem-bif

Conversation

@bettio

@bettio bettio commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

rem_boxed_helper or-combined the boxed sizes of its operands, a
leftover from when a boxed integer could only occupy one or two terms.
Big integers with a 64-bit magnitude occupy exactly 3 terms on 32-bit
builds, so pairing one with any operand of boxed size <= 3 also
produced 3, the same value as the 1-word/2-word mix, and the operation
ran on the low 64 bits reinterpreted as an int64: (1 bsl 63) rem 7
returned -1 instead of 1, and 5 rem ((1 bsl 64) - 1) returned 0
instead of 5. 64-bit builds are unaffected, since the int64 arm is
compiled out there.

Combine the sizes with MAX instead, as the other arithmetic helpers
(add, sub, mul, div) already do, so any pairing that involves a big
integer takes the big integer path, and extend the bigint Erlang test
with boundary cases around the 64-bit magnitude range.

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

rem_boxed_helper was or-combining boxed operand sizes, so 3-term big
integers on 32-bit hit the int64 arm: (1 bsl 63) rem 7 returned -1.

Signed-off-by: Davide Bettio <davide@uninstall.it>

@petermm petermm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Fix rem bigint operand dispatch on 32-bit

Commit: 30de17ff9d9be7feebdbe55505830c9ce45b2a53
Verdict: Approve — no commit-introduced blockers.

Summary

The fix is correct and narrowly scoped. In rem_boxed_helper, boxed-term sizes describe representation classes, not bit flags. Replacing bitwise OR with MAX and removing case 3 from the int64 arm ensures that a 3-term sign-magnitude bigint on a 32-bit build reaches rem_maybe_bigint instead of being decoded as a 2-term two's-complement int64.

The change preserves:

  • badarith for non-integer operands and a zero divisor;
  • the native fast paths for 1-term integers and 2-term int64 values;
  • Erlang remainder semantics, including the dividend's sign;
  • the existing bigint path for values outside signed 64-bit range.

The regression tests in bigint.erl are meaningful. Dynamic binary_to_integer/2 construction plus id/1 prevents constant folding, and values at or above 16#8000000000000000 use the affected 3-term representation on i386. The cases exercise a bigint in both operand positions, multiple divisor representation sizes, a negative dividend, bigint-vs-bigint operations, and division by zero.

Findings

No correctness findings introduced by this commit

The implementation follows the same MAX(arg1_size, arg2_size) dispatch used by the neighboring arithmetic BIFs. The changelog entry accurately describes the affected platform and failure mode.

High, non-blocking follow-up: signed minimum rem -1 has pre-existing C undefined behavior

Location: src/libAtomVM/bif.c:1458-1476

Both native boxed branches evaluate val1 % val2 after checking only for zero. C defines both division and remainder as undefined when the dividend is the minimum signed value and the divisor is -1, because the corresponding quotient is not representable. Erlang requires the remainder to be 0.

This affects INT32_MIN rem -1 in the 1-term branch on 32-bit builds and INT64_MIN rem -1 in the 2-term branch. On 64-bit builds, the corresponding native-minimum case reaches the 1-term branch. The expressions were already present before this commit, so this is not a regression and should not block approval, but the touched helper is a good place to address it.

Suggested small fix

Any integer modulo -1 is zero, so bypassing % avoids the unrepresentable quotient without changing semantics:

diff --git a/src/libAtomVM/bif.c b/src/libAtomVM/bif.c
--- a/src/libAtomVM/bif.c
+++ b/src/libAtomVM/bif.c
@@ -1461,6 +1461,9 @@ static term rem_boxed_helper(Context *ctx, uint32_t fail_label, uint32_t live, t
             if (UNLIKELY(val2 == 0)) {
                 RAISE_ERROR_BIF(fail_label, BADARITH_ATOM);
             }
+            if (UNLIKELY(val2 == -1)) {
+                return term_from_int(0);
+            }
 
             return make_maybe_boxed_int(ctx, fail_label, live, val1 % val2);
         }
@@ -1472,6 +1475,9 @@ static term rem_boxed_helper(Context *ctx, uint32_t fail_label, uint32_t live, t
             if (UNLIKELY(val2 == 0)) {
                 RAISE_ERROR_BIF(fail_label, BADARITH_ATOM);
             }
+            if (UNLIKELY(val2 == -1)) {
+                return term_from_int(0);
+            }
 
             return make_maybe_boxed_int64(ctx, fail_label, live, val1 % val2);
         }
diff --git a/tests/erlang_tests/bigint.erl b/tests/erlang_tests/bigint.erl
--- a/tests/erlang_tests/bigint.erl
+++ b/tests/erlang_tests/bigint.erl
@@ -266,6 +266,8 @@ test_rem() ->
     Int12 = erlang:binary_to_integer(?MODULE:id(<<"-8000000000000001">>), 16),
     Int13 = erlang:binary_to_integer(?MODULE:id(<<"FFFFFFFFFFFFFFFF">>), 16),
     Int14 = erlang:binary_to_integer(?MODULE:id(<<"7FFFFFFFFFFFFFFF">>), 16),
+    Int15 = erlang:binary_to_integer(?MODULE:id(<<"-8000000000000000">>), 16),
+    Int16 = erlang:binary_to_integer(?MODULE:id(<<"-80000000">>), 16),
 
     1 = Int10 rem ?MODULE:id(7),
@@ -276,6 +278,8 @@ test_rem() ->
     42 = ?MODULE:id(42) rem Int10,
     <<"7FFFFFFFFFFFFFFF">> = erlang:integer_to_binary(Int14 rem Int11, 16),
     <<"8000000000000000">> = erlang:integer_to_binary(Int10 rem Int11, 16),
+    0 = Int15 rem ?MODULE:id(-1),
+    0 = Int16 rem ?MODULE:id(-1),
 
     ok = expect_error(badarith, fun() -> Int0 rem ?MODULE:id(0) end),

Test coverage and verification

  • git diff --check HEAD^ HEAD — passed.
  • Native build of test-erlang — passed.
  • ./build/tests/test-erlang -s prime_smp — passed.
  • The repository's CI matrix includes a real i386 build using -m32 -O3, and that job runs test-erlang; this is the configuration that exercises the repaired representation-specific path.

The i386 configuration was not reproduced locally on this arm64 macOS host, so the final 32-bit execution result remains delegated to CI.

Oracle review

Oracle was asked to inspect the commit specifically for platform dispatch errors, invalid operand behavior, signed modulo undefined behavior, and whether the tests execute the changed path. It independently returned an approve verdict with no commit-introduced blocker and identified the same pre-existing MIN_INT rem -1 follow-up above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants