src: use effective cppgc wrapper id to deduce non-cppgc id #48660

joyeecheung · 2023-07-05T15:48:01Z

deps: V8: cherry-pick 93275031284c

Original commit message:

[cppgc] expose wrapper descriptor on CppHeap

This makes it possible for embedders to:

1. Avoid creating wrapper objects that happen to have a layout that
  leads V8 to consider the object cppgc-managed while it's not.
  Refs: https://github.com/nodejs/node/pull/43521
2. Create cppgc-managed wrapper objects when they do not own the
   CppHeap. Refs: https://github.com/nodejs/node/pull/45704

Bug: v8:13960
Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#88490}

Refs: v8/v8@9327503

src: use effective cppgc wrapper id to deduce non-cppgc id

Previously we hard-code a wrapper id to be used in BaseObjects
to avoid accidentally triggering cppgc on these non-cppgc-managed
objects, but hard-coding can be be hacky and result in mismatch
when we start to create CppHeap ourselves. This patch makes it
more robust by deducing non-cppgc id from the effective cppgc id,
if there is one.

Refs: #48660

nodejs-github-bot · 2023-07-05T15:48:07Z

Review requested:

@nodejs/gyp
@nodejs/startup
@nodejs/v8-update

richardlau · 2023-07-05T15:56:28Z

The V8 cherry-pick should have also bumped v8_embedder_string in common.gypi.

joyeecheung · 2023-07-05T16:14:22Z

@richardlau Thanks for catching it, it probably happened because I created the backport commit before ce4102e landed. I've amended the first commit.

nodejs-github-bot · 2023-07-05T19:04:48Z

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

nodejs-github-bot · 2023-07-05T21:14:34Z

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

richardlau · 2023-07-07T12:00:21Z

Did this get a V8 CI run?

joyeecheung · 2023-07-07T14:54:27Z

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

nodejs-github-bot · 2023-07-07T14:54:36Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5433/

nodejs-github-bot · 2023-07-07T14:54:41Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5433/

nodejs-github-bot · 2023-07-07T14:54:47Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5433/

joyeecheung · 2023-07-08T08:16:15Z

Looks like there is a build error in our build machine, perhaps depot_tools is out of date ?

16:54:59 Cloning into '/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/_depot_tools'...
16:55:05   File "/home/iojs/build/workspace/node-test-commit-v8-linux/deps/v8/_depot_tools/gclient.py", line 3461
16:55:05     f'160000,{value},{name}'
16:55:05                            ^
16:55:05 SyntaxError: invalid syntax

joyeecheung · 2023-07-08T13:15:17Z

Rebased to resolve merge conflict

nodejs-github-bot · 2023-07-08T13:15:44Z

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

nodejs-github-bot · 2023-07-12T14:42:53Z

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

Previously we hard-code a wrapper id to be used in BaseObjects to avoid accidentally triggering cppgc on these non-cppgc-managed objects, but hard-coding can be be hacky and result in mismatch when we start to create CppHeap ourselves. This patch makes it more robust by deducing non-cppgc id from the effective cppgc id, if there is one. PR-URL: nodejs#48660 Refs: v8/v8@9327503 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Previously we hard-code a wrapper id to be used in BaseObjects to avoid accidentally triggering cppgc on these non-cppgc-managed objects, but hard-coding can be be hacky and result in mismatch when we start to create CppHeap ourselves. This patch makes it more robust by deducing non-cppgc id from the effective cppgc id, if there is one. PR-URL: nodejs#48660 Refs: v8/v8@9327503 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Previously we hard-code a wrapper id to be used in BaseObjects to avoid accidentally triggering cppgc on these non-cppgc-managed objects, but hard-coding can be be hacky and result in mismatch when we start to create CppHeap ourselves. This patch makes it more robust by deducing non-cppgc id from the effective cppgc id, if there is one. PR-URL: nodejs#48660 Refs: v8/v8@9327503 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Previously we hard-code a wrapper id to be used in BaseObjects to avoid accidentally triggering cppgc on these non-cppgc-managed objects, but hard-coding can be be hacky and result in mismatch when we start to create CppHeap ourselves. This patch makes it more robust by deducing non-cppgc id from the effective cppgc id, if there is one. PR-URL: nodejs#48660 Refs: v8/v8@9327503 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: nodejs#43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: nodejs#45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: nodejs#48660 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Previously we hard-code a wrapper id to be used in BaseObjects to avoid accidentally triggering cppgc on these non-cppgc-managed objects, but hard-coding can be be hacky and result in mismatch when we start to create CppHeap ourselves. This patch makes it more robust by deducing non-cppgc id from the effective cppgc id, if there is one. PR-URL: nodejs#48660 Refs: v8/v8@9327503 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>

Original commit message: [cppgc] expose wrapper descriptor on CppHeap This makes it possible for embedders to: 1. Avoid creating wrapper objects that happen to have a layout that leads V8 to consider the object cppgc-managed while it's not. Refs: #43521 2. Create cppgc-managed wrapper objects when they do not own the CppHeap. Refs: #45704 Bug: v8:13960 Change-Id: If31f4d56c5ead59dc0d56f937494d23d631f7438 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4598833 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/main@{#88490} Refs: v8/v8@9327503 PR-URL: #48660 Backport-PR-URL: #49187 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit

