v8: patch API to be compatible with v8 6.0

[psmarshall]

This is an extension of #12875 which takes into account more recent V8 API changes on the 6.0 branch. V8 6.0 should not have any more API changes, so this should be the final piece to make the Node 8.x branch API compatible with V8 6.0.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

@addaleax @MylesBorins

Affected core subsystem(s)

deps/v8

[psmarshall]

I didn't include the following commits on 6.0 because they were purely adding stuff:

• https://chromium.googlesource.com/v8/v8.git/+/18a26cfe17419633856e614577a84cc1e8385e14
• https://chromium.googlesource.com/v8/v8.git/+/f5fad6d9b85d31fd5d05b5c86bf2f2162391c589

[addaleax]

Hi, thanks for doing this!

I think we’re already floating https://chromium.googlesource.com/v8/v8.git/+/f5fad6d9b85d31fd5d05b5c86bf2f2162391c589 (and as you said it’s additive, so it’s okay anyway), but https://chromium.googlesource.com/v8/v8.git/+/18a26cfe17419633856e614577a84cc1e8385e14 is breaking the ABI because it’s adding virtual methods.

[psmarshall]

Ahh OK thanks, I'll add https://chromium.googlesource.com/v8/v8.git/+/18a26cfe17419633856e614577a84cc1e8385e14.

[jasnell]

Thank you.

@nodejs/ctc ... we should fast track this one. I'd like to get it landed no later than end of day tomorrow.

@psmarshall @targos @fhinkel ... is this the final piece that is needed to ensure 6.0 ABI compatibility? Is there any chance we'll need any other PRs?

[jasnell]

The commit messages need to be changed to match our guidelines.

[addaleax]

Looking through the header diff, these are the remaining breaking changes for some value of “breaking”:

• v8/v8@faf5f52 – Deprecation of “everything in v8-debug.h”. It’s only compiler warnings, though.
• v8/v8@4f82f1d – Follow up to what is in this PR as 31ca76a0d1e1266b24e6504e35912c4c09330001; We can work around this easily if necessary but it would not hurt to include it here.
• v8/v8@43791ce – I don’t think this is something addons actually use, so we are probably good here.

(plus what is mentioned above, ofc)

(oh, and: runtime flags. still not sure whether we should/can do anything about those… ¯\_(ツ)_/¯)

[fhinkel]

Cc @nodejs/V8

This should be the final piece.

[fhinkel]

Lgtm if the commit messages are changed to satisfy our guidelines.

[mscdex]

/cc @nodejs/v8 ^

[bnoordhuis]

LGTM. Would it make sense to remove FunctionTemplate::NewWithFastHandler() now since it's been removed in 6.0?

It lives in the experimental namespace and I don't think anything or anyone actually uses it so I'm perfectly okay with saying 'no'.

[psmarshall]

@addaleax
I've added the three extra CLs you mentioned (v8/v8@faf5f52, v8/v8@4f82f1d and v8/v8@43791ce), thanks for that. Maybe the last one isn't 100% essential but it patched easily so no big hassle there.

I am still trying to get v8/v8@18a26cf to patch cleanly, the CL also contains some other refactoring which is not playing nicely. Once that is ready, I think that will be everything we need. WDYT?

I am not sure about FunctionTemplate::NewWithFastHandler(), which header file did that affect?

[addaleax]

I am not sure about FunctionTemplate::NewWithFastHandler(), which header file did that affect?

That seems to have been v8/v8@2cd2f5f, which we already applied in 0ce23da – I think @bnoordhuis was just looking at the master include/v8.h, not the v8.x-staging one, so he missed that.

I am still trying to get v8/v8@18a26cf to patch cleanly, the CL also contains some other refactoring which is not playing nicely. Once that is ready, I think that will be everything we need. WDYT?

If you can make that happen, yes, that would be great. It seems to actually change the requirements for ArrayBuffer::Allocator instances, and I have to admit I don’t quite understand how exactly the new methods interact; I hope it’s not too off-topic if I ask a few questions:

