call_indirect will test the code pointer for null before using it. This is not the end of the world:

00000055  45 85 f6                  test %r14d, %r14d
00000058  0f 85 02 00 00 00         jnz 0x0000000000000060
0000005E  0f 0b                     ud2

and it may become even better if we start jumping out-of-line to the trap rather than skipping over the trap (bug 1680243), or if the skip across the trap uses a short jump, as it could do here and virtually everywhere.

However, best might be if we could avoid this check altogether. One option mentioned in bug 1340235 comment 13 is to handle the SIGSEGV from the jump to the null pointer, though I'm not sure how that plays now that we have multiple entry points to functions. Another option, since the code pointer loaded from the table is a code pointer that does not escape as a value into wasm code, is to represent a null code pointer not as null, but as a pointer to a procedure that always throws. table.init et al would store this special function pointer for null values; table.get would translate the value into a null pointer. In the future, when the representation of the function in the table is its code pointer, that might add some complication however, so there's a balance here.

Simple test case for the shell to exhibit the problem.

Looking at the code in detail, it turns out we currently check for null against the tls pointer that's stored in the table. This is fine; a dummy tls is more or less just as good as a dummy function. But it will definitely make it harder to keep this optimization once the table representation changes.

(In reply to Lars T Hansen [:lth] from comment #2)

Looking at the code in detail, it turns out we currently check for null against the tls pointer that's stored in the table. This is fine; a dummy tls is more or less just as good as a dummy function. But it will definitely make it harder to keep this optimization once the table representation changes.

Dmitry's patch changes this so that it checks the code pointer, since the tls never needs to be loaded on that path any longer. There's no logical need for the check to check the tls, either.

First, hide the representation of a null function element in the Table
module as much as possible.

Second, hide the representation of a null code pointer behind a
function that can return nullptr or "something else".

Third, initialize null function elements with the abstract null
pointer when it is not all-zero-bits, and use the abstract null
pointer to determine whether null checking is needed in
wasmCallIndirect.

Sketch: Handle indirect-call-to-null with the trap handler.

This works, and generalizes, and is nicely local, but:

• it depends on the call going directly to the pointer, not to the
  pointer plus some offset
• it depends on specific instructions being emitted
• it is unlikely to be completely safe since on x86 and x64 it
  assumes that the stack pointer is pointing to accessible memory

Depends on D132183

I think the signal handling solution (https://phabricator.services.mozilla.com/D132296) is pretty clean now and I have it on Try. I'll benchmark it, but even if it doesn't move the needle much it may be worthwhile eventually because it opens up for better code generation around the call.

Now to find a reviewer for this...

https://treeherder.mozilla.org/jobs?repo=try&revision=14832f51c5a0d66b880b3cbee0e64cc01a208678

https://hg.mozilla.org/mozilla-central/rev/e6529d3eb27e

This was backed out as part of the big backout in bug 1753061. The code that lands in its stead (bug 1754377) will not need a null check on the fast path, but there is an option for removing the null check on the slow path using the code in this patch, see TODO in the patch on the latter bug.

If we re-land this for the slow call path we will probably not need the additional fix from bug 1747540, as the problem that caused that was a consequence of fast-path like code.

Avoid a null check on the slow path of call_indirect by catching the
signal resulting from dereferencing a null Tls pointer when loading
the heap register for the callee instance.

This does not work on x86-32 because we don't have a heap register to
load on that platform.

Depends on D138052

Patch is complete apart from figuring out how to do the ifdef: it's really not necessary for there to be an enable/disable switch for this, there just needs to be an ifdef that's set right, somewhere.

(In reply to Lars T Hansen [:lth] from comment #0)

call_indirect will test the code pointer for null before using it. This is not the end of the world:

00000055  45 85 f6                  test %r14d, %r14d
00000058  0f 85 02 00 00 00         jnz 0x0000000000000060
0000005E  0f 0b                     ud2

This is not important for reviewing the patch, but .. why does the
above example do a null check which ignores the upper 32 bits?

(In reply to Julian Seward [:jseward] from comment #14)

(In reply to Lars T Hansen [:lth] from comment #0)

call_indirect will test the code pointer for null before using it. This is not the end of the world:

00000055  45 85 f6                  test %r14d, %r14d
00000058  0f 85 02 00 00 00         jnz 0x0000000000000060
0000005E  0f 0b                     ud2

This is not important for reviewing the patch, but .. why does the
above example do a null check which ignores the upper 32 bits?

A very, very old bug that was subsequently fixed with bug 1743234 / commit 54664793b10f because it was exposed by Dmitry's patch.

https://hg.mozilla.org/mozilla-central/rev/bc9307b1fb7e

Change the status for beta to have the same as nightly and release.
For more information, please visit auto_nag documentation.