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.
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.
SingleAccretion submitted PR review.
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.
SingleAccretion updated PR #9470.
SingleAccretion updated PR #9470.
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
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.
SingleAccretion updated PR #9470.
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.
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.
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.
SingleAccretion has marked PR #9470 as ready for review.
SingleAccretion requested wasmtime-core-reviewers for a review on PR #9470.
SingleAccretion requested pchickey for a review on PR #9470.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Reading over this again, mind removing the
ManualllyDrop
wrapper here? This is already paying the cost of theOption
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 theDrop
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)
SingleAccretion submitted PR review.
SingleAccretion created PR review comment:
While we're on the topic - I don't quite get this manual
drop
ping. Wouldn't reorderingmmap
to be after the registrations work equivalently?
alexcrichton submitted PR review.
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
andunwind_registration
are justOption
though it would be fine to remove all theManuallyDrop
here and theDrop
implementation would just belet _ = self.field.take()
for the two registrations.
SingleAccretion updated PR #9470.
alexcrichton merged PR #9470.
Last updated: Dec 23 2024 at 12:05 UTC