Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$hp is initialised to incorrect value. #322

Closed
otrho opened this issue May 25, 2022 · 4 comments · Fixed by #464
Closed

$hp is initialised to incorrect value. #322

otrho opened this issue May 25, 2022 · 4 comments · Fixed by #464
Assignees
Labels
comp:FVM Component: FuelVM

Comments

@otrho
Copy link

otrho commented May 25, 2022

At VM initialization it says:

$hp = VM_MAX_RAM - 1: the heap area begins at the top and is empty to start.

It makes more sense for $hp to be initialised to just VM_MAX_RAM. It always essentially points to either non-addressable memory or previously allocated buffers (via ALOC), both of which are essentially 'reserved'.

@adlerjohn
Copy link
Contributor

This change would avoid having to offset the value by 1 to use allocated memory, right?

@adlerjohn adlerjohn added the comp:FVM Component: FuelVM label May 25, 2022
@otrho
Copy link
Author

otrho commented May 25, 2022

Yep. Doing an aloc would be just:

let buf_addr = asm(size: size) {
    aloc size;
    hp: u64
}

...as per the discussion in FuelLabs/sway#1627

@adlerjohn
Copy link
Contributor

@Voxelot is there general consensus to go forward with this change?

@Voxelot
Copy link
Member

Voxelot commented Feb 27, 2023

There is from the client-team side. @otrho also mentioned that the sway team could make a future-compatible workaround so that this change won't immediately break previously compiled code as well. But this workaround may not be needed if we do the change sooner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:FVM Component: FuelVM
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants