MemoryPacking: Optimize overlapping segments on imported memories when provably in-bounds by JPL11 · Pull Request #8882 · WebAssembly/binaryen

JPL11 · 2026-07-06T16:02:09Z

Follow-up to #8834, implementing the TODO added there at @tlively's suggestion.

When active segments overlap and the memory is imported, the pass gave up entirely: a later out-of-bounds segment traps mid-instantiation and leaves the partially-written state visible in the imported memory (which outlives the failed instantiation), so even trampled data mattered.

However, if every active segment is provably in bounds of the memory's declared minimum size, no segment can trap during instantiation at all. Instantiation then always applies every segment, only the final memory contents are observable, and we can zero out trampled data exactly as we do for a memory defined in the module.

This is intentionally coarser than the full condition in the TODO (only the segments after a trampled segment, up to and including its trampling segment, need to be provably in-bounds): that would require tracking which coverage is usable per trampled byte, and the all-in-bounds case is the one that occurs in practice. The TODO is updated to describe the remaining refinement, and a test documents the conservative case.

Also removes the dead maxAddress computation in canOptimize (it has been unused for a while).

New tests cover: in-bounds trampling now optimized (zero and nonzero tramplers), a possibly-out-of-bounds trampler still bailing, and the conservative unrelated-out-of-bounds case.

…n provably in-bounds When active segments overlap and the memory is imported, the pass gave up entirely: a later out-of-bounds segment would trap mid-instantiation and leave the partially-written state visible in the imported memory, so even trampled data mattered. But if every active segment is provably in bounds of the memory's declared minimum size, no trap can happen during instantiation at all, so only the final memory contents are observable and we can zero out trampled data exactly as for a defined memory. This implements part of the TODO left in WebAssembly#8834; the remaining finer- grained condition (only the segments between a trampled segment and its trampler need to be in-bounds) is noted in an updated TODO. Also remove the dead maxAddress computation in canOptimize.

Copilot

Pull request overview

This PR extends the MemoryPacking pass to optimize overlapping active data segments on imported memories when it can prove that no active segment can trap during instantiation (i.e., all active segments are within the imported memory’s declared minimum size). It also updates lit tests to cover the new (and still-conservative) imported-memory overlap behavior.

Changes:

Allow trampling/overlap optimization on imported memories when all active segments are provably in-bounds of the declared minimum size.
Remove the dead maxAddress computation in MemoryPacking::canOptimize.
Add/extend lit tests for imported-memory overlap optimization and conservative bail-out cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/lit/passes/memory-packing_zero-filled-memory.wast	Adds new imported-memory overlap test cases and documents conservative bail-outs.
src/passes/MemoryPacking.cpp	Updates `canOptimize` to permit overlap optimization on imported memory when all active segments are provably in-bounds; removes unused code.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tlively · 2026-07-06T21:21:02Z

 ;; CHECK:      (import "env" "memory" (memory $0 1 1))
 (import "env" "memory" (memory $0 1 1))


No need for the CHECK-NOT; the absence of checks is enough because the checks are generated with update_lit_checks.py --all-items.

tlively · 2026-07-06T21:15:42Z

+          uint64_t memorySize = uint64_t(memory->initial)
+                                << memory->pageSizeLog2;


It would be reasonable to add an assertion that memory->initial is not larger than expected, but I wouldn't consider it critical.

I went one step further than an assertion: the maximal legal memory64 minimum (2^48 pages) is exactly the case where initial << pageSizeLog2 wraps to 0, so an assert on initial couldn't make the shift well-defined. Instead I factored the check into provablyInBounds(), which compares page counts (ceil(end / pageSize) <= initial) and never overflows.

While adding a test for that I noticed calculateRanges() had the same overflowing shift, plus a worse problem: it stored memory64 offsets in 32-bit Index, so a segment at e.g. offset 2^32+1024 on a one-page memory was truncated to 1024, "proven" in bounds, and its instantiation trap silently removed. Both call sites now use the shared helper, and there are regression tests for the maximal-memory64 and truncation cases.

memory->initial << memory->pageSizeLog2 is safe and aready used in many places.

initial memory verified during validation as

info.shouldBeTrue( memory->initial <= memory->maxSize64(), ... )

https://github.com/WebAssembly/binaryen/blob/main/src/wasm/wasm-validator.cpp#L5120

where maxSize64 defaine as:

Address::address64_t maxSize64() const { if (pageSizeLog2 == 0) { return std::numeric_limits<uint64_t>::max(); } return 1ull << (64 - pageSizeLog2); }

https://github.com/WebAssembly/binaryen/blob/main/src/wasm.h#L2646

It would be a good idea to create a method or utility that would immediately return totalBytes as memory->initial << memory->pageSizeLog2 with comment about invariant safety

The validation bound is inclusive though: initial <= maxSize64() allows initial == 2^48 for the default page size, and that is exactly the value where initial << pageSizeLog2 wraps to 0 in uint64. It is not a theoretical case: the test file in this PR contains (memory $0 i64 281474976710656 281474976710656) (2^48 pages) and it validates fine, which is why the comparison counts pages instead of bytes.

The two places you link have the same boundary behavior: at initial == 2^48, module-splitting computes parentSize == 0 and the shell calls resize(0). They just have not been noticed because nothing exercises the maximum there.

Agreed that a shared utility would be better than repeating the pattern. I would make it the comparison itself rather than a totalBytes() getter, since the byte count at the boundary (2^64) is not representable in uint64, so a getter would just move the wrap somewhere else. Something like Memory::isInBounds(offset, size) with the page counting logic from provablyInBounds here could then also replace the shifts in module-splitting and shell-interface. Happy to do that as a follow-up PR if there is interest, keeping this PR as is.

if pageSizeLog2 < 64 use as invariant than totalBytes may be actually pretty simple:

uint64_t totalBytes() const { uint64_t bytes = this->initial << this->pageSizeLog2; return bytes != 0 || this->initial == 0 ? bytes : UINT64_MAX; }

Yes, that works. Under the validator invariant the only value the shift can wrap to is exactly 0 (any initial below the maximum leaves at least one page of headroom), so the bytes != 0 || initial == 0 test catches precisely the boundary case. And saturating to UINT64_MAX is equivalent to the true 2^64 for any bounds check whose end is representable, which ours is since ckd_add already rejects the rest. Nice trick.

So both formulations are correct and it comes down to style, counting pages vs saturating bytes. Happy to switch provablyInBounds to your version or keep the page counting, whichever @tlively prefers. Either way the helper could move onto Memory in a follow-up and replace the shifts in module-splitting and shell-interface, where saturation would also turn the silent resize(0) into a loud failure instead.

I like that this totalBytes implementation is so simple, but it seems odd that it fudges the numbers and returns (2^64)-1 instead of (2^64) when you have a memory with byte size 2^64. I can imagine that would easily lead to off-by-one errors in edge cases.

I think a clearer version of provablyInBounds is the way to go.

Sounds good, kept provablyInBounds with page counting and made it clearer per your other two comments.

tlively · 2026-07-06T21:15:42Z

+          uint64_t memorySize = uint64_t(memory->initial)
+                                << memory->pageSizeLog2;


It would be reasonable to add an assertion that memory->initial is not larger than expected, but I wouldn't consider it critical.

tlively · 2026-07-06T21:17:57Z

