Skip to content

Fix sign of results of several binary functions#2256

Open
pguyot wants to merge 1 commit intoatomvm:release-0.7from
pguyot:w14/fix-binary-at
Open

Fix sign of results of several binary functions#2256
pguyot wants to merge 1 commit intoatomvm:release-0.7from
pguyot:w14/fix-binary-at

Conversation

@pguyot
Copy link
Copy Markdown
Collaborator

@pguyot pguyot commented Apr 4, 2026

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
Copy link
Copy Markdown
Contributor

petermm commented Apr 4, 2026

https://ampcode.com/threads/T-019d5730-ef70-7660-96a2-49b946e0baf7

Oracle's verdict: The binary:at/2 fix is correct, but the fix is incomplete — binary:first/1 and binary:last/1 have the same signedness bug. The same (uint8_t) cast should be applied there too, with matching regression tests.

@petermm
Copy link
Copy Markdown
Contributor

petermm commented Apr 4, 2026

Test Diffs: binary:first/1 and binary:last/1 Signedness Regression Tests

Tests for the same uint8_t signedness bug found in binary:at/2, applied to binary:first/1 and binary:last/1. Each test asserts correct unsigned byte values at boundary points: 128, 200, and 255.

tests/erlang_tests/binary_first_test.erl

--- a/tests/erlang_tests/binary_first_test.erl
+++ b/tests/erlang_tests/binary_first_test.erl
@@ -23,11 +23,26 @@
 -export([start/0, id/1, firstp10/1]).
 
 start() ->
-    firstp10(id(<<"HelloWorld">>)) + firstp10safe(<<>>) + firstp10safe(42) + firstp10safe({<<>>}).
+    firstp10(id(<<"HelloWorld">>)) + firstp10safe(<<>>) + firstp10safe(42) + firstp10safe({<<>>}) +
+        high_byte_first_test(id(<<200, 1, 2>>)) +
+        high_byte_first_boundary_test(id(<<128, 1, 2>>)) +
+        high_byte_first_max_test(id(<<255, 1, 2>>)).
 
 firstp10(Bin) ->
     binary:first(Bin) + 10.
 
+high_byte_first_test(Bin) ->
+    200 = binary:first(Bin),
+    0.
+
+high_byte_first_boundary_test(Bin) ->
+    128 = binary:first(Bin),
+    0.
+
+high_byte_first_max_test(Bin) ->
+    255 = binary:first(Bin),
+    0.
+
 firstp10safe(Bin) ->
     try firstp10(Bin) of
         _Any -> 1

tests/erlang_tests/binary_last_test.erl

--- a/tests/erlang_tests/binary_last_test.erl
+++ b/tests/erlang_tests/binary_last_test.erl
@@ -23,11 +23,26 @@
 -export([start/0, id/1, lastp10/1]).
 
 start() ->
-    lastp10(id(<<"HelloWorld">>)) + lastp10safe(<<>>) + lastp10safe(42) + lastp10safe({<<>>}).
+    lastp10(id(<<"HelloWorld">>)) + lastp10safe(<<>>) + lastp10safe(42) + lastp10safe({<<>>}) +
+        high_byte_last_test(id(<<1, 2, 200>>)) +
+        high_byte_last_boundary_test(id(<<1, 2, 128>>)) +
+        high_byte_last_max_test(id(<<1, 2, 255>>)).
 
 lastp10(Bin) ->
     binary:last(Bin) + 10.
 
+high_byte_last_test(Bin) ->
+    200 = binary:last(Bin),
+    0.
+
+high_byte_last_boundary_test(Bin) ->
+    128 = binary:last(Bin),
+    0.
+
+high_byte_last_max_test(Bin) ->
+    255 = binary:last(Bin),
+    0.
+
 lastp10safe(Bin) ->
     try lastp10(Bin) of
         _Any -> 1

Note: Expected return values for both tests remain unchanged (82 and 110) since all new test functions return 0. These tests will fail until the corresponding (uint8_t) cast is applied in nifs.c for nif_binary_first_1 (line 3459) and nif_binary_last_1 (line 3476).

Fix sign of `binary:at/2`, `binary:first/1` and `binary:last/1` results

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
@pguyot pguyot force-pushed the w14/fix-binary-at branch from e8632c3 to 7c3212e Compare April 4, 2026 20:29
@pguyot pguyot changed the title Fix sign of binary:at/2 result Fix sign of results of several binary functions Apr 4, 2026
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