Fix segfault during GC #5900 (4.x) #7303

ofrobots · 2016-06-15T06:12:36Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
the commit message follows commit guidelines

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

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

targos · 2016-06-15T06:31:50Z

The patch LGTM.

bnoordhuis · 2016-06-15T11:39:43Z

LGTM. Do the V8 and node.js test suite pass with this change?

ofrobots · 2016-06-15T15:37:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/3003/
V8 tests: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/135/

indutny · 2016-06-15T16:33:10Z

@ofrobots do we have understand of what particular call site is causing segfault? Rubber-stamp LGTM

ofrobots · 2016-06-15T17:55:36Z

@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

ofrobots · 2016-06-15T17:58:57Z

And there was a secondary bug in --verify-heap.

ofrobots · 2016-06-15T18:02:58Z

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.

ofrobots · 2016-06-16T06:18:14Z

V8 tests pass for me when run manually on a mac.

MylesBorins · 2016-06-30T23:33:27Z

running v8 tests in CI

https://ci.nodejs.org/job/node-test-commit-v8-linux/171/

Will land if green

MylesBorins · 2016-07-01T00:04:50Z

CI is green LGTM

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>

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 · 2016-07-01T00:05:31Z

landed in 5ba807a...e319d76

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>

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>

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>

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>

ofrobots added 2 commits June 14, 2016 22:39

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 15, 2016

mscdex added the v4.x label Jun 15, 2016

ofrobots mentioned this pull request Jun 15, 2016

Segmentation fault in node v5.* #5900

Closed

MylesBorins force-pushed the v4.x-staging branch 3 times, most recently from ed3d372 to f14d9cf Compare June 28, 2016 22:49

MylesBorins closed this Jul 1, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix segfault during GC #5900 (4.x) #7303

Fix segfault during GC #5900 (4.x) #7303

ofrobots commented Jun 15, 2016

targos commented Jun 15, 2016

bnoordhuis commented Jun 15, 2016

ofrobots commented Jun 15, 2016

indutny commented Jun 15, 2016

ofrobots commented Jun 15, 2016

ofrobots commented Jun 15, 2016

ofrobots commented Jun 15, 2016

ofrobots commented Jun 16, 2016

MylesBorins commented Jun 30, 2016 •

edited

Loading

MylesBorins commented Jul 1, 2016

MylesBorins commented Jul 1, 2016

Fix segfault during GC #5900 (4.x) #7303

Fix segfault during GC #5900 (4.x) #7303

Conversation

ofrobots commented Jun 15, 2016

Checklist

Affected core subsystem(s)

Description of change

targos commented Jun 15, 2016

bnoordhuis commented Jun 15, 2016

ofrobots commented Jun 15, 2016

indutny commented Jun 15, 2016

ofrobots commented Jun 15, 2016

ofrobots commented Jun 15, 2016

ofrobots commented Jun 15, 2016

ofrobots commented Jun 16, 2016

MylesBorins commented Jun 30, 2016 • edited Loading

MylesBorins commented Jul 1, 2016

MylesBorins commented Jul 1, 2016

MylesBorins commented Jun 30, 2016 •

edited

Loading