V8 lateral 3.28 update #9185
V8 lateral 3.28 update #9185
Conversation
trevnorris
force-pushed
the
v8-lateral-3.28-update
branch
from
2aceeac
to
4e53b32
Compare
The 3.28.73 update was technically unstable code. This reverts the code to the latest 3.28 stable release.
Reviewed-By: Fedor Indutny <[email protected]> PR-URL: #8476
Reviewed-By: Fedor Indutny <[email protected]> PR-URL: #8476
trevnorris
force-pushed
the
v8-lateral-3.28-update
branch
from
4e53b32
to
1314cfe
Compare
|
@richardlau Thanks. Taken care of. |
| @@ -150,7 +146,7 @@ | |||
| 'Map, instance_size, int, kInstanceSizeOffset', | |||
| 'Map, bit_field, char, kBitFieldOffset', | |||
| 'Map, bit_field2, char, kBitField2Offset', | |||
| 'Map, bit_field3, int, kBitField3Offset', | |||
| 'Map, bit_field3, SMI, kBitField3Offset', | |||
There was a problem hiding this comment.
4312f8d changed bit_field3 to be an int instead of an SMI, so that mdb_v8 can actually inspect its value and determine the correct type depending on which V8 version is used. We'll need to keep this field as an int to keep mdb_v8 working.
Bugs fixed: * v0.12 and later: in-object properties not printing correctly. * 64-bit: not printing external strings correctly (offset was hardcoded for 32-bit). This would happen with "::jsstack -vn0" because the script "node.js" wasn't printed correctly, at least with 0.10 core files. * 64-bit: printing JS source (via "::jsstack -v") emits errors and shows the wrong code. * Several build warnings. * Two-byte strings are unnecessarily truncated. * Could print friendlier note when given obviously bogus function token positions. New features: * ::jsstack prints much cleaner output by default. * ::jsprint keys are now quoted. * ::jsstack -v includes "this" value for each function on the stack. * ::jsstack -v includes more details about each argument (constructor names for each object). * new commands: ::jsconstructor, ::jsfunctions, ::jssource, ::nodebuffer and ::v8internal. * ::findjsobjects and ::jsprint hidden flags for developers to measure and improve test coverage. * internal jsobj_properties() function is much better documented. Reviewed-By: Timothy J Fontaine <[email protected]> Conflicts: deps/mdb_v8/mdb_v8.c test/pummel/test-postmortem-details.js
|
It would be interesting to know if the following issues would be fixed by this upgrade: |
|
#9113 unfortunately is not fixed by this. Still investigating the others. |
|
This also doesn't help #9084. Don't have the energy today, but may do a bisect of V8 versions until I can find the one that seems to have broken so many things. |
|
Bisected one of the issues down: #9084 (comment) |
|
@trevnorris Just FYI, I asked @piranna to test this PR with NodeOS. It will be interesting to know if it fixes this use case. |
| @@ -35,7 +35,6 @@ namespace std { | |||
| int signbit(double x); | |||
| } | |||
| # endif | |||
| #include <alloca.h> | |||
There was a problem hiding this comment.
This had been added by floating a patch that fixes build issues on SmartOS. In the meantime, I believe most of the changes from this patch except the inclusion of the alloca.h header on Solaris derivatives have been upstreamed.
Floating this patch again would solve this build issue on SmartOS: http://jenkins.nodejs.org/job/node-test-commit-unix/DESTCPU=ia32,label=smartos/13/console.
We should however probably upstream it too.
|
@trevnorris It seems we're also missing another floating patch. |
|
@trevnorris There's also a build error on one of the OS X Jenkins agent. I tried to reproduce it on my local OS X setup without success. |
|
Ok, I've checked NodeOS 32 bits with the commit ac21a53 and it's not working, so seems it's not fixed, sorry... :-( |
|
Pasting my comment in #9097 (comment) here to provide some context: The commit you would want to test though is d6249d0, and it requires the other commits here: https://github.com/joyent/node/pull/9185/commits. ac21a53 in itself wouldn't solve the issue with NodeOS, as it only brings bug fixes and new features to Do you have some time to try these commits? |
|
According to |
|
Yes, that's exactly what I did :-) git cherry-pick is a somewhat advanced command compared to git checkout, so it would be a little bit insane to test it this way, isn't it? ;-) |
|
@trevnorris I've never used what is provided by handle zapping before. If I understand correctly, that helps when investigating crashes/bugs because handles removed from the handles scope have a very specific value that is easily recognizable. I don't know what the impact on performance for removing that is (my understanding is that it could improve performance), and I don't know how useful it is for release builds. @joyent/node-coreteam @indutny @bnoordhuis What do you think? |
| @@ -1,82 +0,0 @@ | |||
| *.a | |||
There was a problem hiding this comment.
Do you know why has the .gitignore file been removed by this downgrade?
There was a problem hiding this comment.
because cp doesn't copy .* from the dir. it can be reverted if it's an issue.
There was a problem hiding this comment.
I think the less changes we have from upstream the better, even though in this case it has no impact on functionality, so I would say that we'd like to put it back in.
|
@trevnorris Finally had some time to do a full review. Aside from the comments I added, I'm not familiar enough with V8's implementation to have a very valid opinion, but if that's what's the V8 team recommends, what are the next steps to land this? |
|
It's been so long I can't remember if it was the V8 team specifically that recommended this be done. It was more because the current version of V8 in Node isn't an actual stable release. This will have to be rebased, and the changes you commented on will have to be made. |
|
@trevnorris Someone from the V8 team recommended this version here: #9241. @trevnorris Do you have the time to do what needs to be done to land this (rebasing + floating some of the missing patches again)? |
|
@misterdjules sorry, i'm on site at Intuit for the week. I could work on this next Wednesday. |
You would normally turn off handle zapping in release builds and turn it on in debug builds or nightlies. By the way, you should do this upgrade sooner rather than later. 3.28.73 has a couple of memory corruption bugs that may be remotely exploitable and I think it also has that security bug where There are probably more security patches that you have to back-port from newer releases, what with 3.28 not being maintained. I'm fairly sure there were fixes in 3.29, 3.30 and 3.31 that haven't been applied to 3.28. |
|
@bnoordhuis Thank you for the feedback! Is there a resource somewhere where some information about V8's security issues/bugs can be found? I looked at v8-users and v8-dev but didn't find anything. /cc @jakobkummerow. |
|
I don't believe there is anything like a v8-security mailing list. I just track V8 development and keep an eye out for patches that look like security fixes. |
|
Right. If you use the tip of a supported branch, you don't have to worry about anything. If you insist on using an outdated, unsupported branch, you can watch what we merge back to still-supported branches. If it fixes code that's already present in your branch, you'll want the patch too. Re handle zapping, as @bnoordhuis said, it flushes out bugs but has a performance penalty. It should be turned off in production. It can be helpful for developing changes either within V8 or in the embedder -- basically anything that uses HandleScopes. |
|
@bnoordhuis Thanks! @jakobkummerow Thank you for your feedback too! Regarding V8's branches and tags: I use this application to determine what the current V8 stable is: https://omahaproxy.appspot.com. Today, it seems that the current stable V8 is In my clone of V8's github mirror repository, I can see the corresponding tag, and it seems that it corresponds to this commit: v8/v8@d7f25f5. However this commit doesn't seem to be on the 4.2.77 branch, but on the master branch. What branch(es) should I watch to know about changes that land in stable versions? |
|
@trevnorris The commit message for ac21a53 seems to include too many things. Something like There's an issue to upstream part of that change here: #9147. I also wonder why we have two different symbols |
|
@misterdjules Close :-) 4.2.77.14 is the version of V8 shipped in the latest stable Chrome (and a reasonable approximation of what you want to ship). The actual latest V8 version on the 4.2 branch is at https://chromium.googlesource.com/v8/v8.git/+/branch-heads/4.2. As of this writing the branch's tip is 4.2.77.15, one back-merge ahead of what Chrome ships -- the next Chrome update will pick it up. This is the authoritative source of the branch that you'll want to watch. Note how the tag you found on github says "Merged d7f25f5" -- so yeah, d7f25f5 is indeed a master commit that was merged to the 4.2 branch. The merge commit is e65bd3efa19fb96186900ea88809a78347bf23aa (you can find this hash a little further down on the page). |
Then maybe we should follow the updates and use the latest stable release, isn't it? Maybe a good aproach would be to use the same major version on the current branch, so since v0.12 ships a 3.x v8 we should use the latest 3.x v8 stable release but the 4.x on Node.js v0.13, isn't it? |
|
@piranna Yes you should follow the updates and use the latest stable release, which right now is 4.2.77.15. You can see on https://chromium.googlesource.com/v8/v8.git/+/branch-heads/4.2 when the tip of the 4.2 branch updates, and you can see on omahaproxy.appspot.com when you should switch from 4.2 to 4.3. (I wonder how many more times I'll have to repeat that.) V8 branch naming works somewhat similar to Linux kernel versions:
|
|
I was not asking to do it myself, I don't have enough Node.js internals to do so, but instead about the path Node.js dev should follow. According to your message, they should release the latest and greatest release of v8, similar to what seems io.js is currently doing, isn't it? 👍 to this. |
|
@trevnorris @joyent/node-coreteam Created #18206 to continue this work. |
|
Do you suggest we test against that brach to see if it works for us (for
|
|
@trevnorris Closing this one in favor of #18206 to avoid confusion, but of course let me know if we should keep it open. Thanks! |
|
@jakobkummerow Thank you very much for the clarification on the branching model, I will use https://chromium.googlesource.com/v8/v8.git/ from now on, since it seems to have all the information I need contrary to the GitHub mirror. Also, to clarify why the current V8 version in Node.js is outdated: I don't think anyone "insist[s] on using an outdated, unsupported branch". We would use the latest and greatest if we could and are very much aware that the differences between V8's release cycle and Node.js' release cycle make this difficult. We are working on solving that problem. However, this is not the only problem we need to solve. It takes time and our time is limited. I hope you'll be happy to hear that you won't have to repeat yourself :) Thank you! |
|
You are welcome :-) |
3.28.73 was actually from an unstable branch of V8. This reverts those changes to the actual latest stable 3.28. The reason for this is because of an edge case IC Miss that is causing a specific case to abort (see #9181 (comment))
I'm not sure if we want to re-enable handle zapping or not.
R=@tjfontaine @misterdjules