• Should we make the methods pure virtual ourselves, right now? It seems that’s what their current semantics are anyway, and that they will be pure virtual at some point in the future. It would be a last-minute breaking change, but looking forward it seems to make the most sense.
• What does Must call |SetProtection| to make memory accessible. in the Reserve doc comment mean? Is SetProtection called by V8, or is it supposed to be called from within Reserve (the latter is what the wording implies but it doesn’t seem to make sense)?
• Is Reserve(·) + SetProtection(·, ·, Protection::kReadWrite) equivalent to AllocateUninitialized(·)?
• Would it be valid to just make Reserve() an AllocateUninitialized() call and SetProtection() a no-op? (Alternatively: Do Reserve() and SetProtection() need to work on page-aligned memory regions?)
• How can the new methods be triggered? We should probably have a test for them in Node anyway, since we likely need to implement them.
• Related: Are they only called as part of WASM code? That’s no longer an experimental feature, right?

(I hope that helps clarify about what exactly is breaking here and what not; and at the very least the answers would probably make a good documentation update.)

/cc @eholk

[eholk]

A little background on the new Allocator changes: For WebAssembly, we're adding a feature that elides explicit bounds checks in favor of using guard regions. Accesses to these guard regions trap in the OS, and V8 is able to recover from the trap, throwing a JavaScript exception. This requires the embedder (Node in this case) to get involved, since the contract is that all array buffer allocations are done through an embedder-supplied allocator.

When creating a Wasm memory, V8 will call Allocator::Reserve to reserve a large chunk of address space (8 gigs currently, but will likely increase to 16). On POSIX, you would typically implement this using mmap with the prot arguments set to PROT_NONE. On Windows you'd call VirtualAlloc with MEM_RESERVE for the dfAllocationType parameter. As long as the memory is not committed it does not count against you, which is important here because we are allocating so much.

Then V8 will call Allocator::SetProtection on a subrange of the previously reserved memory. This makes it so a portion of the memory is read/write. On POSIX, this can be done with a call to mprotect and on Windows you would use a combination of calling VirtualAlloc with MEM_COMMIT and the read/write permission flag.

V8 includes a default implementation of these new functions: https://cs.chromium.org/chromium/src/v8/src/api.cc?type=cs&q=ArrayBuffer::Allocator+package:%5Echromium$&l=468

Now on to your questions:

• These methods should be implemented by Node, just like the other methods in ArrayBuffer::Allocator. Eventually they will be pure virtual in v8.h, but for now we have a placeholder implementation so that embedders won't immediately fail to compile. The trap-based bounds check feature isn't on by default yet, so you can hold off a little bit to implement these if you want.
• I wrote that comment from the perspective of caller of Allocator. Reserve gives you a pointer, but you'll get a segfault if you try to dereference it. In order to access this memory, you have to set it as read/write with SetProtection. SetProtection will be called by V8.
• Basically. The main difference is that SetProtection need not cover the whole reservation.
• No. The important thing about Reserve is that accessing memory returned by Reserve gives a segfault unless we've used SetProtection to make it read/write first. Otherwise, out of bounds memory accesses from WebAssembly will start to succeed. In every OS I know of, memory mapping and protection is done at page granularity. I don't think V8 particularly requires it, but the code was written under that assumption and the odd DCHECK might trip if this is not true.
• V8 will call these methods when creating a WebAssembly.Memory object if guard regions are enabled. This can be done with the --enable_guard_regions flag or --wasm_trap_handler, which implies --enable_guard_regions.
• Yes, these are only called from Wasm code. Wasm is no longer experimental, but trap-based bounds checks are for now. Trap-based bounds checks are only feasible on 64-bit platforms, and for the moment we only support Linux x86_64. Other platforms will come later. My plan is that Windows will be next in line after Linux.

Hopefully this helps! Feel free to ask more questions if anything is unclear.

[addaleax]

@eholk Yes, thank you – this is definitely helpful and makes a lot of sense. Just one small follow-up: Do you have an estimate for when/in which V8 version that feature leaves experimental state? I.e. starting from when will embedders absolutely have to provide these methods?

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/10173/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/717/

[eholk]

@addaleax - We're planning to turn it on by default for Linux in Chrome 61, which I think corresponds to V8 6.1. That said, the feature requires some more help from the embedder to set up the signal handler and such, so it will be up to the embedder to opt in. Until you're ready to turn on traps in Node, it should be fine to have Reserve return nullptr and SetProtection be a no-op.

[psmarshall]

I think this is ready to go from my point of view - what needs to happen next?

[addaleax]

I agree, this is ready – somebody will need to merge this, but since @jasnell is managing the 8.0.0 release and therefore implicitly the v8.x(-staging) branches, too, it seems easiest to let him do that.