FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: cherry-pick 9a49b22 from V8 upstream by danbev · Pull Request #35939 · nodejs/node · GitHub

/ node Public

deps: cherry-pick 9a49b22 from V8 upstream - #35939

Closed
danbev wants to merge 2 commits into
nodejs:masterfrom
danbev:deps-v8-asan-issue
ConversationCommits (2)ChecksFiles changed
Closed

deps: cherry-pick 9a49b22 from V8 upstream#35939
danbev wants to merge 2 commits into
nodejs:masterfrom
danbev:deps-v8-asan-issue

Conversation

danbev commented Nov 3, 2020

Copy link
Copy Markdown
Contributor

Original commit message:
Fix alloc/dealloc size mismatch for v8::BackingStore

On newer compilers the {operator delete} with explicit {size_t}
argument would be instantiated for {v8::BackingStore} and used
in the destructor of {std::unique_ptrv8::BackingStore}. The {size_t}
argument is wrong though, since the pointer actually points
to a {v8::internal::BackingStore} object.
The solution is to explicitly provide a {operator delete}, preventing
an implicitly generated {size_t} operator.

Bug:v8:11081

Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
Commit-Queue: Ulan Degenbaev ulan@chromium.org
Reviewed-by: Camillo Bruni cbruni@chromium.org
Cr-Commit-Position: refs/heads/master@{#70916}

Refs: v8/v8@9a49b22
Fixes: #35669

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

deps: cherry-pick 9a49b22 from V8 upstream
d9e2331 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

Refs: v8/v8@9a49b22
Fixes: nodejs#35669
nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Nov 3, 2020

ExE-Boss commented Nov 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Since this applies a patch to V8 that causes a difference from the official V8 build for the equivalent V8 version, the V8_EMBEDDER_STRING has to be incremented:

node/common.gypi

Lines 37 to 39 in 1a29c09

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.16',

danbev commented Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

Since this applies a patch to V8 that causes a difference from the official V8 build for the equivalent V8 version

Thanks, I've updated this now.

targos commented Nov 3, 2020

Copy link
Copy Markdown
Member

Should it land on v14.x or lower?

danbev commented Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

Should it land on v14.x or lower?

It should be safe to land this in lower versions as well as v14.x 👍

nodejs-github-bot commented Nov 5, 2020

Copy link
Copy Markdown
Collaborator

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

danbev commented Nov 6, 2020

Copy link
Copy Markdown
Contributor Author

There are some failing CI runs which I've tried to re-run and get them to pass but for example commit-linux-containered looks like there might be an issue with available RAM.

I'd like to merge this PR as I think it would be helpful and prevent test-asan failures in other PRs.

nodejs-github-bot commented Nov 6, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Nov 8, 2020

Copy link
Copy Markdown
Collaborator

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

danbev commented Nov 9, 2020

Copy link
Copy Markdown
Contributor Author

Landed in 1bba8c8.

danbev closed this Nov 9, 2020
danbev added a commit that referenced this pull request Nov 9, 2020
deps: cherry-pick 9a49b22 from V8 upstream
1bba8c8 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

PR-URL: #35939
Fixes: #35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
danbev deleted the deps-v8-asan-issue branch November 9, 2020 10:34
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
deps: cherry-pick 9a49b22 from V8 upstream
7cb5c09 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

PR-URL: #35939
Fixes: #35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
danielleadams mentioned this pull request Nov 9, 2020
Merged
targos pushed a commit to targos/node that referenced this pull request Nov 15, 2020
deps: cherry-pick 9a49b22 from V8 upstream
59fe6e4 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

PR-URL: nodejs#35939
Fixes: nodejs#35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos pushed a commit to targos/node that referenced this pull request Nov 19, 2020
deps: cherry-pick 9a49b22 from V8 upstream
1772fe1 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

PR-URL: nodejs#35939
Fixes: nodejs#35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
deps: cherry-pick 9a49b22 from V8 upstream
78ab197 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

PR-URL: #35939
Fixes: #35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
deps: cherry-pick 9a49b22 from V8 upstream
9db4cea 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

PR-URL: #35939
Fixes: #35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
deps: cherry-pick 9a49b22 from V8 upstream
d2ee676 
Original commit message:
  Fix alloc/dealloc size mismatch for v8::BackingStore

  On newer compilers the {operator delete} with explicit {size_t}
  argument would be instantiated for {v8::BackingStore} and used
  in the destructor of {std::unique_ptr<v8::BackingStore>}. The {size_t}
  argument is wrong though, since the pointer actually points
  to a {v8::internal::BackingStore} object.
  The solution is to explicitly provide a {operator delete}, preventing
  an implicitly generated {size_t} operator.

  Bug:v8:11081

  Change-Id: Iee0aa47a67f0e41000bea628942f7e3d70198b83
  Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2506712
  Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#70916}

PR-URL: #35939
Fixes: #35669
Refs: v8/v8@9a49b22
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
cjihrig cjihrig approved these changes
gengjiawen gengjiawen approved these changes
richardlau richardlau approved these changes
+2 more reviewers
targos targos approved these changes
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

asan-test failure

9 participants

Back | FazBrowse Home | New Git URL