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

Add callback to handle memory.grow failures #2522

Merged

Conversation

eloparco
Copy link
Contributor

When embedding WAMR, this PR allows to register a callback that is invoked when memory.grow fails.

In case of memory allocation failures, some languages allow to handle the error (e.g. by checking the return code of malloc/calloc in c), some others (e.g. Rust) just panic. For the latter, it is useful to have a callback function that would be called before the wasm execution crashes, allowing the embedder to detect and handle the memory.grow error. It comes in handy when using shared memory since shared memory forces the maximum linear memory size to be set at compilation time. Setting a value that is too low can lead to memory allocation failures (i.e. memory.grow failures).

It would be nice to capture the crash in Rust directly, but the feature is experimental (rust-lang/rust#51540).

@yamt
Copy link
Collaborator

yamt commented Aug 31, 2023

allowing the embedder to detect and handle the memory.grow error

do you mean just printing a diagnostic message?
or are you thinking about more interesting error handling?

@eloparco
Copy link
Contributor Author

do you mean just printing a diagnostic message?
or are you thinking about more interesting error handling?

My use case is more about logging, storing some stats and doing some cleanup in the embedder. Without that, the execution can crash at any point (in which there's an allocation) and from the embedder there won't be any way to automatically understand that the cause is the memory.grow failure.
I guess there are other possible scenarios and exposing the callback can unlock them.

@yamt
Copy link
Collaborator

yamt commented Aug 31, 2023

do you mean just printing a diagnostic message?
or are you thinking about more interesting error handling?

My use case is more about logging, storing some stats and doing some cleanup in the embedder. Without that, the execution can crash at any point (in which there's an allocation) and from the embedder there won't be any way to automatically understand that the cause is the memory.grow failure. I guess there are other possible scenarios and exposing the callback can unlock them.

for logging purpose, i guess it's better to provide more info like:

  • context
    • wasm_exec_env_t
  • memory
    • module instance
    • memory idx (currently always 0 though)
    • current size of the memory
  • the cause of the failure

@eloparco eloparco force-pushed the eloparco/memory-grow-callback branch from 8ec6b66 to bc58296 Compare August 31, 2023 23:04
@Zzzabiyaka
Copy link
Contributor

just a general note: is there any specific reason why we need goto statement here?

why cannot we call a separate function and then just return false?

maybe I'm missing some context

@eloparco eloparco force-pushed the eloparco/memory-grow-callback branch from bc58296 to 5cfa33a Compare September 2, 2023 23:03
@eloparco
Copy link
Contributor Author

eloparco commented Sep 2, 2023

just a general note: is there any specific reason why we need goto statement here?

It just looked cleaner than calling enlarge_memory_error_cb() in all the places where wasm_enlarge_memory_internal() returns.

@eloparco eloparco force-pushed the eloparco/memory-grow-callback branch from 5cfa33a to c3cd8ae Compare September 3, 2023 10:04
core/iwasm/aot/aot_runtime.c Outdated Show resolved Hide resolved
core/iwasm/common/wasm_memory.c Outdated Show resolved Hide resolved
core/iwasm/common/wasm_memory.c Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_runtime.c Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_runtime.h Outdated Show resolved Hide resolved
@eloparco eloparco force-pushed the eloparco/memory-grow-callback branch from 24e1a12 to 4ca1a77 Compare September 4, 2023 22:36
@eloparco eloparco force-pushed the eloparco/memory-grow-callback branch from 4ca1a77 to 136c50a Compare September 5, 2023 00:30
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@yamt yamt left a comment

Choose a reason for hiding this comment

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

lgtm

@wenyongh wenyongh merged commit 709127d into bytecodealliance:main Sep 5, 2023
368 checks passed
@lum1n0us
Copy link
Collaborator

lum1n0us commented Sep 5, 2023

Without that, the execution can crash at any point (in which there's an allocation) and from the embedder there won't be any way to automatically understand that the cause is the memory.grow failure.

Be curious about the use case. Does it like

  • memory.grow failed in runtime
  • rust(user code or std) detects the failure and panic!
  • embedded doesn't know anything but WASM app exits.

I guess there are other possible scenarios and exposing the callback can unlock them.

It seems like a general global exception handler with variants exceptions is better than a set of callbacks for every possible scenarios.

@eloparco
Copy link
Contributor Author

eloparco commented Sep 5, 2023

Be curious about the use case. Does it like

  • memory.grow failed in runtime
  • rust(user code or std) detects the failure and panic!
  • embedded doesn't know anything but WASM app exits.

Yes, that's my use case

It seems like a general global exception handler with variants exceptions is better than a set of callbacks for every possible scenarios.

Do you mean to implement a general global exception handler in the runtime?
The best option would probably be to override the exception handler for memory allocation in Rust, but that feature is experimental. The problem doesn't appear in C since there we can just check the return code of malloc/realloc in user code.

@lum1n0us
Copy link
Collaborator

lum1n0us commented Sep 6, 2023

The problem doesn't appear in C since there we can just check the return code of malloc/realloc in user code.

Does the uncheckable exception only happen during allocation or it is a general behaviour in Rust? Is it safe to say panic!->abort() (Rust->Wasm) is the root cause of such scenarios?

@eloparco
Copy link
Contributor Author

eloparco commented Sep 8, 2023

Does the uncheckable exception only happen during allocation or it is a general behaviour in Rust?

Yes, the one that I was trying to address happens in Rust whenever a memory allocation fails. It can be raised by any library included (e.g. standard library) and currently there's no way to catch it. To reproduce it, you can just set the maximum linear memory size to a small value and compile some Rust code that allocates memory.

Is it safe to say panic!->abort() (Rust->Wasm) is the root cause of such scenarios?

I think so.

@loganek
Copy link
Collaborator

loganek commented Sep 8, 2023

I think it's worth to mention in this thread that (a cross-runtime) alternative for that would be to add a compiler pass that wraps memory.grow opcode with some additional error checking logic. And that can be extended to handle any other future usecases.

@lum1n0us
Copy link
Collaborator

Is it safe to say the embedded will meet an un-handled exception when the rust code hit a panic!? no matter it is caused by allocation failure, out-of-length index accessing or anything else.

If so, shall we avoid panic! especially when plan to be transferred to Wasm? Maybe use Result<T,E> as a return instead

@eloparco
Copy link
Contributor Author

If so, shall we avoid panic! especially when plan to be transferred to Wasm? Maybe use Result<T,E> as a return instead

The panic! is not called explicitly (in the user code compiled to wasm) but happens whenever Rust fails memory allocation. Failure to allocate memory is an unrecoverable error in Rust.

@lum1n0us
Copy link
Collaborator

Is it safe to say the embedded will meet an un-handled exception when the rust code hit a panic! no matter if it is called by user code(compiled to wasm) or not.

@eloparco
Copy link
Contributor Author

Is it safe to say the embedded will meet an un-handled exception when the rust code hit a panic! no matter if it is called by user code(compiled to wasm) or not.

Yes

victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
When embedding WAMR, this PR allows to register a callback that is
invoked when memory.grow fails.

In case of memory allocation failures, some languages allow to handle
the error (e.g. by checking the return code of malloc/calloc in C), some
others (e.g. Rust) just panic.
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

Successfully merging this pull request may close these issues.

None yet

6 participants