Stream: git-wasmtime

Topic: wasmtime / PR #4609 Shield compiled modules from their ap...

Wasmtime GitHub notifications bot (Aug 04 2022 at 14:57):

alexcrichton opened PR #4609 from fix-a-test to main:

This commit fixes #4600 in a somewhat roundabout fashion. Currently the
main branch of Wasmtime exhibits unusual behavior:

• If ./ci/run-tests.sh is run then the cache_accounts_for_opt_level
  test does not fail.

• If cargo test -p wasmtime --lib is run, however, then the test
  fails.

This test is indeed being run as part of ./ci/run-tests.sh and it's
also passing in CI. The exact failure is that part of the debuginfo
support we have takes an existing ELF image, copies it, and then appends
some information to inform profilers/gdb about the image. This code is
all quite old at this point and not 100% optimal, but that's at least
where we're at.

The problem is that the appended ProgramHeader64 is not aligned
correctly during cargo test -p wasmtime --lib, which is the panic that
happens causing the test to fail. The reason, however, that this test
passes with ./ci/run-tests.sh is that the alignment of
ProgramHeader64 is 1 instead of 8. The reason for that is that the
object crate has an unaligned feature which forcibly unaligns all
primitives to 1 byte instead of their natural alignment. During cargo test -p wasmtime --lib this feature is not enabled but during
./ci/run-tests.sh this feature is enabled. The feature is currently
enabled through inclusion of the backtrace crate which only happens
for some tests in some crates.

The alignment issue explains why the test fails on a single crate test
but fails on the whole workspace tests. The next issue I investigated
was if this test ever passed. It turns out that on v0.39.0 this test
passed, and the regression to main was introduced during #4571. That
PR, however, has nothing to do with any of this! The reason that this
showed up as causing a "regression" however is because it changed
cranelift settings which changed the size of serialized metadata at the
end of a Wasmtime cache object.

Wasmtime compiled artifacts are ELF images with Wasmtime-specific
metadata appended after them. This appended metadata was making its way
all the way through to the gdbjit image itself which mean that while the
end of the ELF file itself was properly aligned the space after the
Wasmtime metadata was not aligned. This metadata changes in size over
time as Cranelift settings change which explains why #4571 was the
"source" of the regression.

The fix in this commit is to discard the extra Wasmtime metadata when
creating an MmapVec representing the underlying ELF image. This is
already supported with MmapVec::drain so it was relatively easy to
insert that. This means that the gdbjit image starts with just the ELF
file itself which is always aligned at the end, which gets the test
passing with/without the unaligned feature in the object crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 09 2022 at 17:55):

alexcrichton requested peterhuene for a review on PR #4609.

Wasmtime GitHub notifications bot (Aug 09 2022 at 23:04):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 10 2022 at 14:58):

alexcrichton merged PR #4609.

Last updated: Nov 22 2024 at 16:03 UTC