Repository navigation
Diagnostic code for fixed bug still present in v4.x's V8 #7574
Description
- Version: v4.0.0 and later in the v4.x release line.
- Platform: all 64bits platforms except AIX.
- Subsystem: V8.
Lately, I came across several occurrences of crashes that look like duplicates of #3898. Here's the callstack of one of these occurrences as displayed by mdb_v8's ::jsstack command:
> ::jsstack
native: v8::base::OS::Abort+9
native: v8::internal::PointersUpdatingVisitor::VisitPointers+0x90
native: v8::internal::HeapObject::IterateBody+0x569
native: v8::internal::MarkCompactCollector::EvacuateNewSpaceAndCandidate...
native: v8::internal::MarkCompactCollector::SweepSpaces+0x141
native: v8::internal::MarkCompactCollector::CollectGarbage+0x48
native: v8::internal::Heap::MarkCompact+0x60
native: v8::internal::Heap::PerformGarbageCollection+0x4c0
native: v8::internal::Heap::CollectGarbage+0x238
native: v8::internal::Heap::HandleGCRequest+0x8f
native: v8::internal::StackGuard::HandleInterrupts+0x31c
native: v8::internal::Runtime_StackGuard+0x2b
(1 internal frame elided)
js: <anonymous> (as ScatterGather.scatter)
(1 internal frame elided)
(1 internal frame elided)
(1 internal frame elided)
js: InnerArrayForEach
js: forEach
(1 internal frame elided)
js: <anonymous> (as MessageIndexer.get_modified_threads)
(1 internal frame elided)
(1 internal frame elided)
(1 internal frame elided)
(1 internal frame elided)
js: <anonymous> (as RiakCacheClient.write_cache)
(1 internal frame elided)
js: <anonymous> (as RiakRequest.callback)
js: <anonymous> (as RiakRequest.on_response)
(1 internal frame elided)
(1 internal frame elided)
js: <anonymous> (as PoolRequestSet.handle_response)
(1 internal frame elided)
js: <anonymous> (as PoolEndpoint.request_succeeded)
js: <anonymous> (as PoolEndpointRequest.on_end)
(1 internal frame elided)
js: emit
js: endReadableNT
js: nextTickCallbackWith2Args
js: _tickDomainCallback
(1 internal frame elided)
(1 internal frame elided)
native: v8::internal::Execution::Call+0x107
native: v8::Function::Call+0xff
native: v8::Function::Call+0x41
native: node::AsyncWrap::MakeCallback+0x22c
native: node::StreamBase::EmitData+0xdd
native: node::StreamWrap::OnReadImpl+0x14a
native: node::StreamWrap::OnRead+0x7c
native: uv__read+0x2ef
native: uv__stream_io+0x2a8
native: uv__io_poll+0x22a
native: uv_run+0x15e
native: node::Start+0x558
native: _start+0x6c
Unfortunately, I can't reproduce that crash and I can't share the core files that were provided to me.
My understanding was that, as mentioned in #3898, that crash was triggered by some diagnostic code that was added in order to investigate another V8 issue.
What surprised me is that it seems that the fix for the original issue was integrated in the v4.x branch as part of the upgrade to V8 4.5.103.24 with c431725 but the diagnostic code is still there.
The commit that upgrade V8 to version 4.5.103.24 in node v4.x doesn't show the fix for the original issue (v8/v8@8606664) in the GitHub web UI, but using git show c43172578e3e10c2de84fc2ce0d6a7fb02a440d8 in a local checkout of the v4.x-staging branch shows that the fix was indeed integrated into the v4.x and v4.x-staging branches.
Another thing that surprised me is that c431725 represents the upgrade of V8 to version 4.5.103.24, but the fix for the original issue (v8/v8@8606664) doesn't seem to be part of V8's 4.5.103.24 release:
➜ v8 git:(master) ✗ git tag --contains 8606664b37f4dc4b42106563984c19e4f72d9d3a | grep 4.5.103.24
➜ v8 git:(master) ✗
It's very likely that I'm missing something about how release branches/tags are organized in V8's repository though.
Now that raises a few questions:
- The current situation seems to be that node v4.x branches have the fix for a bug but that the diagnostic code for that bug, which makes some node programs crash, is still present. Is that on purpose, or is it an inconsistency that fell through the cracks?
- If the fix for that bug is present, how come the diagnostic code still triggers crashes? If I understand correctly, the diagnostic code aborts if a JS object has an address whose value may have been overwritten by a double. "Whether the address of an object may have been overwritten by a double" is determined by checking if its address is
> 1 << 48. Is it possible for a valid JS object to have an address that is >1 << 48for a 64bits process? If so, does that mean that the diagnostic code was overly conservative and triggered false positives? Basically, I'm trying to determine whether it makes sense to remove that diagnostic code by back porting https://codereview.chromium.org/1420253002 to v4.x-staging now that the original bug has been fixed.
FWIW, I tested a back port of https://codereview.chromium.org/1420253002 to v4.x-staging with make check and make test-v8 and it doesn't trigger any regression.
/cc @nodejs/v8.
Activity
misterdjules commentedon Jul 7, 2016
Indeed, I was missing something. The fix for the original issue was integrated into V8's 4.5.103.24 release with v8/v8@ef23c3a. I'm still wondering why the diagnostic code is still there.
MylesBorins commentedon Jul 7, 2016
/cc @ofrobots @nodejs/v8 @nodejs/lts
should we be manually backporting misterdjules@0ee8690 to v4.x-staging?
bnoordhuis commentedon Jul 7, 2016
Seems fine to me. It should bump V8_PATCH_LEVEL in deps/v8/include/v8-version.h though.
ofrobots commentedon Jul 7, 2016
For posterity, to check whether a particular V8 commit has been cherry-picked back to release lines, you can use
tools/release/mergeinfo.pyin a V8 checkoutIt seems that the fix was merged into V8 4.5 as part of the above cherry-pick. However, the diagnostic code wasn't removed until October 23, by which time V8 4.5 was already a dead branch:
misterdjules commentedon Jul 7, 2016
@ofrobots Thank you for the info on
tools/release/mergeinfo.py.Do you have any opinion on question #2 in my comment above:
?
ofrobots commentedon Jul 7, 2016
The diagnostic code is using unsigned arithmetic, so yes, on x86-64, it is possible for an address to be larger than 1 << 48 unsigned (although I am not sure whether linux grants those mappings to user space. No idea about other OSes). x86-64 uses 48-bit sign extended virtual address space.
@misterdjules: What OS and architecture are your core dumps from? If x86-64, I would expect
heap_obj->address()to have 2 most significant bytes to be0xffff.misterdjules commentedon Jul 7, 2016
@ofrobots
Right, my question was more specific to V8's JS heap management than about addresses in any process' native heap. If valid JS objects can have addresses
> 1 << 48, then I fail to see how the diagnostic code, which makes V8 abort when such an object is found, makes sense. So I must be missing something.x86-64, the diagnostic code is only enabled on that target architecture.
Why?
ofrobots commentedon Jul 7, 2016
If the diagnostic code is asserting, then you have an address larger than 1 << 48. Legal addresses larger than 1 << 48 would have the top 16 bits set. I would be a concerned if that wasn't the case. What is the actual value you see?
misterdjules commentedon Jul 7, 2016
@ofrobots Can you quote the parts of the comments you're responding to, otherwise it's difficult to know which part you're responding to, and the discussion becomes confusing to me.
Is it in response to:
?
If so, I thought that by "I would expect heap_obj->address() to have 2 most significant bytes to be 0xffff", you meant that JS heap objects whose address have their 2 most significant bytes to be 0xffff are valid. Which seems contradictory to the diagnostic code that crashes when such an object is found.
ofrobots commentedon Jul 7, 2016
Yes, I agree with this. I am trying get a better understanding of the situation that you are describing from your core dumps. That is why I was asking about the actual value of the address (or just the top 2 bytes, if you are paranoid) you see on which you hit the assert. Here are possible scenarios:
0x0000: Not possible, as you are hitting the assert.0xffff: Surprising, but possible. These would be legal addresses if the OS (which you didn't specify) is granting them to user space. The diagnostic code is incorrect in this case. Also note that the diagnostic code conflates "OS" and "architecture".misterdjules commentedon Jul 7, 2016
This is a bit tricky due to the fact that a lot of this diagnostic code and its callers are inlined.
These cores were generated on SmartOS. It turns out that on SmartOS (and probably most OSes derived from Solaris) mmapped pages can be mapped at addresses with the two most significant bytes set. That was what I was missing. The output of
pmapon one of these core files illustrates this:In the output above, we can see that some
[anon]mappings (mmapped pages, used for allocating space on the JS heap) are mapped at virtual addresses> 2**48, like this instance:So it seems the diagnostic code was probably triggering false positives on SmartOS (and probably most OSes derived from Solaris). Thus, I would think it is safe to remove it, since these crashes in that diagnostic would not indicate that the original bug wasn't fixed, but instead that the diagnostic code was not properly working on Solaris/SmartOS.
@ofrobots Does that sound reasonable to you?
ofrobots commentedon Jul 7, 2016
Thanks for posting the pmap output; based on that I agree that the dumps look like false positives caused by imperfect diagnostic code.
misterdjules commentedon Sep 15, 2016
Closing this issue as I believe this was fixed by #7584 and it landed in v4.x with 4107b5d.