Stream: git-wasmtime

Topic: wasmtime / PR #9470 Move JIT debug image registration to ...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 15 2024 at 21:50):

SingleAccretion opened PR #9470 from SingleAccretion:DI-Dups to bytecodealliance:main:

JIT images correspond to ELF images, which may represent multiple modules within a single component.

Fixes #9461.

Tested manually on moderately large component input as well as with the existing LLDB tests.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 15 2024 at 21:51):

SingleAccretion edited PR #9470:

JIT images correspond to ELF images, which may represent multiple modules within a single component.

Fixes #9461.

Tested manually on moderately large component as well as with the existing LLDB tests.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 15 2024 at 21:54):

SingleAccretion submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 15 2024 at 21:54):

SingleAccretion created PR review comment:

Since these tests are #[ignore]d by default, and run fine on Windows when explicitly requested, it seems there is no reason to have these guards anymore.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 15 2024 at 21:55):

SingleAccretion updated PR #9470.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 15 2024 at 21:56):

SingleAccretion updated PR #9470.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 02:19):

alexcrichton submitted PR review:

Thanks for the PR as well as for the issue! This looks like the right fix to me. I suspect that writing a test for this will be relatively hard, so otherwise having the preexisting tests pass I think is sufficient for this.

I also think that the bug you highlighted where we're double-registering with the native profiler is probably also an issue as well. If you'd like that's ok to defer to a second PR to fix, or if you'd like bundling it in here I think would be reasonable too

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 11:45):

SingleAccretion edited PR #9470:

JIT images correspond to ELF images, which may represent multiple modules within a single component.

Fixes #9461.

Tested manually on a moderately large component as well as with the existing LLDB tests.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:01):

SingleAccretion updated PR #9470.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:05):

SingleAccretion commented on PR #9470:

I also think that the bug you highlighted where we're double-registering with the native profiler is probably also an issue as well.

Opened #9478 about this.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:06):

SingleAccretion edited a comment on PR #9470:

I also think that the bug you highlighted where we're double-registering with the native profiler is probably also an issue as well.

Opened #9478 about this.

The fix there is going to be a little trickier since the profiler "get name" callback operates on actual per-module state.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:07):

SingleAccretion edited a comment on PR #9470:

I also think that the bug you highlighted where we're double-registering with the native profiler is probably also an issue as well.

Opened #9478 about this.

The fix there is going to be a little trickier since the profiler's "get name" callback operates on actual per-module state.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:13):

SingleAccretion has marked PR #9470 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:13):

SingleAccretion requested wasmtime-core-reviewers for a review on PR #9470.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:13):

SingleAccretion requested pchickey for a review on PR #9470.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:22):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:22):

alexcrichton created PR review comment:

Reading over this again, mind removing the ManualllyDrop wrapper here? This is already paying the cost of the Option overhead so I don't think the unsafe drop part is buying a whole lot here.

The destructor can still be early executed through let _ = self.debug_registration.take(); in the Drop implementation though.

(I realize you were probably copying unwind_registration and I think we should apply this change there too, as a separate PR if you'd prefer to leave that as-is for now)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:30):

SingleAccretion submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:30):

SingleAccretion created PR review comment:

While we're on the topic - I don't quite get this manual dropping. Wouldn't reordering mmap to be after the registrations work equivalently?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:37):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:37):

alexcrichton created PR review comment:

I think so yeah, but the drop order of fields with respect to field orderings in structs is pretty subtle so we've occasionally used ManuallyDrop to make it more clear that the drop order changing is an intentional change.

Once both debug_registration and unwind_registration are just Option though it would be fine to remove all the ManuallyDrop here and the Drop implementation would just be let _ = self.field.take() for the two registrations.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:52):

SingleAccretion updated PR #9470.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:19):

alexcrichton merged PR #9470.

Last updated: Oct 23 2024 at 20:03 UTC