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

Stack size as a property of an instance #194

Open
s0me0ne-unkn0wn opened this issue Oct 15, 2024 · 3 comments
Open

Stack size as a property of an instance #194

s0me0ne-unkn0wn opened this issue Oct 15, 2024 · 3 comments

Comments

@s0me0ne-unkn0wn
Copy link

Currently, stack size is defined as a property of program blob/module. There are two problems with that:

  1. The default value is too small without any obvious way to bump it (there's logic that allows setting it from the value stored in .polkavm_min_stack_size ELF section, but IIUC no means are provided for end developers to actually set this value);
  2. To ensure deterministic execution in a parametrized execution environment, the stack size should be a property of an instance, not a module.

I'd propose to replace (or supplement) the Module::instantiate() with something like Module::instance_builder().with_stack_size(...).build().

If we agree on the necessity of the implementation described and on the interface, I'm ready to work on it.

@koute
Copy link
Collaborator

koute commented Oct 15, 2024

  1. The default value is too small without any obvious way to bump it (there's logic that allows setting it from the value stored in .polkavm_min_stack_size ELF section, but IIUC no means are provided for end developers to actually set this value);

I haven't added a macro to set it, but it should be relatively simple to add one in the polkavm_derive crate.

To ensure deterministic execution in a parametrized execution environment, the stack size should be a property of an instance, not a module.

Hm? I don't understand. How is making it a property of the instance non-deterministic? (:

If anything I imagine that making it a property of the module is the way to make it deterministic, because the module itself should know how much stack it needs, and not the execution environment (so the execution environment never needs to know how much stack space the module needs).

@s0me0ne-unkn0wn
Copy link
Author

Well, when you put it like that, I'm starting to argue my own point 🤔

I'm thinking about the PVF use case, of course, and we always wanted the stack size to be deterministically controllable by the environment. First, because we wanted to have a hard limit for that, and second, because we wanted to be able to bump that limit for the whole environment if needed.

But if we make it module-defined... Well, okay, we can rule out modules going over the hard limit on pre-checking. We also would need a parachain to perform a runtime upgrade if it finds it needs more stack -- but that, indeed, increases granularity. Do we really need it to be environment-controlled? Let me think a bit more about that.

In the meantime, I'll try to add a macro to define the stack size. One page is obviously way too small even for a simplest PVFs (one more little bug report is that the stack overflow looks like a nameless "Trap" without debug enabled, and even with full tracing you need quite some time to figure out that the out-of-bound memory region write is indeed a stack overflow).

@koute
Copy link
Collaborator

koute commented Oct 16, 2024

one more little bug report is that the stack overflow looks like a nameless "Trap" without debug enabled, and even with full tracing you need quite some time to figure out that the out-of-bound memory region write is indeed a stack overflow

Yes, because you do get a trap on any out of bounds memory access (and unlike WASM the stack here is just normal memory), and the VM currently doesn't print out which exact memory address was accessed. So this is not a bug, just a missing feature. It wouldn't be that hard for the VM to also grab the address which the guest tried to access, check whether it's near the stack, and print out that it's most likely a stack overflow.

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

No branches or pull requests

2 participants