Skip to content

Conversation

@pguyot
Copy link
Collaborator

@pguyot pguyot commented Nov 28, 2022

Tests pass with beams compiled with OTP-25.

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

@pguyot pguyot marked this pull request as draft November 28, 2022 21:49
@pguyot pguyot force-pushed the w47/add-otp25-opcodes branch from f6bd7da to 65d517b Compare November 29, 2022 23:33
@pguyot pguyot force-pushed the w47/add-otp25-opcodes branch 2 times, most recently from 96f9741 to f7d50aa Compare December 1, 2022 07:56
@pguyot pguyot force-pushed the w47/add-otp25-opcodes branch from f7d50aa to d4f4b2d Compare December 19, 2022 18:58
@pguyot pguyot force-pushed the w47/add-otp25-opcodes branch 2 times, most recently from 7f29a04 to d0f26f5 Compare January 4, 2023 06:03
@pguyot pguyot marked this pull request as ready for review January 4, 2023 06:05
static const char *const avm_floatsize_atom = "\xD" "avm_floatsize";

static const char *const append_atom = "\x6" "append";
static const char *const private_append_atom = "\xD" "private_append";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I counted 14 chars, so it should be "\xE".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Obviously this isn't tested. I haven't encountered private_append in generated beam code but found it in BEAM source code.
Not sure if we should remove the value.
Meanwhile, I did fix the length byte.

Copy link
Collaborator

Choose a reason for hiding this comment

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

default atoms allows us to reference them as constants, that can be used with statements such as switch, therefore if it is not referenced in our code it can be safely discarded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In addition it does consume memory even if not referenced. I would prefer that defaultatoms.[ch] have a very conservative set of declarations that have a high probability of being used. ok, error, undefined all come to mind. Maybe a few others.

If possible I would err on the side of instantiating the atom only when needed, instead of at load time, but that is my personal opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

private_append was used in the code but we had no test. I knew about it, it was a shortcoming of this PR, and was just too lazy to build an example. But then @bettio 's sharp eyes found out that it wouldn't work because the atom was wrongly defined.

So please let me do this properly as it should have been done. Scanning all OTP beams, I found many modules where this is genereated and will create a test case accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will try to take a look to this

@pguyot pguyot force-pushed the w47/add-otp25-opcodes branch from d0f26f5 to 24b44f7 Compare January 5, 2023 06:13
@pguyot pguyot force-pushed the w47/add-otp25-opcodes branch from 24b44f7 to d498d02 Compare January 7, 2023 06:39
pguyot added 2 commits January 7, 2023 07:41
Add the following three OTP-25 opcodes:
- bs_create_bin/7
- call_fun2/3
- badrecord/1

The following opcode isn't part of this commit:
- nif_start/0

But we probably do not need it.

Also add the following two opcodes to extend support of bs_create_bin:
- bs_init_writable
- bs_private_append

This adds (limited) support to binary comprehensions. Unaligned binaries
are still not supported.

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Show the support for all opcodes we need to tests

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
@pguyot pguyot force-pushed the w47/add-otp25-opcodes branch from d498d02 to 45ea90d Compare January 7, 2023 06:41
@bettio bettio merged commit c59671c into atomvm:master Jan 7, 2023
@pguyot pguyot deleted the w47/add-otp25-opcodes branch January 7, 2023 13:43
@pguyot pguyot mentioned this pull request Jan 8, 2023
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.

3 participants