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

test-debug-args.js failing on pLinux #3390

Closed
mhdawson opened this issue Oct 15, 2015 · 8 comments
Closed

test-debug-args.js failing on pLinux #3390

mhdawson opened this issue Oct 15, 2015 · 8 comments
Labels
test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.

Comments

@mhdawson
Copy link
Member

Seeing a failure PPC after this change was merged https://ci.nodejs.org/job/node-test-commit-plinux/9/. However it looks like it is more likely related to bbdbef9. I'm guessing there was not CI run for that one.

not ok 902 test-debug-args.js
#
#
##
## Fatal error in , line 0
## external code buffer is too small
##
#
#==== C stack trace ===============================
#
#1: V8_Fatal
#2: v8::internal::Assembler::GrowBuffer(int)
#3: v8::internal::Assembler::EmitRelocations()
#4: v8::internal::Assembler::GetCode(v8::internal::CodeDesc*)
#5: v8::internal::Builtins::SetUp(v8::internal::Isolate*, bool)
#6: v8::internal::Isolate::Init(v8::internal::Deserializer*)
#7: v8::Isolate::New(v8::Isolate::CreateParams const&)
#8: node::Start(int, char**)
#9: main
#10: 0x3fff9b0847ac
#11: __libc_start_main
  ---
  duration_ms: 0.39
@mhdawson
Copy link
Member Author

CI run on PR with test change https://ci.nodejs.org/job/node-test-commit-plinux/23/

@mscdex mscdex added the test Issues and PRs related to the tests. label Oct 15, 2015
@ofrobots
Copy link
Contributor

@mhdawson Here's the original CI run for that commit: https://ci.nodejs.org/job/node-test-pull-request/509/. Are the pLinux tests not part of the test-pull-request CI builds as documented here?

@mhdawson mhdawson added the v8 engine Issues and PRs related to the V8 dependency. label Oct 16, 2015
@mhdawson
Copy link
Member Author

I added node-test-commit-plinux to node-test-commit Oct 14, and I can see that it is in later CI runs so it must be because of the timing. What I saw in the runs was build 8 being ok, and build 9 having the regression. However, what obviously happened was that you launched the CI run before I stitched it in, then the actual commit went in between runs 8 and 9. That does point out one nice thing in favor of the test and commit as a single job (which we had to back away from) as it makes it clearer where failures start.

Its looking like this is actually an issue where the buffer size at the V8 level is not big enough for ppc because the code size ends up being bigger. We are putting together a chromium review for the change and will add the link here when available. Once its in google master I'll see if its possible for us to float it in the Node repo until we pull in a version of V8 that includes the fix (likely 4.7)

@ofrobots
Copy link
Contributor

One you have the fix in the V8 master, it would be great if you also submitted a merge request to get upstream to back-port it to 4.6 as per this process. That would avoid having to float a patch.

@mtbrandy
Copy link

V8 fix under review here: https://codereview.chromium.org/1415463002/

@mhdawson
Copy link
Member Author

@ofrobots, pulling over to 4.6 and 4.7 is already in progress. Just planning to float until the full update we take for v8

@ofrobots
Copy link
Contributor

Yep; I think this is already included in my PR for vee-eight-4.7: #3481. I haven't seen the merge for 4.6 yet; is there an issue tracking the merge-request? It would be good to pick up this fix for v5.0.

@ofrobots
Copy link
Contributor

I see that a patch is already floating for master (4.6) in #3474.

mhdawson added a commit that referenced this issue Oct 26, 2015
Pull in the change that has been committed to v8 master in
https://codereview.chromium.org/1415463002/.  We are currently
cherry-picking into 4.6 and 4.7 but until next next v8 update
into Node Master I'd like to float it as it will make PPC LE
go green in the CI

Fixes: #3390
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
PR-URL: #3474
mhdawson added a commit that referenced this issue Oct 29, 2015
Pull in the change that has been committed to v8 master in
https://codereview.chromium.org/1415463002/.  We are currently
cherry-picking into 4.6 and 4.7 but until next next v8 update
into Node Master I'd like to float it as it will make PPC LE
go green in the CI

Fixes: #3390
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
PR-URL: #3474
mhdawson added a commit that referenced this issue Oct 29, 2015
Pull in the change that has been committed to v8 master in
https://codereview.chromium.org/1415463002/.  We are currently
cherry-picking into 4.6 and 4.7 but until next next v8 update
into Node Master I'd like to float it as it will make PPC LE
go green in the CI

Fixes: #3390
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <ben@strongloop.com>
PR-URL: #3474
misterdjules pushed a commit to misterdjules/node-1 that referenced this issue Nov 3, 2015
Backports http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=aab3560b65b9254d17770bb6fe3ca7edd7451429
from openssl upstream, to add support for Visual Studio 2015. This is
already included in the newer openssl 1.0.2.

Original commit message:

  e_os.h: limit _MSC_VER trickery to older compilers.

  PR: nodejs#3390

Original pull request:

  http://rt.openssl.org/Ticket/Display.html?user=guest&pass=guest&id=3390

This was ported to v0.10 in nodejs/node-v0.x-archive#25857

PR-URL: nodejs#2843
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
othiym23 pushed a commit to npm/node that referenced this issue Nov 3, 2015
Backports http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=aab3560b65b9254d17770bb6fe3ca7edd7451429
from openssl upstream, to add support for Visual Studio 2015. This is
already included in the newer openssl 1.0.2.

Original commit message:

  e_os.h: limit _MSC_VER trickery to older compilers.

  PR: nodejs#3390

Original pull request:

  http://rt.openssl.org/Ticket/Display.html?user=guest&pass=guest&id=3390

PR-URL: nodejs/node-v0.x-archive#25857
Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
targos added a commit to targos/node that referenced this issue Nov 11, 2015
This update contains the patch floated in
nodejs#3390 and a fix for
Intl.NumberFormat.

Diff: https://chromium.googlesource.com/v8/v8.git/+/4.6.85.28..4.6.85.31

PR-URL: nodejs#3698
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos added a commit that referenced this issue Nov 11, 2015
This update contains the patch floated in
#3390 and a fix for
Intl.NumberFormat.

Diff: https://chromium.googlesource.com/v8/v8.git/+/4.6.85.28..4.6.85.31

PR-URL: #3698
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
joaocgreis pushed a commit to JaneaSystems/node that referenced this issue Aug 24, 2017
Backports http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=aab3560b65b9254d17770bb6fe3ca7edd7451429
from openssl upstream, to add support for Visual Studio 2015. This is
already included in the newer openssl 1.0.2.

Original commit message:

  e_os.h: limit _MSC_VER trickery to older compilers.

  PR: nodejs#3390

Original pull request:

  http://rt.openssl.org/Ticket/Display.html?user=guest&pass=guest&id=3390
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

4 participants