-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: test-tick-processor failed #5903
Comments
I wonder why this is not caught in CI. |
The test already has open issues for it being flaky on two other platforms. It is a non-deterministic test that will vary a lot based on how powerful your computer is, so that's probably what's going on here. |
Getting the same here on OSX 10.11.4 (15E65)
|
There is also #4427 for the same test file - not sure if it is related. |
/cc @nodejs/v8 |
I got this too |
I believe this is a culprit:
Looking... |
Fix: diff --git a/deps/v8/tools/tickprocessor.js b/deps/v8/tools/tickprocessor.js
index 600d2ee..ccdb11e 100644
--- a/deps/v8/tools/tickprocessor.js
+++ b/deps/v8/tools/tickprocessor.js
@@ -757,8 +757,10 @@ inherits(MacCppEntriesProvider, UnixCppEntriesProvider);
MacCppEntriesProvider.prototype.loadSymbols = function(libName) {
this.parsePos = 0;
libName = this.targetRootFS + libName;
+
+ // It seems that with OS X 10.11.4, `nm` thinks that `-f` is a format option
try {
- this.symbols = [os.system(this.nmExec, ['-n', '-f', libName], -1, -1), ''];
+ this.symbols = [os.system(this.nmExec, ['-n', libName], -1, -1), ''];
} catch (e) {
// If the library cannot be found on this system let's not panic.
this.symbols = ''; Going to submit it to V8 team. |
@indutny The test still passes on my end with your fix applied. |
@matthewloring @indutny on my end too. Issue can be closed. |
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{nodejs#35445} Fix: nodejs#5903
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{#35445} Fix: #5903 PR-URL: #6179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{#35445} Fix: #5903 PR-URL: #6179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{#35445} Fix: #5903 PR-URL: #6179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{#35445} Fix: #5903 PR-URL: #6179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
It looks like it's failing again: https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1010/3097/
/cc @indutny |
Hmm.. quite odd. I'm not able to reproduce this on OS X 10.11. I run these tests frequently and haven't seen this lately :-/ ... @indutny any ideas what could be going on? |
@jasnell idk yet, looks like the symbol is gone again:
|
Can confirm I'm also experiencing this. OS X 10.10.5 |
hmm.. just checked to make sure I'm not completely crazy... the OSX CI run for the v6.x release didn't have this: https://ci.nodejs.org/job/node-test-commit-osx/3092/ ... and I haven't seen it locally yet... and yet it showed up in the next CI run from master https://ci.nodejs.org/job/node-test-commit-osx/3093/nodes=osx1010/console. |
So this is what happens:
While
I guess for some reason the tick doesn't get recorded at |
@jasnell It's probably not too surprising that the test sometimes doesn't fail. One of the thorny issues with this test is that it is non-deterministic. It's kind of a "run this a large number of times and sooner or later we should see X" kind of thing. I've never taken the time to fully understand the nature of this particular test, but in previous issue tracker discussions with @matthewloring, it seemed to be indicated that it was very difficult or perhaps even impossible to make the test deterministic. |
I guess one naive question (that may not have an answer yet?): Is it failing because the test is flaky or because there is an issue with profiling (or whatever) on OS X in the current version? |
@Trott I am looking into this issue as well. Looking at the output of a profile and checking for node specific C++ symbols is a good indication of whether or not the test is flaky (if functions with the prefix |
I modified test to run for 20 sec, and it is still failing. I'll figure it out by the end of the day, unless distracted. |
This is surprising to me, but Can someone else compile that commit on OS X and confirm that |
Looks like that 204f3a8 was landed without a CI run, so if it really is the culprit, that explains why CI didn't catch the problem. :-/ |
It looks like the test is failing due to new ASLR features that MACOSX_DEPLOYMENT_TARGET=10.7 enables. If you build with |
@bnoordhuis thank you, this is indeed matches my observations. (And sorry everyone, I was distracted) |
Commit 204f3a8 ("build: Bump MACOSX_DEPLOYMENT_TARGET to 10.7") unwittingly turned on new ASLR features that make `-prof` unusable for profiling C++ code, breaking `test/parallel/test-tick-processor.js` in the process. Build with `-Wl,-no_pie` for now. Fixes: nodejs#5903
#6453 for a proposed (hopefully temporary) workaround. |
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{#35445} Fix: #5903 PR-URL: #6179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Commit 204f3a8 ("build: Bump MACOSX_DEPLOYMENT_TARGET to 10.7") unwittingly turned on new ASLR features that make `-prof` unusable for profiling C++ code, breaking `test/parallel/test-tick-processor.js` in the process. Build with `-Wl,-no_pie` for now. Fixes: #5903 PR-URL: #6453 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{#35445} Fix: #5903 PR-URL: #6179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Original commit message: tools: fix tickprocessor Cpp symbols on mac Despite man page documentation: -f Display the symbol table of a dynamic library flat (as one file not separate modules). `nm` on mac treats `-f` as a shorthand for `-format`. The `-f` argument does not seem to be required, so just remove it completely. (For `-format` documentation - see `nm --help` on mac). BUG= Review URL: https://codereview.chromium.org/1840633002 Cr-Commit-Position: refs/heads/master@{#35445} Fix: #5903 PR-URL: #6179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
When running
make test
, I got the following:Assertion error in this code:
The text was updated successfully, but these errors were encountered: