Extensible name section #933

pipcet · 2017-03-06T23:15:46Z

First stab at addressing #914.

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

Are there any tests I should be running?

kripken · 2017-03-06T23:29:20Z

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 · 2017-03-07T02:24:21Z

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 · 2017-03-07T09:15:52Z

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 · 2017-03-07T19:10:18Z

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

kripken · 2017-03-07T19:16:07Z

src/wasm/wasm-binary.cpp

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

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


convention is nameType etc. for these new vars

kripken · 2017-03-07T19:17:06Z

src/wasm/wasm-binary.cpp

@@ -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);


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

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

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?

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

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 · 2017-03-07T19:19:10Z

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 · 2017-03-08T14:59:20Z

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 · 2017-03-08T16:17:34Z

@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 · 2017-03-30T16:54:15Z

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 · 2017-04-06T22:24:54Z

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 · 2017-04-08T00:25:02Z

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 · 2017-04-08T00:33:09Z

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 · 2017-04-10T17:39:57Z

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

dschuff · 2017-04-13T15:49:16Z

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

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

pipcet added 2 commits March 6, 2017 22:06

extensible name section support

9bf8e9b

See WebAssembly#914.

extensible name section support: read function names, too

e0834ad

c-api-unused-mem.txt: change expected size to match new name section

e527d3a

kripken reviewed Mar 7, 2017

View reviewed changes

* check subsection size matches

136525f

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

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

pipcet deleted the extensible-name-section branch April 15, 2017 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible name section #933

Extensible name section #933

pipcet commented Mar 6, 2017

kripken commented Mar 6, 2017

kripken commented Mar 7, 2017

pipcet commented Mar 7, 2017

kripken commented Mar 7, 2017

kripken Mar 7, 2017

kripken Mar 7, 2017

pipcet Mar 7, 2017

kripken Mar 7, 2017

pipcet Mar 8, 2017

kripken Mar 8, 2017

kripken commented Mar 7, 2017

lukewagner commented Mar 8, 2017

pipcet commented Mar 8, 2017

pipcet commented Mar 30, 2017

dschuff commented Apr 6, 2017

dschuff commented Apr 8, 2017

pipcet commented Apr 8, 2017

kripken commented Apr 10, 2017

dschuff commented Apr 13, 2017

Extensible name section #933

Extensible name section #933

Conversation

pipcet commented Mar 6, 2017

kripken commented Mar 6, 2017

kripken commented Mar 7, 2017

pipcet commented Mar 7, 2017

kripken commented Mar 7, 2017

kripken Mar 7, 2017

Choose a reason for hiding this comment

kripken Mar 7, 2017

Choose a reason for hiding this comment

pipcet Mar 7, 2017

Choose a reason for hiding this comment

kripken Mar 7, 2017

Choose a reason for hiding this comment

pipcet Mar 8, 2017

Choose a reason for hiding this comment

kripken Mar 8, 2017

Choose a reason for hiding this comment

kripken commented Mar 7, 2017

lukewagner commented Mar 8, 2017

pipcet commented Mar 8, 2017

pipcet commented Mar 30, 2017

dschuff commented Apr 6, 2017

dschuff commented Apr 8, 2017

pipcet commented Apr 8, 2017

kripken commented Apr 10, 2017

dschuff commented Apr 13, 2017