Add `binary_part/3` to BIFs #1569

jgonet · 2025-03-13T23:57:37Z

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

bettio

I think that the NIF function can be completely dropped.

src/libAtomVM/nifs.c

CHANGELOG.md

bettio · 2025-03-16T12:34:08Z

A number of CI fixes has been made in release-0.6. Let's rebase it on latest release-0.6.

src/libAtomVM/nifs.c

src/libAtomVM/bif.c

jgonet · 2025-03-31T17:28:04Z

src/libAtomVM/opcodesswitch.h

+                    *return_value = gcbif->gcbif1_ptr(ctx, 0, 1, ctx->x[0]);
                    return true;
                }
                case 2: {
-                    *return_value = gcbif->gcbif2_ptr(ctx, 0, 0, ctx->x[0], ctx->x[1]);
+                    *return_value = gcbif->gcbif2_ptr(ctx, 0, 2, ctx->x[0], ctx->x[1]);
                    return true;
                }
                case 3: {
-                    *return_value = gcbif->gcbif3_ptr(ctx, 0, 0, ctx->x[0], ctx->x[1], ctx->x[2]);
+                    *return_value = gcbif->gcbif3_ptr(ctx, 0, 3, ctx->x[0], ctx->x[1], ctx->x[2]);


I'll rebase after #1610 is merged.

You don't need this, see my other comments.

src/libAtomVM/bif.h

pguyot · 2025-03-31T18:29:01Z

src/libAtomVM/bif.c

+        RAISE_ERROR_BIF(fail_label, OUT_OF_MEMORY_ATOM);
+    }
+
+    return term_maybe_create_sub_binary(ctx->x[0], slice.pos, slice.len, &ctx->heap, ctx->global);


This should be arg1, not ctx->x[0]

pguyot · 2025-03-31T18:29:57Z

src/libAtomVM/bif.c

