Support start #182

jfbastien · 2016-02-04T13:27:17Z

As spec'd in: WebAssembly/design#495
And discussed in: WebAssembly/spec#231

This will make it simpler and more uniform to add a start entry point.

s2wasm is the right place to add start because it'll eventually need to
do other basic setup, e.g. put code in start to setup the stack, as
@dschuff is doing in:
#179
Or rather, the linker is the right place and s2wasm happens to act as
our linker right now.

jfbastien · 2016-02-04T13:28:43Z

@rossberg-chromium @binji it would be good for your parsers to support (start func).

kripken · 2016-02-04T20:33:19Z

src/s2wasm.h

@@ -696,7 +700,7 @@ class S2WasmBuilder {
        Name target = cleanFunction(getCommaSeparated());
        auto aliased = aliasedFunctions.find(target);
        if (aliased != aliasedFunctions.end()) target = aliased->second;
-        if (implementedFunctions.count(target) > 0) {
+        if (implementedFunctions.count(target) != 0) {


Oh extra change, I initially modified implementedFunctions and had to change all its uses, but then put it back. I didn't realize I'd changed it to something else (I was just restoring the old behavior), but it can only ever be 0 or 1 so != seems more natural?

Seems less natural to me, but whatever :)

kripken · 2016-02-04T20:40:52Z

We'll need a followup issue for the interpreter actually calling the start method. I assume the spec repo will get a test for that eventually.

As spec'd in: WebAssembly/design#495 And discussed in: WebAssembly/spec#231 This will make it simpler and more uniform to add a start entry point. s2wasm is the right place to add start because it'll eventually need to do other basic setup, e.g. put code in start to setup the stack, as dschuff is doing in: #179 Or rather, the linker is the right place and s2wasm happens to act as our linker right now.

Support start

jfbastien · 2016-02-05T10:26:17Z

Merged because timezones, let me know if you have any follow-up concerns that I should address.

kripken · 2016-02-05T21:01:54Z

As mentioned above, I actually don't even understand what this pull request does. Please explain when you get a chance.

kripken · 2016-02-05T21:02:40Z

src/s2wasm.h

@@ -1193,6 +1198,42 @@ class S2WasmBuilder {
        }
      }
    }
+    if (!!startFunction) {


Why doesn't startFunction.is() work..?

Because it refers to a string "\0" which isn't nullptr, whereas operator! checks for nullptr and str[0] == '\0'. It's not very idiomatic C++!

isn't startFunction null if not used? Then checking is() which checks the pointer is not null would be fine?

or are you saying startFunction has a valid pointer that points to an empty string?

startFunction is initialized with a std::string which makes it "\0" because Name passes that to IString's ctor. So yes, it's a valid zero-length string :-)

I much prefer idiomatic std::string, despite its shortcomings! Commoning strings is fine in some cases, but I'm not sure these tools really need it especially for such short strings which could just be inline. We could also use folly::string which is more configurable and closer to std::string. Also well tested, but I'm biased on that ;-)

But we use Name extensively, for performance reasons. I'd rather use Name for startFunction as well. Is there a reason not to?

String performance on large strings are a gain with the Name approach, but small strings usually win with the inline approach that most std::string implementations use. Names are usually small, so without measurements showing otherwise I'd just stick to std::string until we measure a perf issue. There's also the idiomatic argument: C++ APIs work a certain way, Name looks different and isn't obvious. This !! thing is an example of that.

I went with Name since the rest of the codebase does that, but the corners seem sharp from here!

I'm ok to change the API to make it more idiomatic if you want, let's discuss.

But in general I think Name is the right thing for the AST - we know it helps perf hugely there. And anything interfacing with that should be a Name or else we'll have constant weird mismatches of std::string here but Name there. That's what I want to avoid.

jfbastien · 2016-02-05T21:24:40Z

As mentioned above, I actually don't even understand what this pull request does. Please explain when you get a chance.

You mean WebAssembly/design#495 isn't clear? Or the way this PR implements it isn't? I'm not sure what's unclear ;-)

kripken · 2016-02-05T21:30:29Z

src/s2wasm.h

@@ -1193,6 +1198,42 @@ class S2WasmBuilder {
        }
      }


Let me try to be clearer. This line, and the big multiline change under it - lines 1201 to 1236 - are completely unclear to me. Why do they exist? What to they do?

Oh, that's creating the _start function, creating enough locals to match main's (or rather, whatever startFunction says the function name is) expected arguments, and calling main with these arguments. It then exports _start and creates the (start $_start) entry.

Thanks, now I see.

Why is this in s2wasm? Should'nt the backend do this?

kripken · 2016-02-05T22:03:25Z

This seems confused to me. Maybe it's me that is confused. There is both a _start function, and a startFunction? Your code creates the former and makes it call the latter? And the former is marked as the wasm "start"?

That seems very convoluted to me. Why not just mark startFunction as the wasm "start"?

kripken · 2016-02-05T22:04:51Z

(I'm fine to iterate on this after the merge, but in the future, how about not merging while there are outstanding questions?)

jfbastien · 2016-02-08T17:38:22Z

Sorry, I didn't realize your question was expressing concern, I thought you just wanted a follow-up :-)

jfbastien assigned kripken Feb 4, 2016

jfbastien mentioned this pull request Feb 4, 2016

Add --entry command line parameter WebAssembly/spec#231

Closed

jfbastien mentioned this pull request Feb 4, 2016

Support start WebAssembly/wabt#34

Closed

kripken reviewed Feb 4, 2016
View reviewed changes

jfbastien force-pushed the start branch from 4f1ba4f to 3fbae87 Compare February 5, 2016 08:56

Review comments.

3140918

jfbastien added a commit that referenced this pull request Feb 5, 2016

Merge pull request #182 from WebAssembly/start

8f67b6e

Support start

jfbastien merged commit 8f67b6e into master Feb 5, 2016

jfbastien deleted the start branch February 5, 2016 10:23

jfbastien mentioned this pull request Feb 5, 2016

Support start in interpreter #186

Closed

kripken reviewed Feb 5, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support start #182

Support start #182

jfbastien commented Feb 4, 2016

jfbastien commented Feb 4, 2016

kripken Feb 4, 2016

jfbastien Feb 5, 2016

kripken Feb 5, 2016

kripken commented Feb 4, 2016

jfbastien commented Feb 5, 2016

kripken commented Feb 5, 2016

kripken Feb 5, 2016

jfbastien Feb 5, 2016

kripken Feb 5, 2016

jfbastien Feb 5, 2016

kripken Feb 5, 2016

jfbastien Feb 5, 2016

kripken Feb 5, 2016

jfbastien commented Feb 5, 2016

kripken Feb 5, 2016

jfbastien Feb 5, 2016

kripken Feb 5, 2016

kripken commented Feb 5, 2016

kripken commented Feb 5, 2016

jfbastien commented Feb 8, 2016

Support start #182

Support start #182

Conversation

jfbastien commented Feb 4, 2016

jfbastien commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Feb 4, 2016

jfbastien commented Feb 5, 2016

kripken commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfbastien commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Feb 5, 2016

kripken commented Feb 5, 2016

jfbastien commented Feb 8, 2016