Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement 0-len/drop spec changes in bulk memory #2529

Merged
merged 5 commits into from
Dec 17, 2019
Merged

Implement 0-len/drop spec changes in bulk memory #2529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bd41a46
Implement 0-len/drop spec changes in bulk memory
aheejin Dec 15, 2019
0a2b805
Fixes
aheejin Dec 16, 2019
806c8ba
Offset 0/size 0 for memory.init of dropped segment
aheejin Dec 16, 2019
b73c0e9
Typo fix
aheejin Dec 16, 2019
635b874
Cosmetic change
aheejin Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/passes/MemoryPacking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,6 @@ struct MemoryPacking : public Pass {
changed = true;
}
}
void visitDataDrop(DataDrop* curr) {
if (!getModule()->memory.segments[curr->segment].isPassive) {
ExpressionManipulator::unreachable(curr);
changed = true;
}
}
void doWalkFunction(Function* func) {
changed = false;
super::doWalkFunction(func);
Expand Down
35 changes: 22 additions & 13 deletions src/wasm-interpreter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1901,18 +1901,22 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
assert(curr->segment < instance.wasm.memory.segments.size());
Memory::Segment& segment = instance.wasm.memory.segments[curr->segment];

if (instance.droppedSegments.count(curr->segment)) {
trap("memory.init of dropped segment");
}

Address destVal(uint32_t(dest.value.geti32()));
Address offsetVal(uint32_t(offset.value.geti32()));
Address sizeVal(uint32_t(size.value.geti32()));

if (offsetVal + sizeVal > 0 &&
instance.droppedSegments.count(curr->segment)) {
trap("out of bounds segment access in memory.init");
}
if ((uint64_t)offsetVal + sizeVal > segment.data.size()) {
trap("out of bounds segment access in memory.init");
}
if ((uint64_t)destVal + sizeVal >
(uint64_t)instance.memorySize * Memory::kPageSize) {
trap("out of bounds memory access in memory.init");
}
for (size_t i = 0; i < sizeVal; ++i) {
if (offsetVal + i >= segment.data.size()) {
trap("out of bounds segment access in memory.init");
}
Literal addr(uint32_t(destVal + i));
instance.externalInterface->store8(instance.getFinalAddress(addr, 1),
segment.data[offsetVal + i]);
Expand All @@ -1921,9 +1925,6 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
}
Flow visitDataDrop(DataDrop* curr) {
NOTE_ENTER("DataDrop");
if (instance.droppedSegments.count(curr->segment)) {
trap("data.drop of dropped segment");
}
instance.droppedSegments.insert(curr->segment);
return {};
}
Expand All @@ -1948,6 +1949,13 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
Address sourceVal(uint32_t(source.value.geti32()));
Address sizeVal(uint32_t(size.value.geti32()));

if ((uint64_t)sourceVal + sizeVal >
(uint64_t)instance.memorySize * Memory::kPageSize ||
(uint64_t)destVal + sizeVal >
(uint64_t)instance.memorySize * Memory::kPageSize) {
trap("out of bounds segment access in memory.copy");
}

int64_t start = 0;
int64_t end = sizeVal;
int step = 1;
Expand All @@ -1958,9 +1966,6 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
step = -1;
}
for (int64_t i = start; i != end; i += step) {
if (i + destVal >= std::numeric_limits<uint32_t>::max()) {
trap("Out of bounds memory access");
}
instance.externalInterface->store8(
instance.getFinalAddress(Literal(uint32_t(destVal + i)), 1),
instance.externalInterface->load8s(
Expand Down Expand Up @@ -1988,6 +1993,10 @@ template<typename GlobalManager, typename SubType> class ModuleInstanceBase {
Address destVal(uint32_t(dest.value.geti32()));
Address sizeVal(uint32_t(size.value.geti32()));

if ((uint64_t)destVal + sizeVal >
(uint64_t)instance.memorySize * Memory::kPageSize) {
trap("out of bounds memory access in memory.fill");
}
uint8_t val(value.value.geti32());
for (size_t i = 0; i < sizeVal; ++i) {
instance.externalInterface->store8(
Expand Down
20 changes: 8 additions & 12 deletions test/passes/memory-packing_all-features.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,20 @@
(type $none_=>_none (func))
(memory $0 1 1)
(func $foo (; 0 ;)
(block
(drop
(i32.const 0)
)
(drop
(i32.const 0)
)
(drop
(i32.const 0)
)
(unreachable)
(drop
(i32.const 0)
)
(drop
(i32.const 0)
)
(drop
(i32.const 0)
)
(unreachable)
)
(func $bar (; 1 ;)
(drop
(loop $loop-in (result i32)
(unreachable)
(i32.const 42)
)
)
Expand Down
2 changes: 0 additions & 2 deletions test/passes/memory-packing_all-features.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,10 @@
(i32.const 0)
(i32.const 0)
)
(data.drop 0)
)
(func $bar
(drop
(loop (result i32)
(data.drop 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this function tests anything useful without the data.drop. Should we just leave the data.drops in these tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After memory packing optimization, this module does not contain any segments left, so this becomes a validation error. The same for the one more removed (data.drop 0) above. Before, it was OK because we converted these data.drops to unreachables, because they tried to drop already dropped segments. But dropping the same segments repeatedly is not a trap anymore, so we shouldn't do that. (So segment 0 existed in the beginning and dropped because it was an active segment, but after transformation it does not exist anymore).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, come to think of it, then it'd be better to just delete this function, now that it does not test anything meaningful... I already merged this :( But I'll post a follow-up patch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, are you saying that the memory packing removes the segment but keeps the data.drop around, leading to a validation error? That sounds like a bug in the memory packing pass. It should convert the data.drop to a nop or remove it instead.

Copy link
Member Author

@aheejin aheejin Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah, that sounds like a new bug introduced... Is it worth fixing given that you are gonna completely overhaul the pass soon?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed a bug report anyway: #2535

(i32.const 42)
)
)
Expand Down
32 changes: 17 additions & 15 deletions test/spec/bulk-memory.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@
;; Succeed when writing 0 bytes at the end of the region.
(invoke "fill" (i32.const 0x10000) (i32.const 0) (i32.const 0))

;; OK to write 0 bytes outside of memory.
(invoke "fill" (i32.const 0x10001) (i32.const 0) (i32.const 0))
;; Writing 0 bytes outside of memory limit is NOT allowed.
(assert_trap (invoke "fill" (i32.const 0x10001) (i32.const 0) (i32.const 0)))

;; memory.copy
(module
Expand Down Expand Up @@ -101,9 +101,9 @@
(invoke "copy" (i32.const 0x10000) (i32.const 0) (i32.const 0))
(invoke "copy" (i32.const 0) (i32.const 0x10000) (i32.const 0))

;; OK copying 0 bytes outside of memory.
(invoke "copy" (i32.const 0x10001) (i32.const 0) (i32.const 0))
(invoke "copy" (i32.const 0) (i32.const 0x10001) (i32.const 0))
;; Copying 0 bytes outside of memory limit is NOT allowed.
(assert_trap (invoke "copy" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
(assert_trap (invoke "copy" (i32.const 0) (i32.const 0x10001) (i32.const 0)))

;; memory.init
(module
Expand All @@ -128,19 +128,22 @@
;; Init ending at memory limit and segment limit is ok.
(invoke "init" (i32.const 0xfffc) (i32.const 0) (i32.const 4))

;; Out-of-bounds writes trap, but all previous writes succeed.
;; Out-of-bounds writes trap, and no partial writes has been made.
(assert_trap (invoke "init" (i32.const 0xfffe) (i32.const 0) (i32.const 3))
"out of bounds memory access")
(assert_return (invoke "load8_u" (i32.const 0xfffe)) (i32.const 0xaa))
(assert_return (invoke "load8_u" (i32.const 0xffff)) (i32.const 0xbb))
(assert_return (invoke "load8_u" (i32.const 0xfffe)) (i32.const 0xcc))
(assert_return (invoke "load8_u" (i32.const 0xffff)) (i32.const 0xdd))

;; Succeed when writing 0 bytes at the end of either region.
(invoke "init" (i32.const 0x10000) (i32.const 0) (i32.const 0))
(invoke "init" (i32.const 0) (i32.const 4) (i32.const 0))

;; OK writing 0 bytes outside of memory or segment.
(invoke "init" (i32.const 0x10001) (i32.const 0) (i32.const 0))
(invoke "init" (i32.const 0) (i32.const 5) (i32.const 0))
;; Writing 0 bytes outside of memory / segment limit is NOT allowed.
(assert_trap (invoke "init" (i32.const 0x10001) (i32.const 0) (i32.const 0)))
(assert_trap (invoke "init" (i32.const 0) (i32.const 5) (i32.const 0)))

;; OK to access 0 bytes at offset 0 in a dropped segment.
(invoke "init" (i32.const 0) (i32.const 0) (i32.const 0))

;; data.drop
(module
Expand All @@ -157,9 +160,8 @@
(memory.init 1 (i32.const 0) (i32.const 0) (i32.const 0)))
)

;; OK to drop the same segment multiple times or drop an active segment.
(invoke "init_passive")
(invoke "drop_passive")
(assert_trap (invoke "drop_passive") "data segment dropped")
(assert_trap (invoke "init_passive") "data segment dropped")
(assert_trap (invoke "drop_active") "data segment dropped")
(assert_trap (invoke "init_active") "data segment dropped")
(invoke "drop_passive")
(invoke "drop_active")