deps: patch the V8 API to be forward compatible with 6.7

[psmarshall]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

@addaleax @MylesBorins

[targos]

Thanks!

Can you please:

• Increment the embedder string in common.gypi
• Increment NODE_MODULE_VERSION and add a line above it for V8 6.7: https://github.com/nodejs/node/blob/master/src/node_version.h#L109-L113

[targos]

Diff:

https://gist.github.com/targos/5b700e76921be024a7ea06127953cf93

Edit: to be clear, this is the output of git diff -- deps/v8/include between this PR and #19989

[targos]

Is this part OK?

https://gist.github.com/targos/5b700e76921be024a7ea06127953cf93#file-v8-6-7-abi-patch-diff-L347-L364

@@ -6193,24 +6263,6 @@ typedef void* (*CreateHistogramCallback)(const char* name,
 
 typedef void (*AddHistogramSampleCallback)(void* histogram, int sample);
 
-// --- Memory Allocation Callback ---
-enum ObjectSpace {
-  kObjectSpaceNewSpace = 1 << 0,
-  kObjectSpaceOldSpace = 1 << 1,
-  kObjectSpaceCodeSpace = 1 << 2,
-  kObjectSpaceMapSpace = 1 << 3,
-  kObjectSpaceLoSpace = 1 << 4,
-  kObjectSpaceAll = kObjectSpaceNewSpace | kObjectSpaceOldSpace |
-                    kObjectSpaceCodeSpace | kObjectSpaceMapSpace |
-                    kObjectSpaceLoSpace
-};
-
-  enum AllocationAction {
-    kAllocationActionAllocate = 1 << 0,
-    kAllocationActionFree = 1 << 1,
-    kAllocationActionAll = kAllocationActionAllocate | kAllocationActionFree
-  };
-
 // --- Enter/Leave Script Callback ---
 typedef void (*BeforeCallEnteredCallback)(Isolate*);
 typedef void (*CallCompletedCallback)(Isolate*);

[psmarshall]

I amended the commit with the version number increments.

I think it's OK to leave the ObjectSpace and AllocationAction enums. They aren't actually used in the API, and if any embedder did use them, they are essentially just integers anyway, so I guess it won't cause breakage. The reason I left them is because they are actually still used internally somewhere in 6.6, so I would need to backport the removal CL rather than just delete them from the API.

[targos]

CI: https://ci.nodejs.org/job/node-test-pull-request/14246/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1321/

[bnoordhuis]

In file included from ../deps/v8/src/allocation.h:8:
../deps/v8/include/v8-platform.h:339:5: error: use of undeclared identifier 'abort'
    abort();
    ^
../deps/v8/include/v8-platform.h:364:5: error: use of undeclared identifier 'abort'
    abort();

Probably missing an #include <cstdlib>.

[psmarshall]

Updated it with the include, thanks

[bnoordhuis]

Cheer. New CI:

https://ci.nodejs.org/job/node-test-pull-request/14252/
https://ci.nodejs.org/job/node-test-commit-v8-linux/1323/

[addaleax]

This is supposed to say NumberOfWorkerThreads(), right? Not that it matters much, but the message might be … confusing? 😄

[targos]

This typo is still present in V8 master, so should be fixed there first.

[MylesBorins]

Do we want to land this on master or on 10.x-staging?

[targos]

Why not on master?

[addaleax]

The last time around we didn’t land it on master, but I agree that I don’t really see a problem with doing so.

[jasnell]

If this is going directly to 10.x-staging, please allow me to land it as part of the next round of updates that I make to that. If it lands in master, I'll cherry-pick it back.

[hashseed]

Thanks, Peter!

[targos]

New CI: https://ci.nodejs.org/job/node-test-pull-request/14347/

[targos]

CI is green.

I'm going to land this on master if there are no objections. That way we can immediately work on replacing our usage of deprecated APIs

[jasnell]

@targos ... if you can, please land this today before noon pacific so I can make sure it is in today's 10.0.0 test build.

[targos]

This has to land after #20105. I won't be around soon enough to do it myself.

[MylesBorins]

landed in 8bcefd0
Thanks @psmarshall