Conversation
A practical guide about how to make commits and PRs for this project. Signed-off-by: Davide Bettio <davide@uninstall.it>
|
@codex review. |
There was a problem hiding this comment.
1 issue found across 34 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/build-and-test-macos.yaml">
<violation number="1" location=".github/workflows/build-and-test-macos.yaml:43">
P2: The workflow adds `mbedtls@4` to the matrix but only configures `MBEDTLS_PREFIX` for `mbedtls@3`. For `mbedtls@4` runs, the build won’t set `CMAKE_PREFIX_PATH`/flags and may not find the library. Add a matching setup step for `mbedtls@4` or generalize the prefix detection.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| os: ["macos-14", "macos-15", "macos-15-intel", "macos-26"] | ||
| otp: ["25", "26", "27", "28"] | ||
| mbedtls: ["mbedtls@3"] | ||
| mbedtls: ["mbedtls@3", "mbedtls@4"] |
There was a problem hiding this comment.
P2: The workflow adds mbedtls@4 to the matrix but only configures MBEDTLS_PREFIX for mbedtls@3. For mbedtls@4 runs, the build won’t set CMAKE_PREFIX_PATH/flags and may not find the library. Add a matching setup step for mbedtls@4 or generalize the prefix detection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/build-and-test-macos.yaml, line 43:
<comment>The workflow adds `mbedtls@4` to the matrix but only configures `MBEDTLS_PREFIX` for `mbedtls@3`. For `mbedtls@4` runs, the build won’t set `CMAKE_PREFIX_PATH`/flags and may not find the library. Add a matching setup step for `mbedtls@4` or generalize the prefix detection.</comment>
<file context>
@@ -40,7 +40,7 @@ jobs:
os: ["macos-14", "macos-15", "macos-15-intel", "macos-26"]
otp: ["25", "26", "27", "28"]
- mbedtls: ["mbedtls@3"]
+ mbedtls: ["mbedtls@3", "mbedtls@4"]
cmake_opts_other: [""]
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a2edf1a5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(6, 0, 0) | ||
| esp_sleep_gpio_wake_up_mode_t mode = term_to_int(argv[1]); | ||
| esp_err_t err = esp_sleep_enable_gpio_wakeup_on_hp_periph_powerdown(mask, mode); |
There was a problem hiding this comment.
Restore the deep-sleep GPIO wakeup API on ESP-IDF 6
On ESP-IDF 6+ this changes esp:deep_sleep_enable_gpio_wakeup/2 to call esp_sleep_enable_gpio_wakeup_on_hp_periph_powerdown(), but that Espressif API is for the light-sleep/HP-peripheral-powerdown path, not the deep-sleep GPIO wakeup path that this NIF, its docs, and examples/erlang/esp32/deep_sleep.erl expose. For C2/C3/C6/P4 builds on IDF 6, configuring GPIO wakeup before esp:deep_sleep/1 will stop working even though the public API name is unchanged.
Useful? React with 👍 / 👎.
Add new brief jit documentation with information on DWARF support Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Add git guide 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
Implement erlang:node/1 for pid, port, and reference terms. Add coverage for local, external, and badarg cases in test_node. Signed-off-by: Peter M <petermm@gmail.com>
Per the Erlang specification (erlang.org/doc/system/expressions.html),
the term ordering is:
number < atom < reference < fun < port < pid < tuple < map < nil < list < bit string
Maps were incorrectly placed after bitstrings (index 12) instead of
between tuples (8) and nil. This caused comparisons like #{} > [] to
return true, contrary to OTP behavior.
Update test_types_ordering to include maps in the sorted type list,
verifying correct placement between tuples and nil/lists.
Signed-off-by: Peter M <petermm@gmail.com>
Implement erts_internal:cmp_term/2 which compares two terms using exact comparison and returns -1, 0, or 1. Signed-off-by: Peter M <petermm@gmail.com>
JIT: Add DWARF support Continuation of: - atomvm#1881 - atomvm#1829 - atomvm#1891 - atomvm#1838 - atomvm#1900 - atomvm#1901 - atomvm#1903 - atomvm#1913 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
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Fix flaky tests related to GitHub DNS Resolver 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
Add erlang node/1 BIF Implement erlang:node/1 for pid, port, and reference terms. Add coverage for local, external, and badarg cases in test_node. Fixes atomvm#2195 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
Fix map type ordering & add erts_internal:cmp_term/2 Note map types had incorrect Index, first commit fixes that. Fixes atomvm#2188 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
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Use shorter instruction encodings where possible: - xorl reg,reg (2-3 bytes) instead of movq $0,reg (7 bytes) to zero registers - movl imm32,reg (5-6 bytes) instead of movq imm32,reg (7 bytes) for unsigned 32-bit values - jmpq memory-indirect for call_primitive_last with no arguments Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Test make_fun2 opcode on OTP26 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
Test allocate_zero opcode on OTP26 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
Using X macros allows us to turn opcodes.h simple definitions into an opcode table with multiple columns. Signed-off-by: Davide Bettio <davide@uninstall.it>
Replace duplicated decode logic in opcodesswitch.h with a new code loader in module.c driven by opcode signatures declared in opcodes.def. Each opcode declares a compact signature string (e.g. "Af", "jtbssd") that describes its argument encoding. The loader iterates the signature and dispatches to the appropriate DECODE_* macro for each character, removing per-opcode inline decode duplication and reducing binary size. Variable-length arguments use a [X] loop syntax that handles extended list tags automatically, eliminating most custom handler functions. The signature alphabet is inspired to OTP's beam_makeops conventions (s, d, f, j, t, I, A, Q, e, b, F, etc.) so signatures are readable by anyone familiar with BEAM internals. Opcodes removed in commit 94f6f0b are marked with X_OPCODE_REMOVED. Signed-off-by: Davide Bettio <davide@uninstall.it>
Code loading is now handled by parse_core_chunk() in module.c using opcode signatures. Remove the IMPL_CODE_LOADER compilation pass from opcodesswitch.h, leaving it as execution-only. - Remove all #ifdef IMPL_CODE_LOADER blocks (decode macros, TRACE, UNUSED, module_add_label, module_insert_line_ref_offset, entry points) - Remove #ifdef IMPL_EXECUTE_LOOP guards (now always true) - Remove redundant USED_BY_TRACE for variables already used in execution code Signed-off-by: Davide Bettio <davide@uninstall.it>
Run clang-format after code loading logic removal. Signed-off-by: Davide Bettio <davide@uninstall.it>
Signed-off-by: Davide Bettio <davide@uninstall.it>
eee6de2 to
24daab8
Compare
When UPDATE_JIT_TESTS env var is set, mismatched JIT test expectations are automatically updated. This proves useful to adapt to important changes such as generational gc. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Part of the code is shared with riscv32 assembler. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
…coding-optimizations JIT x86: instruction encoding optimizations Use shorter instruction encodings where possible: - xorl reg,reg (2-3 bytes) instead of movq $0,reg (7 bytes) to zero registers - movl imm32,reg (5-6 bytes) instead of movq imm32,reg (7 bytes) for unsigned 32-bit values - jmpq memory-indirect for call_primitive_last with no arguments 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
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Create libs/avm_unix with a uart module implementing the uart_hal behaviour using posix_open, posix_read, posix_write, posix_select_read and the new termios NIFs. Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Fix bug in `bs_match` `get_tail` handling 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
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Introduce shortest-roundtrip formatting for float_to_binary/2 and
float_to_list/2 via the [short] option, matching Erlang/OTP
behavior.
- Add float_utils with Grisu3 for doubles and best-effort snprintf
for 32-bit floats. Grisu3 was chosen over Ryu (used by
Erlang/OTP) for its compact code and small powers table, saving
flash space on embedded targets.
- Add unlocalized_snprintf to fix locale-dependent decimal
separator bug in float formatting.
- Extend option ranges to {decimals, 0..253} and
{scientific, 0..249} to match Erlang/OTP limits.
Signed-off-by: Davide Bettio <davide@uninstall.it>
Now that float_to_binary/2 and float_to_list/2 support [short],
replace the {decimals, 17} + compact workaround with [:short].
Signed-off-by: Davide Bettio <davide@uninstall.it>
Add short option to float_to_binary/list
- Add `[short]` option for shortest-roundtrip float formatting, matching
Erlang/OTP behavior. Uses Grisu3 for 64-bit doubles, best-effort snprintf
for 32-bit floats.
- Fix locale-dependent decimal separator bug (e.g. `,` instead of `.` in
some locales) via `unlocalized_snprintf`.
- Extend option ranges to `{decimals, 0..253}` and `{scientific, 0..249}`
to match Erlang/OTP limits.
Fixes atomvm#2189
Erlang/OTP uses Ryu, which guarantees shortest output for all doubles.
Grisu3 produces the shortest output for ~99.5% of values and falls back
to a longer (but correct) 17-digit representation for the rest.
Grisu3 was chosen for its smaller binary footprint on embedded targets:
Stripped AtomVM binary (x86_64, -Os):
Before this PR: 653,768 bytes
With Grisu3:657,864 bytes(+4 KB)
With Ryu (full):756,168 bytes(+98 KB)
With Ryu (small): 747,976 bytes(+90 KB)
For most values the output is identical. The Grisu3 fallback (~0.5% of
doubles) produces longer output:
```erlang
%% Erlang/OTP (Ryu)AtomVM (Grisu3)
%% <<"1.0e23">><<"9.9999999999999992e22">>
Both representations are correct and roundtrip to the same IEEE 754 double.
With 32-bit floats (AVM_USE_32BIT_FLOAT), short uses a best-effort
algorithm (%.9g) since Grisu3 operates on 64-bit doubles only:
%% Erlang/OTP (always 64-bit)AtomVM 32-bit
%% <<"3.14">><<"3.1400001">>
The 32-bit output is longer but always roundtrips correctly.
```
Tests:
Grisu3 implementation has been tested against a dataset with milions of
doubles with no errors.
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
Fix `cancel_timer/1` spec and documentation 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
Add UART support on generic_unix via POSIX termios NIFs 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
Resurrect opcodes emitted with no_bs_create_bin 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
JIT: add support for riscv64 target 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
JIT: remove redundant AND before shift right when untagging integers 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
`binary_to_float(<<"1.0e999">>)` returned inf instead of raising `badarg`. Add `isfinite()` check after `sscanf()` to reject non-finite results, matching Erlang/OTP behavior. Signed-off-by: Davide Bettio <davide@uninstall.it>
Replace sscanf-based float parsing in `binary_to_float`/`list_to_float`
with a proper implementation that validates strict bare float format
and uses locale-independent `strtod`/`strtof`.
Rename `unlocalized_snprintf.{c,h}` to `unlocalized.{c,h}` and add
`unlocalized_strtod()`, `unlocalized_strtof()`, and
`unlocalized_validate_bare_float_format()`. An inline dispatcher
`unlocalized_strto_avm_float()` selects the right variant based on
`AVM_USE_SINGLE_PRECISION`. The three locale strategies (`strtod_l` /
`uselocale` / fallback with separator retry) are shared via a
`prepare_locale_retry_buf()` helper.
Refactor `float_utils` to follow the same pattern: guard
`double_write_to_ascii_buf` and Grisu3 internals behind
`FLOAT_UTILS_ENABLE_DOUBLE_API`, and 32-bit support behind
`FLOAT_UTILS_ENABLE_FLOAT_API`, so no `double` code leaks into a
float-only build and vice versa. Rename `double_format_t` to
`float_format_t` since the enum belongs to the `float_utils` module,
not the `double` type.
The bare float format `[+|-]DIGITS.DIGITS[(e|E)[+|-]DIGITS]` matches
Erlang/OTP's `binary_to_float`/`list_to_float`, except commas are not
accepted as decimal separators (aligning with OTP's future
direction per erlang/otp#9061).
Signed-off-by: Davide Bettio <davide@uninstall.it>
binary_to_float and list_to_float share the same parsing code, so there is no need for two near-identical test modules. Merge them into a single string2float test that covers binary_to_float exhaustively and adds a few basic list_to_float smoke tests. Signed-off-by: Davide Bettio <davide@uninstall.it>
Fix and rework binary_to_float/list_to_float
Fix `binary_to_float/1` and `list_to_float/1` overflow bug and replace
the locale-dependent sscanf-based parser with strict, locale-
independent float parsing.
- Fix `binary_to_float(<<"1.0e999">>)` returning `inf` instead of
raising `badarg`, matching Erlang/OTP behavior.
- Replace `sscanf` with `unlocalized_strtod()`/`unlocalized_strtof()`,
which validate strict bare float format and use a 3-strategy
locale-independent approach (`strtod_l`/`strtof_l` / `uselocale` /
fallback with `prepare_locale_retry_buf()` helper).
- Add `unlocalized_strto_avm_float()` inline dispatcher that selects
the right variant based on `AVM_USE_SINGLE_PRECISION`.
- Rename `unlocalized_snprintf.{c,h}` to `unlocalized.{c,h}` and add
`unlocalized_validate_bare_float_format()`, `unlocalized_strtod()`,
and `unlocalized_strtof()` sharing the same cached C locale
infrastructure.
- Guard double-only code behind `UNLOCALIZED_ENABLE_DOUBLE_API` /
`FLOAT_UTILS_ENABLE_DOUBLE_API` and float-only code behind
`UNLOCALIZED_ENABLE_FLOAT_API` / `FLOAT_UTILS_ENABLE_FLOAT_API`,
so no `double` code leaks into a float-only build and vice versa.
- Rename `double_format_t` to `float_format_t` and `DoubleFormat*`
to `FloatFormat*` since the enum belongs to the `float_utils`
module, not the `double` type.
- Reject inputs that `sscanf` wrongly accepted: hex floats, `inf`,
`nan`, whitespace, missing decimal point, missing digits around
dot, and commas as decimal separator (per erlang/otp#9061).
- Merge `bin2float` and `list2float` test modules into a single
`string2float` test and expand coverage from 9 to 39 cases
including overflow, underflow, special values, format violations,
and edge cases.
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
Port otp_crypto to the PSA Crypto API used by mbedtls 4.x. - Replace deprecated low-level mbedtls APIs with PSA equivalents for hash, HMAC, cipher, and AEAD operations - Guard legacy mbedtls 2/3 code paths with version checks - Update CMake to detect mbedtls 4 and set HAVE_PSA_CRYPTO - Keep ESP32 JIT config outside mbedtls version guards Signed-off-by: Peter M <petermm@gmail.com>
Avoid including mbedtls/pkcs5.h when building against mbedtls 4, where that header is not available. Keep the existing PKCS5-based pbkdf2_hmac implementation for mbedtls 2/3, but switch the mbedtls 4 path to the PSA key derivation API so crypto:pbkdf2_hmac/5 remains available. Also reject zero iterations in PBKDF2 with a clear error message, and update the feature/NIF guards so pbkdf2_hmac stays registered on both legacy and mbedtls 4 builds. Signed-off-by: Peter M <petermm@gmail.com>
Improve PSA crypto resource management and memory safety: - Normalize do_psa_init() across all PSA-backed NIFs so every entry point initializes PSA consistently - Destroy PSA key handles immediately after finalization instead of deferring to GC, reducing key material residency time - Abort PSA operations and destroy keys on update failure to avoid dangling handles - Replace free() with secure_free() for all scratch buffers that may contain sensitive data (plaintext, key material) Signed-off-by: Peter M <petermm@gmail.com>
Declare the PSA output buffer size variables before any goto-based cleanup path can skip their initialization. This fixes Clang -Wsometimes-uninitialized failures in crypto_one_time/4-5 and crypto_update/2 when cleanup frees scratch buffers after early exits. Signed-off-by: Peter M <petermm@gmail.com>
Allocate at least 1 byte when the computed size is zero to avoid undefined malloc(0) behaviour on embedded libc implementations that may legally return NULL for zero-length allocations. This aligns the one-shot cipher and handle_iodata paths with the streaming cipher code that already guards against this case. Signed-off-by: Peter M <petermm@gmail.com> Amp-Thread-ID: https://ampcode.com/threads/T-019d2a7c-9760-707a-b24e-f6f6475a8608 Co-authored-by: Amp <amp@ampcode.com>
- Reset key attributes after psa_import_key in one-shot cipher path to match all other PSA import sites - Use secure_free for all crypto-adjacent buffers (sign/verify data, signature buffers, MAC data, AEAD AAD and combined buffers) to prevent sensitive data from lingering in freed memory - Reject AEAD decryption without a tag early with a clear error instead of letting it fail deep in PSA - Add finalized flag to MAC state so repeated mac_final/mac_update calls after finalization raise a clear error instead of a generic PSA failure - Document that ssl:nif_conf_rng is a no-op on mbedtls 4.x where PSA handles randomness internally Signed-off-by: Peter M <petermm@gmail.com>
Signed-off-by: Peter M <petermm@gmail.com>
Signed-off-by: Peter M <petermm@gmail.com>
46cb4cb to
5574dab
Compare
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
Summary by cubic
Adds
mbedtls@4via PSA Crypto while keeping existing crypto APIs, brings optional JIT DWARF debug/profiling, adds a new RISC‑V64 JIT backend, improves float parsing/formatting, introduces a generic Unix UART, and replaces the code loader with a signature‑driven one to cut binary size ~10%. Also addserlang:node/1anderts_internal:cmp_term/2, fixes term ordering and several crypto/bitstring/float edge cases, and expands CI.New Features
mbedtls@4via PSA Crypto; hash/cipher/HMAC/AEAD/RNG preserved;crypto:pbkdf2_hmac/5on v4 via PSA derivation; platforms init PSA at startup; CI builds exercise v4 on Linux, macOS, and FreeBSD.shorttoerlang:float_to_binary/2anderlang:float_to_list/2, extend ranges to{decimals, 0..253}/{scientific, 0..249};binary_to_float/1andlist_to_float/1now parse with strict, locale‑independent rules; exavmlib uses:short.socatto run loopback.opcodes.def) reduces binary size by ~10%.erlang:node/1,erts_internal:cmp_term/2; OTP 26 tests forallocate_zeroandmake_fun2. Git workflow/style guide added.Bug Fixes
mbedtls@4hardening: consistent PSA init; secure_free for sensitive buffers; reset key attrs; destroy keys on finalize; abort on update errors; block MAC updates after finalization; require AEAD tag; avoidmalloc(0); reject zero PBKDF2 iterations; fix uninitialized cleanup sizes; documentssl:nif_conf_rngas a no‑op on v4.bs_matchget_tail fix and correct binary encoding forint:24and similar sizes.binary_to_float/1andlist_to_float/1: fix overflow returninginfby raisingbadarg; parsing no longer depends on locale.net:getaddrinfotests. Updatecancel_timer/1spec/docs and add tests for post‑expiry cancellation.Written for commit 5574dab. Summary will update on new commits.