Skip to content

Handle passive segments in wasm-emscripten-finalize#2217

Merged
tlively merged 6 commits intoWebAssembly:masterfrom
tlively:emscripten-passive-pthreads
Jul 12, 2019
Merged

Handle passive segments in wasm-emscripten-finalize#2217
tlively merged 6 commits intoWebAssembly:masterfrom
tlively:emscripten-passive-pthreads

Conversation

@tlively
Copy link
Copy Markdown
Member

@tlively tlively commented Jul 11, 2019

This will be necessary once we use passive segments for pthread code.

@tlively tlively requested review from kripken and quantum5 July 11, 2019 00:55
Copy link
Copy Markdown
Member

@quantum5 quantum5 left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I don't really understand binaryen that well.

Comment thread scripts/test/lld.py
}
if not is_passive:
extension_arg_map.update({
'.mem.out': ['--separate-data-segments', mem_file],
Copy link
Copy Markdown
Member

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

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.

Comment thread src/wasm/wasm-emscripten.cpp Outdated
}

std::vector<Address> getSegmentOffsets(Module& wasm) {
std::unordered_map<unsigned, Address> passiveOffsets;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unsigned can be Index as it is the index of a wasm module thing IIUC

if (it != passiveOffsets.end()) {
segmentOffsets.push_back(it->second);
} else {
// This was a non-constant offset (perhaps TLS)
Copy link
Copy Markdown
Member

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

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

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

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.

@tlively
Copy link
Copy Markdown
Member Author

tlively commented Jul 11, 2019

@kripken This should be good to go now

@tlively tlively merged commit 29b6458 into WebAssembly:master Jul 12, 2019
@tlively tlively deleted the emscripten-passive-pthreads branch July 12, 2019 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants