Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v20.x backport] cppgc related changes #49187

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

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
PR-URL: #48660
Reviewed-By: Chengzhong Wu legendecas@gmail.com
Reviewed-By: Jiawen Geng technicalcute@gmail.com

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.

PR-URL: #48660
Refs: v8/v8@9327503
Reviewed-By: Chengzhong Wu legendecas@gmail.com
Reviewed-By: Jiawen Geng technicalcute@gmail.com

src,tools: initialize cppgc

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
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

@joyeecheung joyeecheung changed the base branch from main to v20.x-staging August 15, 2023 19:41
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Aug 15, 2023
@joyeecheung
Copy link
Member Author

oops, wrong base branch, it looks like github is not smart enough to pick it up after changing the base..opening a new one

@joyeecheung
Copy link
Member Author

Oh, looks like it was because v20.x-staging was force-pushed. Rebasing it made it work again

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@RafaelGSS RafaelGSS force-pushed the v20.x-staging branch 2 times, most recently from 2a9aa46 to 21949c4 Compare August 17, 2023 11:48
joyeecheung and others added 3 commits August 17, 2023 19:02
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>
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: nodejs#40786
PR-URL: nodejs#45704
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>
@joyeecheung joyeecheung mentioned this pull request Aug 17, 2023
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 17, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

UlisesGascon pushed a commit that referenced this pull request Aug 18, 2023
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
UlisesGascon pushed a commit that referenced this pull request Aug 18, 2023
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
UlisesGascon pushed a commit that referenced this pull request Aug 18, 2023
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
@UlisesGascon
Copy link
Member

Landed in 00fc8bb...099159c

minglechen pushed a commit to CTSRD-CHERI/node that referenced this pull request Dec 11, 2024
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
Backport-PR-URL: nodejs#49187
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Refs: nodejs#40786
Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants