There is an issue with theinitialPages calculation ininitDefaultMemory, however. The existing condition to run that calculation depends onmemorySegments.length, which is now zero if onlymemory.data(n: i32) is called, as in the memory-config-errors test.
Making that calculation occur regardless of the number of memory segments would cause a separate issue: due to the default memory offsets ofoptions.memoryBase,1024, or8, modules that previously compiled with an initial memory of 0 pages would compile with a minimum of 1 page.

I've read the contributing guidelines
I've added my name and email to the NOTICE file

CountBleck commented

Mar 25, 2024

View reviewed changes

src/compiler.ts OutdatedShow resolvedHide resolved

Copy link

Member

dcodeIO commentedMar 25, 2024

Would this perhaps become simpler if we'd keep tracking such segments asMemorySegments but allowed the buffer to benull, indicating all zeroes?

More generally, when not indicating the presence of the segment to Binaryen, we'd always have to assume that memory is zeroed, as the zeroes won't be in the(memory ...). Might well be a non-issue in practice, though.

Copy link

MemberAuthor

CountBleck commentedMar 25, 2024

Would this perhaps become simpler[...]?

Oh, that's...a much better idea. Thanks and sorry about that.

CountBleck force-pushed thefix-large-memory-crash branch from373166c to6a89a09Compare

March 25, 2024 01:49

Copy link

MemberAuthor

CountBleck commentedMar 25, 2024•
edited
Loading

Might well be a non-issue in practice, though.

@dcodeIO hmm, would this conflict when--importMemory is specified without--zeroFilledMemory?

CountBleck requested review fromdcodeIO andHerrCai0907

March 25, 2024 01:53

CountBleck marked this pull request as ready for review

March 25, 2024 03:00

HerrCai0907 approved these changes

Mar 28, 2024

View reviewed changes

src/module.tsShow resolvedHide resolved

CountBleck force-pushed thefix-large-memory-crash branch 3 times, most recently fromed50aef to4465767Compare

April 3, 2024 01:46

BREAKING CHANGE: Do not allocate buffers for memory.data(i32, i32?)

163a631

MemorySegments can now hold null buffers, which signifies that memory isin fact used in the compiled program, for programs that only reservezero-filled memories. Using null buffers avoids passing zero-filledbuffers to Binaryen, which isn't necessary since Wasm memories areinitialized with zeroes when a (data ...) segment isn't present.This change is breaking in that modules using imported memories thatcontain non-zero values in such memory ranges at the time ofinstantiation will retain those values, as opposed to being filled.This shouldn't affect most users, however, and it could possibly bedesirable.FixesAssemblyScript#2827.