Skip to content

Handle passive segments in wasm-emscripten-finalize #2217

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

Merged
tlively merged 6 commits into from
Jul 12, 2019
Merged

Handle passive segments in wasm-emscripten-finalize #2217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
094909b
Handle passive segments in wasm-emscripten-finalize
tlively Jul 11, 2019
d606577
Fix flake8
tlively Jul 11, 2019
995115d
unsigned -> Index
tlively Jul 11, 2019
3578a64
Prevent use of bogus addresses
tlively Jul 11, 2019
18e35d5
Use -1 as invalid offset
tlively Jul 11, 2019
db00af8
Fix shared library test failure
tlively Jul 11, 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
14 changes: 11 additions & 3 deletions scripts/test/lld.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,15 @@ def test_wasm_emscripten_finalize():

for wast_path in files_with_pattern(options.binaryen_test, 'lld', '*.wast'):
print '..', wast_path
is_passive = '.passive.' in wast_path
mem_file = wast_path + '.mem'
extension_arg_map = {
'.out': [],
'.mem.out': ['--separate-data-segments', mem_file],
'.out': [],
}
if not is_passive:
extension_arg_map.update({
'.mem.out': ['--separate-data-segments', mem_file],
Copy link
Copy Markdown
Member

@quantum5 quantum5 Jul 11, 2019

Choose a reason for hiding this comment

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

What's the indentation we are supposed to use here? This file looks like it's 2 space indented, but this is 4 spaces indented.

Copy link
Copy Markdown
Member Author

@tlively tlively Jul 11, 2019

Choose a reason for hiding this comment

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

I think it uses four here because it is a continuation of an expression from the previous line. Flake8 doesn't complain at any rate.

})
for ext, ext_args in extension_arg_map.items():
expected_file = wast_path + ext
if ext != '.out' and not os.path.exists(expected_file):
Expand All @@ -63,11 +67,15 @@ def update_lld_tests():

for wast_path in files_with_pattern(options.binaryen_test, 'lld', '*.wast'):
print '..', wast_path
is_passive = '.passive.' in wast_path
mem_file = wast_path + '.mem'
extension_arg_map = {
'.out': [],
'.mem.out': ['--separate-data-segments', mem_file + '.mem'],
}
if not is_passive:
extension_arg_map.update({
'.mem.out': ['--separate-data-segments', mem_file + '.mem'],
})
for ext, ext_args in extension_arg_map.items():
out_path = wast_path + ext
if ext != '.out' and not os.path.exists(out_path):
Expand Down
38 changes: 36 additions & 2 deletions src/wasm/wasm-emscripten.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,10 +444,43 @@ void EmscriptenGlueGenerator::replaceStackPointerGlobal() {
wasm.removeGlobal(stackPointer->name);
}

const Address UNKNOWN_OFFSET(uint32_t(-1));

