Fix segfault during GC #5900 (4.x) - #7303
Closed
ofrobots wants to merge 2 commits into
Closed
Conversation
Copy link
Copy Markdown
Contributor
Sorry, something went wrong.
This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: nodejs#5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871
This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: nodejs#5900
Copy link
Copy Markdown
Member
|
The patch LGTM. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
LGTM. Do the V8 and node.js test suite pass with this change? |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
CI: https://ci.nodejs.org/job/node-test-pull-request/3003/ |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
|
@ofrobots do we have understand of what particular call site is causing segfault? Rubber-stamp LGTM |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
@indutny The crash happens when the StoreBuffer is iterating pointers to new space: (lldb) bt
* thread #1: tid = 0x135643b, 0x00000001003956c4 node`v8::internal::StoreBuffer::IteratePointersToNewSpace(void (*)(v8::internal::HeapObject**, v8::internal::HeapObject*)) + 1972, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x7)
* frame #0: 0x00000001003956c4 node`v8::internal::StoreBuffer::IteratePointersToNewSpace(void (*)(v8::internal::HeapObject**, v8::internal::HeapObject*)) + 1972
frame #1: 0x0000000100331899 node`v8::internal::Heap::Scavenge() + 1273
frame #2: 0x00000001003301cf node`v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) + 879
frame #3: 0x000000010032fb91 node`v8::internal::Heap::CollectGarbage(v8::internal::GarbageCollector, char const*, char const*, v8::GCCallbackFlags) + 689
frame #4: 0x00000001002e88bc node`v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) + 108
frame #5: 0x000000010053431e node`v8::internal::Runtime_AllocateInTargetSpace(int, v8::internal::Object**, v8::internal::Isolate*) + 110
|
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
And there was a secondary bug in --verify-heap. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
It seems like it is not possible to run the V8 tests in CI on the v4.x branch? (/cc @mhdawson). See https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/135/. I will run them manually and report back. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
Author
|
V8 tests pass for me when run manually on a mac. |
Sorry, something went wrong.
MylesBorins
force-pushed
the
v4.x-staging
branch
3 times, most recently
from
ed3d372 to
f14d9cf
Compare
June 28, 2016 22:49
Copy link
Copy Markdown
Contributor
|
running v8 tests in CI https://ci.nodejs.org/job/node-test-commit-v8-linux/171/ Will land if green |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
CI is green LGTM |
Sorry, something went wrong.
MylesBorins
pushed a commit
that referenced
this pull request
Jul 1, 2016
This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: #5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871 PR-URL: #7303 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Jul 1, 2016
This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: #5900 PR-URL: #7303 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Jul 12, 2016
This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: #5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871 PR-URL: #7303 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins
pushed a commit
that referenced
this pull request
Jul 12, 2016
This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: #5900 PR-URL: #7303 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
Merged
richardlau
added a commit
to ibmruntimes/node
that referenced
this pull request
Jul 14, 2016
Original commit message: deps: backport e7cc609 from upstream V8 This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: nodejs/node#5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871 PR-URL: nodejs/node#7303 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
richardlau
added a commit
to ibmruntimes/node
that referenced
this pull request
Jul 14, 2016
Original commit message: deps: fix segfault during gc This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: nodejs/node#5900 PR-URL: nodejs/node#7303 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Affected core subsystem(s)
deps: v8
Description of change
These two patches comprise a fix for #5900 – a segfault during GC that can happen under rare circumstances. The interesting part is that the one of the fixes never landed upstream in V8 because the bug doesn't exist in any active V8 branches.
The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3).
This second patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications.
This also needs to be fixed in v5.x; but I'm not sure how much runway is left on that branch and whether it would give us sufficient feedback. Regardless, I think an independent determination can be made whether this is worth fixing on v4.x in the first place (given than the scenario is rare).
R=@nodejs/lts
/cc @nodejs/v8