+        RAISE_ERROR_BIF(fail_label, BADARG_ATOM);
+    }
+
+    if (UNLIKELY(memory_ensure_free_with_roots(ctx, slice.heap_size, live, ctx->x, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {


You need to add arg1 to ctx->[live], and pass live+1 with a comment saying you can do that because x has size MAX_REGS+1.

I don't understand. If live is always zero, we'd clobber x[1] in that case and preserve nothing since roots length is still 0.

Sorry for the confusion.

I meant:

Suggested change

if (UNLIKELY(memory_ensure_free_with_roots(ctx, slice.heap_size, live, ctx->x, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {

// We can always use ctx->x[live] as ctx->x has MAX_REGS+1 slots

ctx->x[live] = arg1;

if (UNLIKELY(memory_ensure_free_with_roots(ctx, slice.heap_size, live + 1, ctx->x, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {

If live is 0, you can clobber x[1]. This is what live being 0 means.

I don't understand the implementation then. In maybe_call_native we hardcode live=0. It's used in tail-calls (which is fine) and in apply. Why it's fine to clobber registers in apply case?

I believe it's fine to clobber x regs in any NIF or external calls. My understanding is that x regs are used for args and results for NIF or external calls, and they do not need to be preserved. It is as if live was equal to 0. Caller does not even ensure that they are valid beyond argc. On return, only x[0] is considered valid.

I agree with @pguyot, it works with both NIFs and BIFs.

pguyot · 2025-03-31T18:30:27Z

src/libAtomVM/opcodesswitch.h

+                    *return_value = gcbif->gcbif1_ptr(ctx, 0, 1, ctx->x[0]);
                    return true;
                }
                case 2: {
-                    *return_value = gcbif->gcbif2_ptr(ctx, 0, 0, ctx->x[0], ctx->x[1]);
+                    *return_value = gcbif->gcbif2_ptr(ctx, 0, 2, ctx->x[0], ctx->x[1]);
                    return true;
                }
                case 3: {
-                    *return_value = gcbif->gcbif3_ptr(ctx, 0, 0, ctx->x[0], ctx->x[1], ctx->x[2]);
+                    *return_value = gcbif->gcbif3_ptr(ctx, 0, 3, ctx->x[0], ctx->x[1], ctx->x[2]);


You don't need this, see my other comments.

bettio · 2025-04-03T13:14:30Z

src/libAtomVM/opcodesswitch.h

@@ -1612,15 +1612,15 @@ static bool maybe_call_native(Context *ctx, AtomString module_name, AtomString f
            const struct GCBif *gcbif = EXPORTED_FUNCTION_TO_GCBIF(exported_bif);
            switch (arity) {
                case 1: {
-                    *return_value = gcbif->gcbif1_ptr(ctx, 0, 0, ctx->x[0]);
+                    *return_value = gcbif->gcbif1_ptr(ctx, 0, 1, ctx->x[0]);


By the way, since we have another open discussion on this topic, and the issue we experienced here turned out to be something different (did I understood this right?), I suggest removing this change from here and making this PR focused on just binary_part.

bettio · 2025-04-03T13:19:24Z

Let's move forward with binary_part, and let's have an additional (but separated) discussion about live parameter when doing the NIF trick on that specific PR.

bettio

I'm ok with this approach, it is a matter of polishing. Correct naming and placing functions in the right place is important to keep everything maintainable.

bettio · 2025-04-04T10:20:42Z

src/libAtomVM/bif.c

@@ -108,6 +108,48 @@ term bif_erlang_bit_size_1(Context *ctx, uint32_t fail_label, int live, term arg
    return term_from_int32(len);
 }

+bool get_sub_binary_slice(term bin_term, term pos_term, term len_term, struct SubBinarySlice *slice)


The name didn't suggest me the purpose of this function without looking at the actual implementation. What about maybe_get_sub_binary_slice or even sanitize_sub_binary_slice.
Furthermore,

we keep prefixing "public" function names, so a "public" function declared in bif.h is named such as bif_sanitize_sub_binary_slice.

I would keep this function in term.h, since it is a common helper between nifs and bifs.

It might be even a more reusable API if take indexes as avm_int_t, so we might use it internally in other situations.

bettio · 2025-04-04T10:27:30Z

src/libAtomVM/bif.h

@@ -39,11 +39,20 @@ extern "C" {

 #define MAX_BIF_NAME_LEN 260

+struct SubBinarySlice


We are using this struct only as an output of get_sub_binary_slice, so we might link better the 2 names when we rename the function. It might be called SlicedBinary or SanitizedSlice or whatever we'll use as new name.
Also let's not forget to do the typedef trick, so we don't have to use all around structSubBinarySlice foo_bar;

Signed-off-by: Jakub Gonet <[email protected]>

bettio · 2025-04-09T17:13:48Z

src/libAtomVM/term.h

+    return pos_len.pos == -1 && pos_len.len == -1;
+}
+
+static inline BinaryPosLen term_invalid_binary_pos_len(void)


Sorry, last minute review: term_is_invalid_binary_pos_len and term_invalid_binary_pos_len are unused, let's remove them.
Also, for the future: it is not necessary having (void) for functions with no parameters. () is ok.

bettio

Sorry, forget my last comment. I didn't notice this was required for #1621

Merge `binary_part/3` BIF (PR #1569), fix to `is_number` (#1624) and migration to ubuntu-20.04 container (#1626) from release-0.6 branch.

jgonet mentioned this pull request Mar 13, 2025

Elixir dynamic module compilation #1509

Open

36 tasks

bettio requested changes Mar 14, 2025

View reviewed changes

src/libAtomVM/nifs.c Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

bettio mentioned this pull request Mar 16, 2025

v0.6.6 release #1581

Open

3 tasks

bettio requested changes Mar 26, 2025

View reviewed changes

src/libAtomVM/nifs.c Show resolved Hide resolved

src/libAtomVM/bif.c Outdated Show resolved Hide resolved

jgonet force-pushed the jgonet/binary-part branch 2 times, most recently from cac7b78 to 0b91671 Compare March 31, 2025 17:27

jgonet commented Mar 31, 2025

View reviewed changes

pguyot requested changes Mar 31, 2025

View reviewed changes

bettio requested changes Apr 3, 2025

View reviewed changes

jgonet force-pushed the jgonet/binary-part branch from 0b91671 to ec4baea Compare April 3, 2025 17:58

bettio requested changes Apr 4, 2025

View reviewed changes

Add binary_part/3 to BIFs

19395fa

Signed-off-by: Jakub Gonet <[email protected]>

jgonet force-pushed the jgonet/binary-part branch from ec4baea to 19395fa Compare April 7, 2025 14:54

bettio approved these changes Apr 8, 2025

View reviewed changes

bettio changed the base branch from main to release-0.6 April 8, 2025 22:35

bettio changed the title ~~Add binary_part/3 to BIFs~~ Add binary_part/3 to BIFs Apr 8, 2025

bettio requested changes Apr 9, 2025

View reviewed changes

bettio approved these changes Apr 9, 2025

View reviewed changes

bettio merged commit d024e6d into atomvm:release-0.6 Apr 9, 2025
146 of 149 checks passed

bettio added a commit that referenced this pull request Apr 9, 2025

Forward port changes from v0.6 release branch

75d6d2d

Merge `binary_part/3` BIF (PR #1569), fix to `is_number` (#1624) and migration to ubuntu-20.04 container (#1626) from release-0.6 branch.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `binary_part/3` to BIFs #1569

Add `binary_part/3` to BIFs #1569

jgonet commented Mar 13, 2025

bettio left a comment

bettio commented Mar 16, 2025

jgonet Mar 31, 2025

pguyot Mar 31, 2025

pguyot Mar 31, 2025

pguyot Mar 31, 2025

jgonet Mar 31, 2025

pguyot Apr 1, 2025

jgonet Apr 1, 2025

pguyot Apr 2, 2025

bettio Apr 3, 2025

pguyot Mar 31, 2025

bettio Apr 3, 2025

bettio commented Apr 3, 2025

bettio left a comment

bettio Apr 4, 2025

bettio Apr 4, 2025

bettio Apr 4, 2025

bettio Apr 9, 2025

bettio left a comment

-    if (UNLIKELY(memory_ensure_free_with_roots(ctx, slice.heap_size, live, ctx->x, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {
+    // We can always use ctx->x[live] as ctx->x has MAX_REGS+1 slots
+    ctx->x[live] = arg1;
+    if (UNLIKELY(memory_ensure_free_with_roots(ctx, slice.heap_size, live + 1, ctx->x, MEMORY_CAN_SHRINK) != MEMORY_GC_OK)) {

		@@ -39,11 +39,20 @@ extern "C" {

		#define MAX_BIF_NAME_LEN 260

		struct SubBinarySlice

Add binary_part/3 to BIFs #1569

Add binary_part/3 to BIFs #1569

Conversation

jgonet commented Mar 13, 2025

bettio left a comment

Choose a reason for hiding this comment

bettio commented Mar 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bettio commented Apr 3, 2025

bettio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bettio left a comment

Choose a reason for hiding this comment

Add `binary_part/3` to BIFs #1569

Add `binary_part/3` to BIFs #1569