std::vector<Address> getSegmentOffsets(Module& wasm) {
std::unordered_map<Index, Address> passiveOffsets;
if (wasm.features.hasBulkMemory()) {
// Fetch passive segment offsets out of memory.init instructions
struct OffsetSearcher : PostWalker<OffsetSearcher> {
std::unordered_map<Index, Address>& offsets;
OffsetSearcher(std::unordered_map<unsigned, Address>& offsets)
: offsets(offsets) {}
void visitMemoryInit(MemoryInit* curr) {
auto* dest = curr->dest->dynCast<Const>();
if (!dest) {
return;
}
auto it = offsets.find(curr->segment);
if (it != offsets.end()) {
Fatal() << "Cannot get offset of passive segment initialized "
"multiple times";
}
offsets[curr->segment] = dest->value.geti32();
}
} searcher(passiveOffsets);
searcher.walkModule(&wasm);
}
std::vector<Address> segmentOffsets;
for (unsigned i = 0; i < wasm.memory.segments.size(); ++i) {
if (auto* addrConst = wasm.memory.segments[i].offset->dynCast<Const>()) {
auto& segment = wasm.memory.segments[i];
if (segment.isPassive) {
auto it = passiveOffsets.find(i);
if (it != passiveOffsets.end()) {
segmentOffsets.push_back(it->second);
} else {
// This was a non-constant offset (perhaps TLS)
Copy link
Copy Markdown
Member

@kripken kripken Jul 11, 2019

Choose a reason for hiding this comment

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

I don't understand what this does. What will happen with that 0?

Copy link
Copy Markdown
Member Author

@tlively tlively Jul 11, 2019

Choose a reason for hiding this comment

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

Basically if AsmConstWalker or EmJsWalker try to extract anything from such a segment they will silently get a bogus address. I added a condition to stringAtAddr, which seems to be the ultimate consumer of these segment offsets, to make it skip any segment with a 0 offset, which can never be valid.

Copy link
Copy Markdown
Member

@kripken kripken Jul 11, 2019

Choose a reason for hiding this comment

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

Hmm, why can't 0 be a valid offset? Doesn't that mean it starts at address 0 which is valid?

Copy link
Copy Markdown
Member Author

@tlively tlively Jul 11, 2019

Choose a reason for hiding this comment

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

0 can't be a valid segment offset because that would mean the linker tried to put some data at address 0, which is of course inaccessible from C.

segmentOffsets.push_back(UNKNOWN_OFFSET);
}
} else if (auto* addrConst = segment.offset->dynCast<Const>()) {
auto address = addrConst->value.geti32();
segmentOffsets.push_back(address);
} else {
Expand Down Expand Up @@ -494,7 +527,8 @@ const char* stringAtAddr(Module& wasm,
for (unsigned i = 0; i < wasm.memory.segments.size(); ++i) {
Memory::Segment& segment = wasm.memory.segments[i];
Address offset = segmentOffsets[i];
if (address >= offset && address < offset + segment.data.size()) {
if (offset != UNKNOWN_OFFSET && address >= offset &&
address < offset + segment.data.size()) {
return &segment.data[address - offset];
}
}
Expand Down
40 changes: 40 additions & 0 deletions test/lld/hello_world.passive.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
(module
(type $0 (func (param i32) (result i32)))
(type $1 (func))
(type $2 (func (result i32)))
(type $3 (func (param i32 i32) (result i32)))
(import "env" "puts" (func $puts (param i32) (result i32)))
(memory $0 2)
(data passive "Hello, world\00")
(table $0 1 1 funcref)
(global $global$0 (mut i32) (i32.const 66128))
(global $global$1 i32 (i32.const 66128))
(global $global$2 i32 (i32.const 581))
(export "memory" (memory $0))
(export "__wasm_call_ctors" (func $__wasm_call_ctors))
(export "__heap_base" (global $global$1))
(export "__data_end" (global $global$2))
(export "main" (func $main))
(func $__wasm_call_ctors (; 1 ;) (type $1)
(call $__wasm_init_memory)
)
(func $__wasm_init_memory (; 2 ;) (type $1)
(memory.init 0
(i32.const 568)
(i32.const 0)
(i32.const 14)
)
)
(func $__original_main (; 3 ;) (type $2) (result i32)
(drop
(call $puts
(i32.const 568)
)
)
(i32.const 0)
)
(func $main (; 3 ;) (type $3) (param $0 i32) (param $1 i32) (result i32)
(call $__original_main)
)
;; custom section "producers", size 111
)
111 changes: 111 additions & 0 deletions test/lld/hello_world.passive.wast.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
(module
(type $0 (func (param i32) (result i32)))
(type $1 (func))
(type $2 (func (result i32)))
(type $3 (func (param i32 i32) (result i32)))
(import "env" "puts" (func $puts (param i32) (result i32)))
(memory $0 2)
(data passive "Hello, world\00")
(table $0 1 1 funcref)
(global $global$0 (mut i32) (i32.const 66128))
(global $global$1 i32 (i32.const 66128))
(global $global$2 i32 (i32.const 581))
(export "memory" (memory $0))
(export "__wasm_call_ctors" (func $__wasm_call_ctors))
(export "__heap_base" (global $global$1))
(export "__data_end" (global $global$2))
(export "main" (func $main))
(export "stackSave" (func $stackSave))
(export "stackAlloc" (func $stackAlloc))
(export "stackRestore" (func $stackRestore))
(export "__growWasmMemory" (func $__growWasmMemory))
(func $__wasm_call_ctors (; 1 ;) (type $1)
(call $__wasm_init_memory)
)
(func $__wasm_init_memory (; 2 ;) (type $1)
(memory.init 0
(i32.const 568)
(i32.const 0)
(i32.const 14)
)
)
(func $__original_main (; 3 ;) (type $2) (result i32)
(drop
(call $puts
(i32.const 568)
)
)
(i32.const 0)
)
(func $main (; 4 ;) (type $3) (param $0 i32) (param $1 i32) (result i32)
(call $__original_main)
)
(func $stackSave (; 5 ;) (result i32)
(global.get $global$0)
)
(func $stackAlloc (; 6 ;) (param $0 i32) (result i32)
(local $1 i32)
(global.set $global$0
(local.tee $1
(i32.and
(i32.sub
(global.get $global$0)
(local.get $0)
)
(i32.const -16)
)
)
)
(local.get $1)
)
(func $stackRestore (; 7 ;) (param $0 i32)
(global.set $global$0
(local.get $0)
)
)
(func $__growWasmMemory (; 8 ;) (param $newSize i32) (result i32)
(memory.grow
(local.get $newSize)
)
)
)
(;
--BEGIN METADATA --
{
"staticBump": 13,
"tableSize": 1,
"initializers": [
"__wasm_call_ctors"
],
"declares": [
"puts"
],
"externs": [
],
"implementedFunctions": [
"___wasm_call_ctors",
"_main",
"_stackSave",
"_stackAlloc",
"_stackRestore",
"___growWasmMemory"
],
"exports": [
"__wasm_call_ctors",
"main",
"stackSave",
"stackAlloc",
"stackRestore",
"__growWasmMemory"
],
"namedGlobals": {
"__heap_base" : "66128",
"__data_end" : "581"
},
"invokeFuncs": [
],
"features": [
]
}
-- END METADATA --
;)