Skip to content

core/config.h: Bump the default WASM_STACK_GUARD_SIZE #3344

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 23, 2024
Merged

core/config.h: Bump the default WASM_STACK_GUARD_SIZE #3344

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e1fffc
core/config.h: Bump the default WASM_STACK_GUARD_SIZE
yamt Apr 22, 2024
0e6dfa1
Revert WASM_STACK_GUARD_SIZE to 1024 for non-posix platforms
yamt Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion core/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,26 @@
/* UVWASI requires larger native stack */
#define WASM_STACK_GUARD_SIZE (4096 * 6)
#else
#define WASM_STACK_GUARD_SIZE (1024)
/*
* Use a larger default for platforms like macOS/Linux.
*
* For example, wasm_interp_call_func_bytecode + wasm_runtime_set_exception
* would consume >4KB stack on x86-64 macOS.
*
* Although product-mini/platforms/nuttx always overrides
* WASM_STACK_GUARD_SIZE, exclude NuttX here just in case.
*/
#if defined(__APPLE__) || (defined(__unix__) && !defined(__NuttX__))
#define WASM_STACK_GUARD_SIZE (1024 * 5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to increase the size only for linux/macos or posix like platforms? Seems that not all platforms need so much guard stack space, e.g. zephyr/esp-idf 32-bit, had better keep it unchanged for them now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, maybe we can also decrease the native stack usage by refining the code which calls snprintf? For example, in wasm_set_exception_local and set_exception_visitor, copy character one by one from source string until '\0' is found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?

I am not sure, my suggestion is to keep it unchanged temporarily, and change it after we do more test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ps. keep it unchanged for non-posix like platforms until we do more test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?

I am not sure, my suggestion is to keep it unchanged temporarily, and change it after we do more test.

i can probably investigate the situation on xtensa/nuttx. do you think it's good enough to make it represent the "low end" side?

OK, thanks. Distinguish it into high end side and low end side is good to me, do yo think we should put the macro in each platform's platform_internal.h?

maybe. after all, it's very platform dependent.
otoh, i suspect that embedders for very small devices usually fine-tune WASM_STACK_GUARD_SIZE for their specific applications anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, maybe we can also decrease the native stack usage by refining the code which calls snprintf? For example, in wasm_set_exception_local and set_exception_visitor, copy character one by one from source string until '\0' is found.

i agree it's probably a good idea to reduce the stack usage of wasm_runtime_set_exception and friends.

but i'm not sure if we can reduce WASM_STACK_GUARD_SIZE with it unless we introduce a rather strong policy about where os_printf, LOG_DEBUG, etc can be used.

Yes, I think we can increase WASM_STACK_GUARD_SIZE to 5KB and also refine the code in some places to enhance the security. And decrease WASM_STACK_GUARD_SIZE only when we do a full test to make sure it is good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe for 32-bit archs it can be a bit smaller. but are you sure 1KB is enough?

I am not sure, my suggestion is to keep it unchanged temporarily, and change it after we do more test.

i can probably investigate the situation on xtensa/nuttx. do you think it's good enough to make it represent the "low end" side?

OK, thanks. Distinguish it into high end side and low end side is good to me, do yo think we should put the macro in each platform's platform_internal.h?

maybe. after all, it's very platform dependent. otoh, i suspect that embedders for very small devices usually fine-tune WASM_STACK_GUARD_SIZE for their specific applications anyway.

OK, so had better keep putting the macro in core/config.h, and maybe we can add a document for detailed description about this macro and the native stack overflow check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted for non-posix platforms for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe. after all, it's very platform dependent. otoh, i suspect that embedders for very small devices usually fine-tune WASM_STACK_GUARD_SIZE for their specific applications anyway.

Agree.

#else
/*
* Otherwise, assume very small requirement for now.
*
* Embedders for very small devices likely fine-tune WASM_STACK_GUARD_SIZE
* for their specific applications anyway.
*/
#define WASM_STACK_GUARD_SIZE 1024
#endif
#endif
#endif

Expand Down