Stream: git-wasmtime

Topic: wasmtime / PR #3734 Fix a debug assertion in `externref` ...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:14):

fitzgen requested alexcrichton for a review on PR #3734.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:14):

fitzgen opened PR #3734 from externref-debug-assertion-fix to main:

See commit messages for details.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:22):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:22):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:22):

alexcrichton created PR review comment:

I don't think I understand why this is necessary, but was this required from fuzzing?

Unlike the wasmtime_activations_table_insert_with_gc function above the externref here is not on the stack in wasm, it's only on the stack here in Rust, and it's already rooted with the Some(externref) which should be an owned value.

I think that means that there's no need to manually insert it into the table because a stack walk otherwise won't find this (unless it would otherwise find it for some other reason)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:22):

alexcrichton created PR review comment:

This can end up doing a double-insertion between this and insert_with_gc below, and it might be good to avoid that? This method is only called when the table is full so insert_without_gc is already guaranteed to go straight to insertion in the slow path, so should a specialized method of the table be added for basically this use case?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:36):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:36):

fitzgen created PR review comment:

It won't insert into the bump chunk, so I don't think there is anything to worry about here. We just do one more hash lookup than is strictly necessary, but that is fine as this is a slow path anyways.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:45):

fitzgen created PR review comment:

No, this isn't required, it was just me being overly cautious. I'll remove it and force push.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:45):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 27 2022 at 22:46):

fitzgen updated PR #3734 from externref-debug-assertion-fix to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2022 at 00:53):

fitzgen updated PR #3734 from externref-debug-assertion-fix to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2022 at 14:21):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2022 at 14:21):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2022 at 14:21):

alexcrichton created PR review comment:

That's true, yeah. Could you update the comment here to indicate that this is doing some extra work but it's the slow path so it should be ok?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2022 at 17:47):

fitzgen updated PR #3734 from externref-debug-assertion-fix to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2022 at 19:03):

fitzgen merged PR #3734.

Last updated: Nov 22 2024 at 17:03 UTC