-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Segfault in v8.2.1 #14567
Comments
Updated to fix formatting of details block Just got another more interesting version: core_dump.node.20212.myitcv-virtual-machine.1501589446
|
/cc @nodejs/v8 @hashseed |
Reproduced using
core_dump.node.23896.myitcv-virtual-machine.1501590439
|
@myitcv could you try and capture a stacktrace with lldb ( |
If it reproduces with v6.11.1, then at least the string rehashing patch cannot be responsible for it. |
@myitcv What happens when you remove |
@refack with a new core dump: https://gist.github.com/myitcv/4fbcb7b34ea44f645b81ba4c3c139e78 You will notice that there is a fair amount of recursion going on here. This is expected given the test it is running. |
@bnoordhuis thanks, I didn't set that myself... going with the status quo. If I remove
So I suspect that flag was put in there intentionally... will check. |
@myitcv Try setting |
@bnoordhuis 👍 - that seems to have sorted things. Prior to your suggestion my With the two equal I now cannot repro this in I'm tempted to close this now unless there's some merit in keeping it open from your perspective? Thanks again |
I'll close for now, but please feel free to re-open if as I say there's a reason you would like to keep it open. |
As outlined in gopherjs#661, the compiler fails to generate code to copy a struct/array value for an assignment when the target's underlying type is an interface type, whether for explicit variable assignments, implicit function/method parameters etc. Instead, taking the example of explicit variable assignment, the interface variable is assigned a value that contains the same pointer to the source struct/array val (we're in Javascript world, so everything is a pointer). This means that changes to the struct/array value via the source variable are, incorrectly, visible via the target variable. gopherjs#661 gives a simple example. There is a further issue when interface values are assigned to interface-typed variables: struct/array values are not copied when they should be. For some reason the changes that address the aforementioned issues start to tip the std library tests over the V8 stack size limit, causing text/template TestMaxExecDepth to fail randomly locally and on CI. This had previously been addressed by @neelance in 1f89545; the --stack_size flag was passed to NodeJS which in turn passed the value onto V8. But per nodejs/node#14567 (comment) it was pointed out that the value of ulimit -s must be >= the value of --stack_size for the --stack_size to make any sort of sense. Hence this commit also harmonises the setting of ulimit -s during the CI test run with the value of --stack_size that is passed to NodeJS (which in turn passes that value onto V8) when running either gopherjs test or gopherjs run. Fixes gopherjs#661.
Picking up on @shurcooL's comment in gopherjs#669 (comment). We've started randomly seeing std library tests tip over the V8 stack size limit, causing text/template TestMaxExecDepth to fail randomly locally and on CI. This had previously been addressed by @neelance in 1f89545; the --stack_size flag was passed to NodeJS which in turn passed the value onto V8. But per nodejs/node#14567 (comment) it was pointed out that the value of ulimit -s must be >= the value of --stack_size for the --stack_size to make any sort of sense. Hence this commit also harmonises the setting of ulimit -s during the CI test run with the value of --stack_size that is passed to NodeJS (which in turn passes that value onto V8) when running either gopherjs test or gopherjs run.
Picking up on @shurcooL's comment in gopherjs#669 (comment). We've started randomly seeing std library tests tip over the V8 stack size limit, causing text/template TestMaxExecDepth to fail randomly locally and on CI. This had previously been addressed by @neelance in 1f89545; the --stack_size flag was passed to NodeJS which in turn passed the value onto V8. But per nodejs/node#14567 (comment) it was pointed out that the value of ulimit -s must be >= the value of --stack_size for the --stack_size to make any sort of sense. Hence this commit also harmonises the setting of ulimit -s during the CI test run with the value of --stack_size that is passed to NodeJS (which in turn passes that value onto V8) when running either gopherjs test or gopherjs run.
The goal of this change is to fix intermittent test failures we've started seeing recently: === RUN TestMaxExecDepth FAIL text/template 2.272s In 1f89545, the --stack_size flag was passed to Node.js. In nodejs/node#14567, it was pointed out that the value of ulimit -s must be >= the value of --stack_size. This change makes that so by setting --stack_size based on the current value of ulimit, and sets ulimit to 10000 in CI.
Unclear whether this is a dup of #14228 so reporting separately for now.
The segfault occurs randomly when running the test suite for GopherJS (I can give more details if that is relevant)
Following the example of #14228 here is the
gdb
information I have from one of the core dumps (I'm absolutely nogdb
expert so please shout where I'm going wrong):core_dump.node.14386.myitcv-virtual-machine.1501588338
Very happy to provide further information.
The text was updated successfully, but these errors were encountered: