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

Do not clone stack frames if there's no trap #3008

Conversation

eloparco
Copy link
Contributor

When running the no_pthread sample, the assert was failing on src->num_elems != 0.
That's because, if the exception is proc_exit, there's no trap (the execution didn't fail, no stack frames).

The problem was not detected in CI because there we build in release mode:

cmake --build . --config Release --parallel 4

Should we use debug instead so that we can take advantage of asserts?

@wenyongh
Copy link
Contributor

When running the no_pthread sample, the assert was failing on src->num_elems != 0. That's because, if the exception is proc_exit, there's no trap (the execution didn't fail, no stack frames).

The problem was not detected in CI because there we build in release mode:

cmake --build . --config Release --parallel 4

Should we use debug instead so that we can take advantage of asserts?

Yes, agree, it should be easier to expose issues, how about using cmake --build . --config Debug --parallel 4 for some other samples?

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

@@ -1924,7 +1924,8 @@ wasm_frame_func_offset(const wasm_frame_t *frame)
void
wasm_frame_vec_clone_internal(Vector *src, Vector *out)
{
bh_assert(src->num_elems != 0 && src->data);
if (src->num_elems == 0)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Had better to bh_vector_destory(out) before return to free its possible frames and zero-initialize out? For example, an instance's frames may be created previously and then wasi_proc_exit occurs.

BTW, in L1932, seems no need to destroy out again if bh_vector_init fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, in L1932, seems no need to destroy out again if bh_vector_init fails?

Right, changed to make it similar to

if (!bh_vector_destroy(module_inst->frames)

@eloparco eloparco force-pushed the eloparco/fix-crash-when-trap-stack-trace-empty branch 2 times, most recently from e32ff1c to 0d6b30f Compare January 15, 2024 09:13
@eloparco eloparco force-pushed the eloparco/fix-crash-when-trap-stack-trace-empty branch from 0d6b30f to bcbb0df Compare January 15, 2024 09:26
@wenyongh wenyongh merged commit 892a94f into bytecodealliance:main Jan 15, 2024
393 checks passed
wenyongh pushed a commit that referenced this pull request Jan 16, 2024
Follow-up on #3008. Compiling samples in Debug mode allows us to
take advantage of asserts and would have prevented the fix in #3008.
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
…dealliance#3008)

When running the wasi-threads no_pthread sample, the assert was failing
on `src->num_elems != 0` in debug mode, it is because that the exception
is `proc_exit`, there is no trap (the execution didn't fail, no stack frames):
https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/samples/wasi-threads/wasm-apps/no_pthread.c
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Follow-up on bytecodealliance#3008. Compiling samples in Debug mode allows us to
take advantage of asserts and would have prevented the fix in bytecodealliance#3008.
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

2 participants