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.

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

Extensible name section #933

Merged
merged 4 commits into from Apr 13, 2017

Conversation

Projects
None yet
4 participants
@pipcet
Copy link
Contributor

commented Mar 6, 2017

First stab at addressing #914.

No locals yet, but we weren't emitting those before.

Are there any tests I should be running?

@kripken

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Thanks! I didn't look closely at the code yet, but to verify this works, the binaryen tests can be run with ./check.py --no-test-waterfall, and emscripten has specific tests for this, ./tests/runner.py other.test_binaryen_debuginfo and ./tests/runner.py binaryen*.test_demangle_stacks (if you don't already have an emscripten environment set up and don't want to bother just for this, I can run those for you).

@kripken

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

The error on travis ci here looks like it's due to the binary size changing in a test. Can run ./auto_update_tests.py to fix that (or just edit the output file manually).

@pipcet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2017

Thanks! Yeah, I verified the size change matches what should happen (again, if I'm reading the spec correctly): plus 5 bytes for the subsection size, 1 byte for the subsection type, 2x1 byte for the indices, minus 2 bytes for the missing local count.

There's one slight change in behavior: the old code used to die when presented with a module that defined local names, the new code simply ignores the local[2] subsection of the "name" section. I think the new behavior is better, as I can run binaryen utilities directly on my wasm modules without having to strip the "name" section, but if you prefer the old behavior I can change it.

@kripken

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

Yeah, that change makes sense. Although perhaps we should print a warning instead of silently ignoring it?

@@ -1511,23 +1524,27 @@ void WasmBinaryBuilder::readTableElements() {

void WasmBinaryBuilder::readNames() {
if (debug) std::cerr << "== readNames" << std::endl;
auto name_type = getU32LEB();

This comment has been minimized.

Copy link
@kripken

kripken Mar 7, 2017

Member

convention is nameType etc. for these new vars

@@ -1511,23 +1524,27 @@ void WasmBinaryBuilder::readTableElements() {

void WasmBinaryBuilder::readNames() {
if (debug) std::cerr << "== readNames" << std::endl;
auto name_type = getU32LEB();
auto subsection_size = getU32LEB();
WASM_UNUSED(subsection_size);

This comment has been minimized.

Copy link
@kripken

kripken Mar 7, 2017

Member

we could use it at the end, to compare pos and see it incremented by the right amount? unless I'm missing something

This comment has been minimized.

Copy link
@pipcet

pipcet Mar 7, 2017

Author Contributor

absolutely! And that actually caught a silly bug in my non-binaryen code, so thanks for the suggestion!

This comment has been minimized.

Copy link
@kripken

kripken Mar 7, 2017

Member

Oh, heh, nice :)

Btw, is the other code you mention your gcc wasm backend? Are you connecting those in some way, or just using binaryen to inspect the output from gcc?

This comment has been minimized.

Copy link
@pipcet

pipcet Mar 8, 2017

Author Contributor

Just the latter, currently, but I'm looking at combining them when it makes sense to do so (I'm currently emitting tee_local at way too late a stage, by looking at the opcode stream for the right combinations, for example).

This comment has been minimized.

Copy link
@kripken

kripken Mar 8, 2017

Member

I see, thanks. I'd be curious to see how the binaryen optimizer works on your output. It's a goal for it to be as general purpose as possible, so testing on more compilers can help.

@kripken

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

Ok, just some minor comments, overall this looks correct, thanks @pipcet :)

@lukewagner: you asked for a ping when we are ready to test this with a VM, this PR should be ready for that.

* check subsection size matches
* print warning for unknown name subsections (including the local
  section)

* name variables properly
@lukewagner

This comment has been minimized.

Copy link
Member

commented Mar 8, 2017

Hey thanks a lot! So I don't know if yous have been following design/#989, but @titzer's proposal is to name the updated extensible-names section utf8-names instead of just names. Still need a PR on design repo to nail that down, though. What's nice is this could allow engines to accept both names and utf8-names for a period of time, smoothing the transition.

@pipcet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2017

@lukewagner I confess I haven't been following that discussion, but I think that's an excellent idea (I'm not so sure utilities should support the "name" subsection still, but that's a separate issue from changing the name).

@kripken I suggest delaying this PR here until the design repo PR is through.

@pipcet

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2017

If I'm reading WebAssembly/design#1016 correctly, the proposal to rename the "name" section has been dropped.

The question now is whether it's too late to handle only the new extensible name section, or whether some sort of autodetection (not very hard) is required to tell whether we're looking at an old-style or new-style "name" section. (Unless I'm mistaken, an old-style "name" section will never split into valid subsections).

In either case, I think it's time to make the extensible name section the default.

@dschuff

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

I think we should just support the extensible name section. Speaking of which, do we have any tests that we drop bogus name sections? we should.

@dschuff

This comment has been minimized.

Copy link
Member

commented Apr 8, 2017

V8 just landed a patch to update to extensible names section and I tested this patch against it. All of the aforementioned tests are passing, so I think this change can go in whenever we are ready.
As for when we are ready: basically, those tests break when binaryen doesn't match the JS engine. Since V8 is already updated and SM is presumably not, the right time to land it is presumably any time between now and when SM updates what it accepts. (at which point our waterfall with V8 will go green and those tests will cover this change, and the waterfall with SM will go red until it updates). Since it looks like the main emscripten buildbot is currently broken (and therefore we don't have SM test coverage right now anyway), I'd vote for just merging this without waiting.

@pipcet

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2017

Thanks for testing!

I think the plan was for SM to update when binaryen does, so obviously I agree we should merge it soon.

@kripken

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

I agree with @dschuff, we can merge this asap, unless @lukewagner has concerns?

@dschuff

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

OK, our bots are able to cover this again, so hearing no objections I'm going to go ahead and merge it.

@dschuff dschuff merged commit 57a2bb9 into WebAssembly:master Apr 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pipcet pipcet deleted the pipcet:extensible-name-section branch Apr 15, 2017

hubot pushed a commit to WebKit/webkit that referenced this pull request May 10, 2017

jfbastien@apple.com
WebAssembly: support name section
JSTests:

https://bugs.webkit.org/show_bug.cgi?id=171263

Reviewed by Keith Miller.

* wasm/function-tests/nameSection.js: Added.
(const.compile):
* wasm/function-tests/nameSection.wasm: Added.
* wasm/function-tests/stack-trace.js: Update format

Source/JavaScriptCore:

https://bugs.webkit.org/show_bug.cgi?id=171263

Reviewed by Keith Miller.

The name section is an optional custom section in the WebAssembly
spec. At least when debugging, developers expect to be able to use
this section to obtain intelligible stack traces, otherwise we
just number the wasm functions which is somewhat painful.

This patch parses this section, dropping its content eagerly on
error, and if there is a name section then backtraces use their
value instead of numbers. Otherwise we stick to numbers as before.

Note that the format of name sections changed in mid-February:
  WebAssembly/design#984
And binaryen was only updated in early March:
  WebAssembly/binaryen#933

* CMakeLists.txt:
* JavaScriptCore.xcodeproj/project.pbxproj:
* interpreter/Interpreter.cpp:
(JSC::GetStackTraceFunctor::operator()):
* interpreter/StackVisitor.cpp:
(JSC::StackVisitor::readNonInlinedFrame):
(JSC::StackVisitor::Frame::functionName):
* interpreter/StackVisitor.h:
(JSC::StackVisitor::Frame::wasmFunctionIndexOrName):
* runtime/StackFrame.cpp:
(JSC::StackFrame::functionName):
* runtime/StackFrame.h:
(JSC::StackFrame::StackFrame):
(JSC::StackFrame::wasm):
* wasm/WasmBBQPlanInlines.h:
(JSC::Wasm::BBQPlan::initializeCallees):
* wasm/WasmCallee.cpp:
(JSC::Wasm::Callee::Callee):
* wasm/WasmCallee.h:
(JSC::Wasm::Callee::create):
(JSC::Wasm::Callee::indexOrName):
* wasm/WasmFormat.cpp:
(JSC::Wasm::makeString):
* wasm/WasmFormat.h:
(JSC::Wasm::isValidExternalKind):
(JSC::Wasm::isValidNameType):
(JSC::Wasm::NameSection::get):
* wasm/WasmIndexOrName.cpp: Copied from Source/JavaScriptCore/wasm/WasmCallee.cpp.
(JSC::Wasm::IndexOrName::IndexOrName):
(JSC::Wasm::makeString):
* wasm/WasmIndexOrName.h: Copied from Source/JavaScriptCore/wasm/WasmFormat.cpp.
* wasm/WasmModuleInformation.h:
* wasm/WasmModuleParser.cpp:
* wasm/WasmName.h: Copied from Source/JavaScriptCore/wasm/WasmCallee.cpp.
* wasm/WasmNameSectionParser.cpp: Added.
* wasm/WasmNameSectionParser.h: Copied from Source/JavaScriptCore/wasm/WasmCallee.cpp.
(JSC::Wasm::NameSectionParser::NameSectionParser):
* wasm/WasmOMGPlan.cpp:
(JSC::Wasm::OMGPlan::work):
* wasm/WasmParser.h:
(JSC::Wasm::Parser<SuccessType>::consumeUTF8String):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@216597 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.