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
Merged

Conversation

pipcet
Copy link
Contributor

@pipcet pipcet 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
Copy link
Member

kripken 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
Copy link
Member

kripken 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
Copy link
Contributor Author

pipcet 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
Copy link
Member

kripken 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();
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

kripken 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.

* print warning for unknown name subsections (including the local
  section)

* name variables properly
@lukewagner
Copy link
Member

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
Copy link
Contributor Author

pipcet 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
Copy link
Contributor Author

pipcet 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
Copy link
Member

dschuff 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
Copy link
Member

dschuff 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
Copy link
Contributor Author

pipcet 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
Copy link
Member

kripken commented Apr 10, 2017

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

@dschuff
Copy link
Member

dschuff 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
@pipcet pipcet deleted the extensible-name-section branch April 15, 2017 08:54
hubot pushed a commit to WebKit/WebKit-http that referenced this pull request May 10, 2017
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants