Stream: git-wasmtime

Topic: wasmtime / PR #1513 Expose memory-related options in `Con...

Wasmtime GitHub notifications bot (Apr 15 2020 at 14:47):

alexcrichton opened PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 15 2020 at 17:04):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 17:04):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 17:04):

fitzgen created PR Review Comment:

"emit" -> "omit"?

Wasmtime GitHub notifications bot (Apr 15 2020 at 18:03):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 15 2020 at 18:19):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 17 2020 at 22:24):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 20 2020 at 20:59):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 21 2020 at 18:37):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 21 2020 at 18:37):

alexcrichton requested sunfishcode for a review on PR #1513.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

typo: a -> at

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

Code-quote LinearMemory.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

Because wasm sometimes measures sizes in pages, we should clarify that this is a size in bytes.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

This first-to-last property is interesting, as optimizers could be tempted to optimize memory accesses in ways that could change the access order -- for example optimizing load.i32(x) & 0xff000000 into a one-byte load from the last byte. Could you add some text to https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/docs/ir.md#memory-operation-flags that says that when an access is not notrap, the bytes are defined to be accessed first-to-last, so that we at least document it?

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

One subtlety that would be good to clarify here is that this is talking about reserving address space, not necessarily actual memory.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

This could produce a value which is lower than the given val, but I guess that's ok here because it'd be an unusably large value in either case.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

"size in bytes", and so on below.

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

sunfishcode created PR Review Comment:

The page size will always be a power of 2, so we can do val & -page_size.

Wasmtime GitHub notifications bot (Apr 22 2020 at 15:12):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 22 2020 at 15:22):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 15:22):

alexcrichton created PR Review Comment:

So thinking more about this, I think I just carried this comment over from somewhere that I didn't fully understand. I think this property isn't actually necessary (and may not be necessary to document).

The main assumption I think we can make is that all our accesses are smaller than the size of a page (e.g. the minimum size of the guard region). In that case the first byte is either:

• in wasm memory which is normal-bounds-checked and all remaining bytes are either in wasm memory or the guard-page
• in the guard page, in which case the bounds check will fail.

Does that sound right to you as well? If so I'll reword this to and probably throw in an assert that the width is less than 1k or something like that with a doc comment explaining.

Wasmtime GitHub notifications bot (Apr 22 2020 at 15:22):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 15:22):

alexcrichton created PR Review Comment:

Ah true yeah, but I was mostly just striving to do something reasonable-ish here.

Wasmtime GitHub notifications bot (Apr 22 2020 at 15:24):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 23 2020 at 01:27):

eust-dfinity submitted PR Review.

Wasmtime GitHub notifications bot (Apr 23 2020 at 01:27):

eust-dfinity created PR Review Comment:

Actually that example is allocating one guard page

Wasmtime GitHub notifications bot (Apr 23 2020 at 15:31):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 23 2020 at 15:31):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 23 2020 at 15:31):

alexcrichton created PR Review Comment:

I went ahead and tried to update the comment with my thinking here.

Wasmtime GitHub notifications bot (Apr 23 2020 at 15:32):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 23 2020 at 15:32):

alexcrichton created PR Review Comment:

True yeah, I was mostly just getting this test to pass. Since the JIT is configured that there might not be a guard page then the guard page allocation here shouldn't be necessary.

Wasmtime GitHub notifications bot (Apr 29 2020 at 19:33):

alexcrichton updated PR #1513 from configurable-memory-bounds to master:

This commit was initially motivated by looking more into #1501, but it
ended up balooning a bit after finding a few issues. The high-level
items in this commit are:

• New configuration options via wasmtime::Config are exposed to
  configure the tunable limits of how memories are allocated and such.

• The MemoryCreator trait has been updated to accurately reflect the
  required allocation characteristics that JIT code expects.

• A bug has been fixed in the cranelift wasm code generation where if no
  guard page was present bounds checks weren't accurately performed.

The new Config methods allow tuning the memory allocation
characteristics of wasmtime. Currently 64-bit platforms will reserve 6GB
chunks of memory for each linear memory, but by tweaking various config
options you can change how this is allocate, perhaps at the cost of
slower JIT code since it needs more bounds checks. The methods are
intended to be pretty thoroughly documented as to the effect they have
on the JIT code and what values you may wish to select. These new
methods have been added to the spectest fuzzer to ensure that various
configuration values for these methods don't affect correctness.

The MemoryCreator trait previously only allocated memories with a
MemoryType, but this didn't actually reflect the guarantees that JIT
code expected. JIT code is generated with an assumption about the
minimum size of the guard region, as well as whether memory is static or
dynamic (whether the base pointer can be relocated). These properties
must be upheld by custom allocation engines for JIT code to perform
correctly, so extra parameters have been added to
MemoryCreator::new_memory to reflect this.

Finally the fuzzing with Config turned up an issue where if no guard
pages present the wasm code wouldn't correctly bounds-check memory
accesses. The issue here was that with a guard page we only need to
bounds-check the first byte of access, but without a guard page we need
to bounds-check the last byte of access. This meant that the code
generation needed to account for the size of the memory operation
(load/store) and use this as the offset-to-check in the no-guard-page
scenario. I've attempted to make the various comments in cranelift a bit
more exhaustive too to hopefully make it a bit clearer for future
readers!

Closes #1501

<!--

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 (Apr 30 2020 at 00:10):

sunfishcode merged PR #1513.

Wasmtime GitHub notifications bot (May 05 2020 at 19:16):

plicease submitted PR Review.

Wasmtime GitHub notifications bot (May 05 2020 at 19:16):

plicease created PR Review Comment:

After testing this from Perl, I think the _set suffix was accidentally omitted from this declaration. Same for wasmtime_config_dynamic_memory_guard_size below.

Last updated: Nov 22 2024 at 16:03 UTC