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: detect all types of aborts in windows #12856

Closed

Conversation

@gireeshpunathil
Copy link
Member

commented May 5, 2017

On Windows, 'aborts' are of 2 types, depending on the context:
(i) Forced access violation, if --abort-on-uncaught-exception is on
which corresponds to exit code 3221225477 (0xC0000005)
(ii) raise(SIGABRT) or abort(), which lands up in CRT library calls
which corresponds to exit code 3

For background, please refer to #12823

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@addaleax

This comment has been minimized.

Copy link
Member

commented May 5, 2017

It looks like this already needs a rebase 😄

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

@addaleax - I don't know what would have caused this (branch conflict), and how to resolve it. Any pointers please?

@addaleax

This comment has been minimized.

Copy link
Member

commented May 5, 2017

git fetch upstream (or whatever your remote that points to nodejs/node is called) and then git rebase upstream/master, fix whatever conflicts come up … let me know if that’s unclear, git is always tricky :)

test: detect all types of aborts in windows
On Windows, 'aborts' are of 2 types, depending on the context:
(i) Forced access violation, if --abort-on-uncaught-exception is on
which corresponds to exit code 3221225477 (0xC0000005)
(ii) raise(SIGABRT) or abort(), which lands up in CRT library calls
which corresponds to exit code 3

@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:windowsabortcodes branch to bd0e41a May 5, 2017

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

@addaleax - thanks! I guess I did it right.

@cjihrig
cjihrig approved these changes May 5, 2017
@gireeshpunathil gireeshpunathil referenced this pull request May 5, 2017
2 of 2 tasks complete
@refack

This comment has been minimized.

Copy link
Member

commented May 8, 2017

@gireeshpunathil could you write a few words about how come tests haven't been failing? And do we need a test that aborts with code 3?

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

@refack -

  if (exports.isWindows) {
     expectedExitCodes = [3221225477];
+    console.log('XXX');
+    console.trace();
  }

With this change I ran the entire unit tests (python tools\test.py --mode=release) and it hit only once - the new test case I added (parallel/test-http-parser-consume).

Looks like we never have a test case which aborts in Windows either through JS process.abort(), or through C++ Assert(FALSE), or through --abort-on-uncaught-exception

If you propose I can add one.

@refack

This comment has been minimized.

Copy link
Member

commented May 8, 2017

@gireeshpunathil thank you. IMHO adding a test would be great.

@sreepurnajasti sreepurnajasti referenced this pull request May 9, 2017
3 of 3 tasks complete
@gibfahn
gibfahn approved these changes May 9, 2017
@jasnell
jasnell approved these changes May 9, 2017
@jasnell

This comment has been minimized.

@addaleax addaleax added the windows label May 10, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented May 10, 2017

Lande in 6914aea

@addaleax addaleax closed this May 10, 2017

addaleax added a commit that referenced this pull request May 10, 2017
test: detect all types of aborts in windows
On Windows, 'aborts' are of 2 types, depending on the context:
(i) Forced access violation, if --abort-on-uncaught-exception is on
which corresponds to exit code 3221225477 (0xC0000005)
(ii) raise(SIGABRT) or abort(), which lands up in CRT library calls
which corresponds to exit code 3

PR-URL: #12856
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

thanks all!

@refack

This comment has been minimized.

Copy link
Member

commented May 11, 2017

thanks all!

No, thank you 🥇

@gibfahn

This comment has been minimized.

Copy link
Member

commented May 12, 2017

There seems to be a 6.10 branch in node now, which has 6914aea as the latest commit. Is this an accident (I guess maybe @addaleax, idk)?

@arturgvieira

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

I was having Git tech difficulties the other day. It was after my first PR got merged I believe.

@addaleax

This comment has been minimized.

Copy link
Member

commented May 12, 2017

@gibfahn That wasn’t me, I just saw the ref in a git fetch upstream as a new branch myself. But anyway, I’ve deleted it now, it shouldn’t be there (maybe somebody who keeps IRC open knows why this happened?)

how could I have made a mistake without access rights I don't know

@arturgvieira You couldn’t, this is definitely not something you have caused. :) It’s not a big deal anyway.

@arturgvieira

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

@addaleax I see, thanks, I feel much better

@refack

This comment has been minimized.

Copy link
Member

commented May 12, 2017

two days ago (EDT time)
[19:15] -nodejs-gh:#node-dev- [node] eugeneo created 6.10 from master (+0 new commits): https://git.io/v9MrZ

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: detect all types of aborts in windows
On Windows, 'aborts' are of 2 types, depending on the context:
(i) Forced access violation, if --abort-on-uncaught-exception is on
which corresponds to exit code 3221225477 (0xC0000005)
(ii) raise(SIGABRT) or abort(), which lands up in CRT library calls
which corresponds to exit code 3

PR-URL: nodejs#12856
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
gibfahn added a commit that referenced this pull request Jun 20, 2017
test: detect all types of aborts in windows
On Windows, 'aborts' are of 2 types, depending on the context:
(i) Forced access violation, if --abort-on-uncaught-exception is on
which corresponds to exit code 3221225477 (0xC0000005)
(ii) raise(SIGABRT) or abort(), which lands up in CRT library calls
which corresponds to exit code 3

PR-URL: #12856
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: detect all types of aborts in windows
On Windows, 'aborts' are of 2 types, depending on the context:
(i) Forced access violation, if --abort-on-uncaught-exception is on
which corresponds to exit code 3221225477 (0xC0000005)
(ii) raise(SIGABRT) or abort(), which lands up in CRT library calls
which corresponds to exit code 3

PR-URL: #12856
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.