Open
Conversation
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>
146cf6d to
1f51a95
Compare
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>
1f51a95 to
7ce8135
Compare
Contributor
Oracle Review of the 3 Commits (opcodes.def X-macro, signature-driven loader, IMPL_CODE_LOADER removal)High Severity
Medium Severity
Low Severity
What Looks Good
|
pguyot
reviewed
Mar 25, 2026
| break; | ||
| default: | ||
| fprintf(stderr, "bs_match loader: unknown command %i\n", (int) term_to_atom_index(command)); | ||
| abort(); |
| if (opcode_signature == NULL) { | ||
| fprintf(stderr, "missing opcode: %i\n", (int) opcode); | ||
| fflush(stdout); | ||
| abort(); |
| default: { | ||
| fprintf(stderr, "unknown signature: %c\n", opcode_signature[arg_index]); | ||
| fflush(stdout); | ||
| abort(); |
|
|
||
| if (opcode_signature == NULL) { | ||
| fprintf(stderr, "missing opcode: %i\n", (int) opcode); | ||
| fflush(stdout); |
Collaborator
There was a problem hiding this comment.
flush stdout but fprint stderr.
|
|
||
| default: { | ||
| fprintf(stderr, "unknown signature: %c\n", opcode_signature[arg_index]); | ||
| fflush(stdout); |
Collaborator
There was a problem hiding this comment.
flush stdout but fprintf stderr
| } \ | ||
| } | ||
|
|
||
| #define DECODE_DEST_REGISTER_GC_SAFE(dreg, decode_pc) \ |
|
|
||
| #ifdef ENABLE_TRACE | ||
|
|
||
| // About X macro: https://en.wikipedia.org/wiki/X_macro |
Collaborator
There was a problem hiding this comment.
We don't need to repeat this.
| module_insert_line_ref_offset(mod, line_refs, line_ref, offset); | ||
| } | ||
|
|
||
| // About X macro: https://en.wikipedia.org/wiki/X_macro |
Collaborator
There was a problem hiding this comment.
We don't need to repeat this.
| const uint8_t *new_encoded = encoded; | ||
| uint32_t len; | ||
| DECODE_LITERAL(len, new_encoded); | ||
| // TODO: check this: actually should be enough: len = *(new_encoded)++ >> 4; |
Collaborator
There was a problem hiding this comment.
We have a lot of aborts, should we have one here?
|
|
||
| union opcode_arg | ||
| { | ||
| uint32_t u32_arg; |
Collaborator
There was a problem hiding this comment.
Do we need a union for this?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
size (bytes) | executable | description
x86_64 | 1018312 | src/AtomVM | stripped, -O3, OLD loader
x86_64 | 928200 | src/AtomVM | stripped, -O3, NEW loader
ESP32 | 1498203 | atomvm-esp32.bin | debug, -Og, OLD loader
ESP32 | 1418775 | atomvm-esp32.bin | debug, -Og, NEW loader
ESP32 | 1476281 | atomvm-esp32.bin | perf, -O2, OLD loader
ESP32 | 1402729 | atomvm-esp32.bin | perf, -O2, NEW loader
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