+            uint64_t end;
+            if (seg->isActive() &&
+                (std::ckd_add(&end,
+                              seg->offset->cast<Const>()->value.getUnsigned(),


We can't assume that all the segments have Const offsets, can we? IIRC, they could also be global.gets of imported globals or simple arithmetic expressions. Unless I misremember, it would be good to add tests for non-Const offsets.

We can here, but only because of the guard earlier in canOptimize: when there are multiple segments, any active segment with a non-Const offset makes us return false before we ever reach this loop (the single-segment case returns even earlier). So by this point every active offset is known to be a Const.

You're right that there was no test locking that in for the imported-memory path, though — added one with a global.get of an imported global as the offset, verifying nothing is optimized.

tlively · 2026-07-06T21:19:18Z

+                << "warning: active memory segments have overlap, which "
+                << "prevents some optimizations.\n";


This warning should be updated to say that there is overlap as well as potentially out-of-bounds segments.

Done — it now says the segments overlap and some may be out of bounds of the imported memory's declared minimum size.

tlively · 2026-07-06T21:21:02Z

 ;; CHECK:      (import "env" "memory" (memory $0 1 1))
 (import "env" "memory" (memory $0 1 1))


No need for the CHECK-NOT; the absence of checks is enough because the checks are generated with update_lit_checks.py --all-items.

Factor the segment in-bounds proof into provablyInBounds(), comparing page counts rather than byte sizes so the maximal memory64 declared minimum (2^48 pages) cannot overflow the computation. Use it in calculateRanges() too, which had the same overflowing shift and also truncated memory64 offsets to 32-bit Index, wrongly proving out-of-bounds segments in bounds and removing their instantiation traps. Update the bail-out warning to mention possibly out-of-bounds segments. New tests: non-constant segment offsets on an imported memory, the maximal memory64 declared size, and the 32-bit truncation trap case.

tlively · 2026-07-10T00:02:25Z

+// Whether [offset, offset + size) provably fits in the declared minimum size
+// of the memory, so that writing the segment cannot trap. Compares page
+// counts rather than byte sizes, as the byte size of a maximal memory64
+// (2^48 pages) does not fit in 64 bits.


The offset and size operands are in bytes, right? This comment makes it sound like they might be in pages. Maybe move the part of the comment about using page sizes inside the function so it's clear that it's an implementation detail.

Done. The doc comment now only states the contract (byte range vs the declared minimum size), and the page counting rationale moved inside the function.

tlively · 2026-07-10T00:02:57Z

+  if (end & (memory.pageSize() - 1)) {
+    endPages++;
+  }


This could use a comment; I don't know what it is doing. If we need to adjust the calculation for the case of one-byte memory pages, then it's probably best to just do that in a separate arm of an if-else than to be clever with the arithmetic.

That line was computing ceil(end / pageSize), not a special case for one-byte pages: a segment ending partway into a page still needs that whole page to exist. Rewritten with end % memory.pageSize() != 0 plus a comment saying exactly that, which reads a lot better than the mask trick.

…e counting as an implementation detail

tlively · 2026-07-10T00:42:00Z

+  if (std::ckd_add(&end, offset, size)) {
+    return false;
+  }


IIUC, this will return false when the memory size == offset + size == 2^64, when it could return true. Would that be too complicated to fix?

Right, that corner was conservatively rejected: a maximal memory64 declares exactly 2^64 bytes, so a segment ending exactly at the memory end has a byte end that does not fit in uint64 and tripped the ckd_add bail. Not complicated to fix: on overflow, the mathematical end is 2^64 plus the wrapped value, which is in bounds only when the wrapped value is 0 and the declared minimum is the maximal size. (With one byte pages the largest declarable minimum is 2^64 - 1, so that case stays false, which also avoids a shift by 64.) Pushed, with a test where a segment at (i64.const -1) of size 1 no longer blocks the overlap optimization on a maximal memory64.

A maximal memory64 declares 2^48 pages, exactly 2^64 bytes, so a segment ending exactly at the memory end has a byte end that does not fit in uint64. Previously we conservatively treated the ckd_add overflow as not provable; now we prove it in bounds when the wrapped end is exactly 0 and the declared minimum is the maximal size.

tlively

Thanks!

JPL11 requested a review from a team as a code owner July 6, 2026 16:02

JPL11 requested review from Copilot and tlively and removed request for a team July 6, 2026 16:02

Copilot started reviewing on behalf of JPL11 July 6, 2026 16:02 View session

Copilot AI reviewed Jul 6, 2026

View reviewed changes

tlively reviewed Jul 6, 2026

View reviewed changes

JPL11 added 2 commits July 6, 2026 15:37

Fix clang-format

32c8b3d

tlively reviewed Jul 10, 2026

View reviewed changes

Clarify provablyInBounds: byte-range contract in the doc comment, pag…

abaeeef

…e counting as an implementation detail

tlively reviewed Jul 10, 2026

View reviewed changes

tlively approved these changes Jul 10, 2026

View reviewed changes

tlively merged commit db30c15 into WebAssembly:main Jul 10, 2026
16 checks passed

		;; CHECK: (import "env" "memory" (memory $0 1 1))
		(import "env" "memory" (memory $0 1 1))

		uint64_t memorySize = uint64_t(memory->initial)
		<< memory->pageSizeLog2;

		<< "warning: active memory segments have overlap, which "
		<< "prevents some optimizations.\n";

Uh oh!

Conversation

JPL11 commented Jul 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGraey Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGraey Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MaxGraey Jul 9, 2026 •

edited

Loading

MaxGraey Jul 9, 2026 •

edited

Loading