Previously we hard-code a wrapper id to be used in BaseObjects to avoid accidentally triggering cppgc on these non-cppgc-managed objects, but hard-coding can be be hacky and result in mismatch when we start to create CppHeap ourselves. This patch makes it more robust by deducing non-cppgc id from the effective cppgc id, if there is one. PR-URL: #48660 Backport-PR-URL: #49187 Refs: v8/v8@9327503 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Refs: #40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit

This patch: - Initializes cppgc in InitializeOncePerProcess() when kNoInitializeCppgc is not set - Create a CppHeap and attach it to the Isolate when there isn't one already during IsolateData initialization. The CppHeap is detached and terminated when IsolateData is freed. - Publishes the cppgc headers in the tarball. This allows C++ addons to start using cppgc to manage objects. A helper node::SetCppgcReference() is also added to help addons enable cppgc tracing in a user-defined object. Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> Refs: #40786 PR-URL: #45704 Backport-PR-URL: #49187 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> PR-URL: #48660 Refs: v8/v8@9327503

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 PR-URL: #49185

deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) #48660 and #45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49185

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) #48660 and #45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49185

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) nodejs#48660 doc: * add new TSC members (Michael Dawson) nodejs#48841 * add rluvaton to collaborators (Raz Luvaton) nodejs#49215 esm: * unflag import.meta.resolve (Guy Bedford) nodejs#49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) nodejs#48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) nodejs#48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) nodejs#48765 module: * implement `register` utility (João Lenon) nodejs#46826 * make CJS load from ESM loader (Antoine du Hamel) nodejs#47999 src: * add built-in `.env` file support (Yagiz Nizipli) nodejs#48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) nodejs#48660 and nodejs#45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs#48975 PR-URL: nodejs#49185

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 5, 2023

joyeecheung force-pushed the wrapper branch from b221c47 to b84c5e0 Compare July 5, 2023 16:12

joyeecheung mentioned this pull request Jul 5, 2023

src,tools: initialize cppgc #45704

Merged

joyeecheung force-pushed the wrapper branch from b84c5e0 to 168c23a Compare July 5, 2023 18:35

This was referenced Jul 6, 2023

CI Reliability 2023-07-06 nodejs/reliability#609

Open

CI Reliability 2023-07-07 nodejs/reliability#610

Open

legendecas approved these changes Jul 7, 2023

View reviewed changes

gengjiawen approved these changes Jul 7, 2023

View reviewed changes

gengjiawen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 7, 2023

joyeecheung removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 7, 2023

github-actions bot mentioned this pull request Jul 8, 2023

CI Reliability 2023-07-08 nodejs/reliability#611

Open

23 tasks

joyeecheung force-pushed the wrapper branch from 168c23a to f5abc09 Compare July 8, 2023 13:14

This was referenced Jul 9, 2023

CI Reliability 2023-07-09 nodejs/reliability#612

Open

CI Reliability 2023-07-10 nodejs/reliability#613

Open

richardlau mentioned this pull request Jul 10, 2023

deps: update V8 to 11.6 #48710

Closed

joyeecheung mentioned this pull request Aug 15, 2023

[v20.x backport] cppgc related changes #49187

Closed

UlisesGascon mentioned this pull request Aug 18, 2023

v20.6.0 proposal #49185

Merged

targos added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels Nov 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use effective cppgc wrapper id to deduce non-cppgc id #48660

src: use effective cppgc wrapper id to deduce non-cppgc id #48660

joyeecheung commented Jul 5, 2023 •

edited

Loading

nodejs-github-bot commented Jul 5, 2023

richardlau commented Jul 5, 2023

joyeecheung commented Jul 5, 2023

nodejs-github-bot commented Jul 5, 2023

nodejs-github-bot commented Jul 5, 2023

richardlau commented Jul 7, 2023

joyeecheung commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

joyeecheung commented Jul 8, 2023

joyeecheung commented Jul 8, 2023

nodejs-github-bot commented Jul 8, 2023

nodejs-github-bot commented Jul 12, 2023

src: use effective cppgc wrapper id to deduce non-cppgc id #48660

src: use effective cppgc wrapper id to deduce non-cppgc id #48660

Conversation

joyeecheung commented Jul 5, 2023 • edited Loading

deps: V8: cherry-pick 93275031284c

src: use effective cppgc wrapper id to deduce non-cppgc id

nodejs-github-bot commented Jul 5, 2023

richardlau commented Jul 5, 2023

joyeecheung commented Jul 5, 2023

nodejs-github-bot commented Jul 5, 2023

nodejs-github-bot commented Jul 5, 2023

richardlau commented Jul 7, 2023

joyeecheung commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

nodejs-github-bot commented Jul 7, 2023

joyeecheung commented Jul 8, 2023

joyeecheung commented Jul 8, 2023

nodejs-github-bot commented Jul 8, 2023

nodejs-github-bot commented Jul 12, 2023

joyeecheung commented Jul 5, 2023 •

edited

Loading