1500203 - Baldr: add wasmCompileInSeparateProcess() testing function

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Luke Wagner [:luke]]

To cache cases where asm.js compilation accidentally bakes in a process global address, asm.js uses nestedShell() and a rather magic mechanism for sharing asm.js cache entries between processes.

WebAssembly tests caching via the more general streamCacheEntry() primitive, but this doesn't interact at all with nestedShell() and so we don't have a way to test that we don't bake process global addresses into wasm code.  I was thinking a nice clean design might be to allow passing a stream cache entry object to nestedShell() as an argument that gets passed to the nestedShell() script as a parameter.

Comment 1

[Luke Wagner [:luke]]

Attachment: almost done (just needs Windows)

Comment 2

[Luke Wagner [:luke]]

Attachment: nested-shell-testing

Now with Windows support.  I can see now why the fork()+exec() pattern is actually useful (or the file-actions argument in posix_spawn()).

Comment 3

[Luke Wagner [:luke]]

Attachment: rm-header-dep

Working on the previous patch, I noticed that js.cpp was over-including SM internals; it's nice to have it use the same JS API as Gecko.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 9022641 [details] [diff] [review]
nested-shell-testing

Need a few tweaks found by try and found a bit better factoring of pipe code.

Comment 5

[Luke Wagner [:luke]]

Attachment: nested-shell-testing

Much better factored this time, and now propagating flags so that the cpuid in the child is the same as the parent.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9022781 [details] [diff] [review]
nested-shell-testing

Review of attachment 9022781 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, modulo a few minor issues.

::: js/src/shell/js.cpp
@@ +5839,5 @@
> +            if (readCount < 0) {
> +                if (errno == EINTR) {
> +                    continue;
> +                }
> +                return false;

Can this just be `if (readCount < 0 && errno != EINTR) { return false; }`, and then we don't even need the do while(false) block?

@@ +5867,5 @@
> +    }
> +
> +    ~AutoPipe() {
> +        if (fds_[0] != -1) {
> +            close(fds_[0]);

close() can return an error, so we need to check the return value (in particular, do we want to ignore INTR here too?)

@@ +5870,5 @@
> +        if (fds_[0] != -1) {
> +            close(fds_[0]);
> +        }
> +        if (fds_[1] != -1) {
> +            close(fds_[1]);

ditto

@@ +5876,5 @@
> +    }
> +
> +    bool init() {
> +#ifdef XP_WIN
> +        return !_pipe(fds_, 4096, O_BINARY)) {

nit: no { and ; at the end

@@ +5994,5 @@
> +        close(stdIn.reader());
> +        close(stdIn.writer());
> +        close(stdOut.reader());
> +        close(stdOut.writer());
> +        execv(sArgv[0], argv.get());

ditto x4 for the return values of close + 1 for execv.

@@ +6004,5 @@
> +    // reading ends to hit EOF.
> +    stdIn.closeReader();
> +    stdOut.closeWriter();
> +
> +    if (write(stdIn.writer(), bytecode, bytecodeLength) < 0) {

Don't we need to loop here to make sure all the content is written, in case write() returns < bytecodeLength?
Also, compare with equality against -1 for consistency?

@@ +6024,5 @@
> +    }
> +#else
> +    do {
> +        if (waitpid(childPid, &status, 0) < 0) {
> +            if (errno == EINTR) {

same question about the errno business and do while(false) loop?

@@ +6065,5 @@
> +    if (!wasm::CompileAndSerialize(*bytecode, &serialized)) {
> +        return false;
> +    }
> +
> +    if (write(stdOut, serialized.begin(), serialized.length()) == -1) {

Same question about write.

@@ +6069,5 @@
> +    if (write(stdOut, serialized.begin(), serialized.length()) == -1) {
> +        return false;
> +    }
> +
> +    return true;

Should Windows close the stdIn/stdOut before returning?

@@ +10681,5 @@
>  static int
>  Shell(JSContext* cx, OptionParser* op, char** envp)
>  {
> +    if (op->getBoolOption("wasm-compile-and-serialize")) {
> +        return WasmCompileAndSerialize(cx) ? EXIT_SUCCESS : -1;

Won't there be an issue somewhere up if the cx had an exception (e.g. oom in cx allocation) that didn't get reported?

Comment 7

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
Thanks, and great comments/catches!

> Can this just be `if (readCount < 0 && errno != EINTR) { return false; }`,
> and then we don't even need the do while(false) block?

Oh wow, I was n00bishly mistaken in thinking that 'continue' does not execute the do{}while(c) condition c, but of course it does and so you're right that your suggested rewrite would be equivalent to what's written.  But I actually *wanted* the loop when EINTR, so I'll rewrite to this (and the waitpid() loop below) into the more canonical form I should have written the first time:

   while (true) {
       readCount = read(fd, bytes->begin() + lastLength, ChunkSize);
       if (readCount >= 0) { break; }
       if (errno != EINTR) { return false; }
   }


> close() can return an error, so we need to check the return value (in
> particular, do we want to ignore INTR here too?)

There's nothing to do if close() fails; it'd be like a destructor or free() or delete failing.  EINTR is only a case you have to worry about for certain POSIX calls like reads/writes/waits.

> Don't we need to loop here to make sure all the content is written, in case
> write() returns < bytecodeLength?

Great point; and I forgot about the EINTR case.  I'll factor out a WriteAll() loop for the two write()s.

> Should Windows close the stdIn/stdOut before returning?

Process exit will close all file descriptors, not just stdin/stdout.

> 
> > +    if (op->getBoolOption("wasm-compile-and-serialize")) {
> > +        return WasmCompileAndSerialize(cx) ? EXIT_SUCCESS : -1;
> 
> Won't there be an issue somewhere up if the cx had an exception (e.g. oom in
> cx allocation) that didn't get reported?

We can actually MOZ_ASSERT(!cx->isExceptionPending()) here, but that's locally non-obvious, so I'll add that assert and comment why.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4068f0a3955
Baldr: reduce js.cpp dependency on wasm internal headers (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e842238034c
Baldr: add wasmCompileInSeparateProcess() shell testing function and tests using it (r=bbouvier)

Comment 9

[Luke Wagner [:luke]]

ni? just for FYI: this new shell function, wasmCompileInSeparateProcess(), would be ideal for occasional fuzzing runs to help things like bug 1499198.  It's basically like `new WebAssembly.Module(buffer)`.

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/f4068f0a3955
https://hg.mozilla.org/mozilla-central/rev/6e842238034c