fix(bindgen): ensure moving storage pointers by abi size#1452
Merged
vados-cosmonic merged 1 commit intoMay 8, 2026
Merged
Conversation
vados-cosmonic
approved these changes
May 8, 2026
Collaborator
vados-cosmonic
left a comment
vados-cosmonic
left a comment
There was a problem hiding this comment.
LGTM 🚀
Thanks for fixing this and updating the tests -- glad those bits were easy to extend. Tests look good to me (and feel free to refactor/aggressively cut if any of it awkward)
The patch includes lower and lift side fixes. The lower side fix is basically: treat ctx.storagePtr as a canon abi cursor and move it by abi size rather than stopping where nested lift ended. The general pattern is this: - align cursor - remember slot start - lower nested value - advance cursor to at least slot start + abi slot size The last step is really a gist of it, because nested can values can write less than their abi slot size. For example for records that looks like this: ``` const originalPtr = ctx.storagePtr; for each field: align ctx.storagePtr to field align fieldPtr = ctx.storagePtr lower field ctx.storagePtr = Math.max(ctx.storagePtr, fieldPtr + fieldSize) ctx.storagePtr = Math.max(ctx.storagePtr, originalPtr + recordSize) align ctx.storagePtr to record align ``` The lifting is basically the same but it accounts for direct params reading so the algorithm annoyingly checks if pointers are not undefined. I think there is a refactor opportunity here: split direct params and memory indirection modes but I wanted to focus on correctness fixes here. The transpiler fix just ensures that each nested element gets its own abi metadata rather than using parent metadata: so for instance record gets fields meta and tuples get each element meta.
andreiltd
force-pushed
the
fix/flat-variant-lowering
branch
from
274c0c1 to
124604e
Compare
Collaborator
|
NOTE: I'm force merging this as the failures in CI (on only macOS) seem to be unrelated: |
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
2 participants
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.
The patch includes lower and lift side fixes. The lower side fix is basically: treat
ctx.storagePtras a canon abi cursor and move it by abi size rather than stopping where nested lift ended.The general pattern is this:
The last step is really a gist of it, because nested values can write less than their abi slot size.
For example for records that looks like this:
The lifting is basically the same but it accounts for direct params reading so the algorithm annoyingly checks if pointers are not undefined. I think there is a refactor opportunity here: split direct params and memory indirection modes but I wanted to focus on correctness fixes here.
The transpiler fix just ensures that each nested element gets its own abi metadata rather than using parent metadata: so for instance record gets fields meta and tuples get each element meta.
I first across this issue for descriptor types:
record directory-entry { type: variant { directory, file, symlink, unknown, ... }, name: string, }If the variant case has no payload, old lowering/lifting left the cursor too early, and the following
name: stringwas read/written at the wrong offset.