Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update v8 to 4.4.63.26 #2220

Closed
wants to merge 3 commits into from
Closed

deps: update v8 to 4.4.63.26 #2220

wants to merge 3 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 22, 2015

Includes cherry-picks for:

This is the v8 version of stable Chrome 44.

@targos targos added the v8 engine Issues and PRs related to the V8 dependency. label Jul 22, 2015
@ofrobots
Copy link
Contributor

Rubberstamp LGTM.

@rvagg
Copy link
Member

rvagg commented Jul 23, 2015

I'd love a way to verify that what's in tree is what's upstream, a diff -r perhaps, would that work if I had V8 checked out too? Otherwise I'm hesitant to give an 'lgtm' when I just don't know!

@targos
Copy link
Member Author

targos commented Jul 23, 2015

@rvagg I just tried your suggestion and it works :)

@ofrobots
Copy link
Contributor

@rvagg that's precisely what I verified before my LGTM. This PR seems to be the equivalent to git diff 4.4.63.12 4.4.63.19 with the exception of the patch we are floating for https://crrev.com/f7969b1d5a55e66237221a463daf422ac7611788.

@targos
Copy link
Member Author

targos commented Jul 23, 2015

@@ -32,7 +32,7 @@
# library.

import os, re, sys, string
import optparse
import argparse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Centos CI boxes are failing because of this. I suspect the Centos dists we are using use Python 2.6, while argparse was added in Python 2.7.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @nodejs/build

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous conversation about this here: #2022 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been fixed with https://crrev.com/44bc918458481d60b08d5566f0f31a79e39b85d7 but not backported to 4.4. I will include the patch tomorrow.
Thanks @ofrobots.

@targos
Copy link
Member Author

targos commented Jul 24, 2015

I cherry-picked the argparse fix.
New CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/189/

@rvagg
Copy link
Member

rvagg commented Jul 24, 2015

af162c8 lgtm

@targos
Copy link
Member Author

targos commented Jul 27, 2015

Sorry for the delay, Chrome jumped to 4.4.63.25 so I updated the PR.
https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/198/

@targos targos changed the title deps: update v8 to 4.4.63.19 deps: update v8 to 4.4.63.25 Jul 27, 2015
@bnoordhuis
Copy link
Member

LGTM. The last two regress-crbug tests I missed on the last upgrade, mea culpa.

@targos Maybe you can run the CI one more time? The test-vm-debug-context.js failure on the pi1-raspbian-wheezy is probably a flake but it would be good to have that confirmed.

@targos
Copy link
Member Author

targos commented Jul 27, 2015

@targos
Copy link
Member Author

targos commented Jul 27, 2015

@bnoordhuis test-vm-debug-context.js failed again.
I don't have a Pi1, what can I do to investigate ?

@targos targos added this to the 3.0.0 milestone Jul 28, 2015
@bnoordhuis
Copy link
Member

Maybe someone from @nodejs/build can give you temp access?

@targos
Copy link
Member Author

targos commented Jul 29, 2015

@rvagg gave me access and I could successfully compile but after that I lost control of the machine and I cannot connect anymore.

@ofrobots
Copy link
Contributor

I wonder if it would be possible to reproduce the failure on an QEmu ARM machine. Some instructions here.

@rvagg
Copy link
Member

rvagg commented Jul 30, 2015

it turns out that ssh tunnel from that machine could only handle one connection, after which it fails to properly accept additional connections, I've set up a tunnel using a different machine to do the same thing and it should be fine .. I hope

@targos
Copy link
Member Author

targos commented Jul 30, 2015

I'm connected, it is fine, thank you.

@targos
Copy link
Member Author

targos commented Jul 30, 2015

I've tracked the issue to the following part of the test:

var vm = require('vm');
var assert = require('assert');

// See https://github.com/nodejs/io.js/issues/1190, accessing named interceptors
// and accessors inside a debug event listener should not crash.
(function() {
  var Debug = vm.runInDebugContext('Debug');
  var breaks = 0;

  function ondebugevent(evt, exc) {
    if (evt !== Debug.DebugEvent.Break) return;
    exc.frame(0).evaluate('process.env').properties();  // Named interceptor.
    exc.frame(0).evaluate('process.title').getTruncatedValue();  // Accessor.
    breaks += 1;
  }

  function breakpoint() {
    debugger;
  }

  assert.equal(breaks, 0);
  Debug.setListener(ondebugevent);
  assert.equal(breaks, 0);
  breakpoint();
  assert.equal(breaks, 1);
})();

Crash happens when breakpoint() is called.

The way the process crashes is variable. Here are 3 cases I had:

Segmentation fault
#
# Fatal error in ../deps/v8/src/type-feedback-vector.cc, line 323
# Check failed: feedback == *vector()->UninitializedSentinel(isolate).
#

==== C stack trace ===============================

(empty)
Aborted
module.js:430
  return compiledWrapper.apply(self.exports, args);
                         ^

TypeError: compiledWrapper.apply is not a function
    at Module._compile (module.js:430:26)
    at Object.Module._extensions..js (module.js:448:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:471:10)
    at startup (node.js:117:18)
    at node.js:947:3

@targos
Copy link
Member Author

targos commented Jul 30, 2015

Here is a very minimal test case:

var vm = require('vm');

var Debug = vm.runInDebugContext('Debug');

function breakpoint() {
  debugger;
}

Debug.setListener(function(){console.log('debug event')});

console.log('before break');
breakpoint();
console.log('after break');

Output:

before break
debug event
after break


#
# Fatal error in ../deps/v8/src/type-feedback-vector.cc, line 323
# Check failed: feedback == *vector()->UninitializedSentinel(isolate).
#

==== C stack trace ===============================

(empty)
Aborted

The crash happens outside of my script

@bnoordhuis
Copy link
Member

Long shot: does it work when you pass --novector_ics?

@targos
Copy link
Member Author

targos commented Jul 30, 2015

@bnoordhuis yes it does !

@targos
Copy link
Member Author

targos commented Jul 30, 2015

I'm sorry I don't know what to do now, it is beyond my understanding of v8

@bnoordhuis
Copy link
Member

You could disable it in src/node.cc for now, grep for --nofast_math for another ARM example (which could be removed in next+1 but that aside.)

It might be worthwhile to file a V8 issue for it.

@targos
Copy link
Member Author

targos commented Jul 30, 2015

OK, let's try again with that change

CI: https://jenkins-iojs.nodesource.com/job/node-test-commit/62/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants