We'll start with just supporting this for OS(DARWIN). With this feature, we can now get crash diagnostics like the following: Filtered syslog: Timestamp Thread Type Activity PID 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: BEGIN SIGILL analysis 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x0: ffff0000ffffffff x1: ffff0000ffffffff x2: 0000000000000001 x3: 000000000000009a 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x4: 0000000000000005 x5: 0000000000000060 x6: 0000000000000000 x7: 0000000000000000 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x8: 0000000104a00018 x9: 0000000000000000 x10: 0000000000000001 x11: 0000000000000001 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x12: 0000000000000000 x13: 00000001043ac1b8 x14: 000000016fd0bce0 x15: 000000016fd09708 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x16: 0000000000000000 x17: 0000000104a082e8 x18: 0000000000000000 x19: 0000000000000000 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x20: 0000000000000000 x21: 0000000000000000 x22: 0000000000000000 x23: 0000000000000000 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x24: 0000000000000000 x25: 0000000000000000 x26: 0000000000000000 x27: ffff000000000000 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: x28: ffff000000000002 fp: 000000016fd096c0 lr: 0000000153c1d744 2017-02-01 14:06:20.1410 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: sp: 000000016fd09640 pc: 0000000155c17bec cpsr: 20000000 2017-02-01 14:06:20.1411 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: pc 0x155c17bec is in valid JIT executable memory 2017-02-01 14:06:20.1411 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: instruction bits at pc 0x155c17bec is: 0x00000000 2017-02-01 14:06:20.1411 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: pc 0x155c17bec belongs to CodeBlock 0x104240760 of type DFG 2017-02-01 14:06:20.1411 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: JITCode 0x1051bd000 [0x155c17580-0x155c17f20]: 2017-02-01 14:06:20.1411 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: [0x155c17580-0x155c1759c]: a9bf7bfd 910003fd d280ec10 f2a08490 f2c00030 f80103b0 d10203a1 d2905b11 2017-02-01 14:06:20.1411 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: [0x155c175a0-0x155c175bc]: f2a09411 f2c00031 f87f6a30 eb01021f 540034e8 d10203bf f81f03bb f81f83bc 2017-02-01 14:06:20.1411 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: [0x155c175c0-0x155c175dc]: b2503ffb b27f037c f2400bbf 54000060 52800150 d4200000 b2503ff0 eb10037f ... 2017-02-01 14:06:20.1412 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: [0x155c17f00-0x155c17f1c]: 17801710 00000000 00000000 00000000 00000000 00000000 00000000 00000000 2017-02-01 14:06:20.1412 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: Disassembly: 2017-02-01 14:06:20.1412 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: 0x155c17580: stp fp, lr, [sp, #-16]! 2017-02-01 14:06:20.1412 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: 0x155c17584: mov fp, sp 2017-02-01 14:06:20.1412 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: 0x155c17588: movz x16, #1888 2017-02-01 14:06:20.1412 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: 0x155c1758c: movk x16, #1060, lsl #16 ... 2017-02-01 14:06:20.1455 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: 0x155c17be8: mov x1, x0 2017-02-01 14:06:20.1455 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: * 0x155c17bec: .long 00000000 <========================= 2017-02-01 14:06:20.1455 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: 0x155c17bf0: tst w1, #0x7fffffff ... 2017-02-01 14:06:20.1465 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: 0x155c17f1c: .long 00000000 2017-02-01 14:06:20.1465 -0800 0x29d81 1024 0x0 3711 JavaScriptCore: END SIGILL analysis Patch coming soon.
<rdar://problem/30318237>
Created attachment 300361 [details] proposed patch. Let's try this on the EWSs to make sure it doesn't hurt other platforms.
Attachment 300361 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:61: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:97: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:124: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 300370 [details] proposed patch. Let's try the EWS again.
Attachment 300370 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:61: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:97: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:124: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 300371 [details] proposed patch.
Attachment 300371 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:61: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:97: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:124: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 300373 [details] proposed patch.
Attachment 300373 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:61: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:97: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:124: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 300375 [details] proposed patch: with speculative fix for Windows build.
Attachment 300375 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:61: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:97: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:124: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 300389 [details] proposed patch: with another windows fix.
Attachment 300389 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:64: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:102: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:129: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 3 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 300389 [details] proposed patch: with another windows fix. Ready for a review.
Comment on attachment 300389 [details] proposed patch: with another windows fix. View in context: https://bugs.webkit.org/attachment.cgi?id=300389&action=review > Source/JavaScriptCore/heap/Heap.cpp:2031 > +void Heap::forEachCodeBlockIgnoringJITPlansImpl(const ScopedLambda<bool(CodeBlock*)>& func) > +{ > + return m_codeBlocks->iterate(func); > +} So I guess the idea is that this could crash if we're racing, but we don't care 'cause we were going down anyway? > Source/JavaScriptCore/tools/VMInspector.cpp:63 > +auto VMInspector::lock(unsigned timeoutInSeconds) -> Expected<LockToken, Error> Seconds for timeout > Source/JavaScriptCore/tools/VMInspector.cpp:77 > +#if !OS(WINDOWS) > + (static_cast<unsigned (*)(unsigned)>(sleep))(1); > +#endif I wonder if WTF::sleep(Seconds) works from signal handlers. It probably doesn't, but you could fix that. Basically you just need it to stop using ParkingLot for sleeping, since that grabs locks. Just make it call usleep on POSIX and its equivalent on Windows, I think just Sleep(). > Source/JavaScriptCore/tools/VMInspector.h:49 > + Expected<LockToken, Error> lock(unsigned timeout = std::numeric_limits<unsigned>::max()); should be Seconds timeout = Seconds::infinity()
Comment on attachment 300389 [details] proposed patch: with another windows fix. View in context: https://bugs.webkit.org/attachment.cgi?id=300389&action=review >> Source/JavaScriptCore/heap/Heap.cpp:2031 >> +} > > So I guess the idea is that this could crash if we're racing, but we don't care 'cause we were going down anyway? This is safe to call from VMInspector because: 1. CodeBlocks are added to the CodeBlockSet from the main thread before they are given to the JIT plans. Those codeBlocks will have a null jitCode, but VMInspector checks for that. 2. CodeBlockSet::iterate() will acquire the CodeBlockSet lock before iterating. This ensures that a CodeBlock won't be GCed while we're iterating. 3. VMInspector first does a tryLock on the CodeBlockSet's lock first to ensure that it is safe for the current thread to lock it before calling Heap::forEachCodeBlockIgnoringJITPlans(). Therefore, there's no risk of re-entering the lock. Hence, there is no race, and should not be a crash from this iteration. I'll add a comment in VMInspector to document why this is safe. >> Source/JavaScriptCore/tools/VMInspector.cpp:63 >> +auto VMInspector::lock(unsigned timeoutInSeconds) -> Expected<LockToken, Error> > > Seconds for timeout Will fix. >> Source/JavaScriptCore/tools/VMInspector.cpp:77 >> +#endif > > I wonder if WTF::sleep(Seconds) works from signal handlers. It probably doesn't, but you could fix that. > > Basically you just need it to stop using ParkingLot for sleeping, since that grabs locks. Just make it call usleep on POSIX and its equivalent on Windows, I think just Sleep(). sigaction's man page explicitly states that sleep() is safe to call from signal handlers. There's no mention of usleep() nor nanosleep() (which usleep() is built on). The man pages for usleep() and nanosleep() also do not say anything about signal safety. We also don't care about the Windows port yet because we're not supporting signal handlers on Windows yet. For these reasons, I will leave this code as is. >> Source/JavaScriptCore/tools/VMInspector.h:49 >> + Expected<LockToken, Error> lock(unsigned timeout = std::numeric_limits<unsigned>::max()); > > should be Seconds timeout = Seconds::infinity() Will fix.
Created attachment 300459 [details] proposed patch with Fil's feedback + the feature is now disabled by default.
Attachment 300459 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:64: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:103: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/tools/VMInspector.cpp:130: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 4 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 300459 [details] proposed patch with Fil's feedback + the feature is now disabled by default. Nice!
Thanks for the review. Landed in r211603: <http://trac.webkit.org/r211603>.
Follow up build fix for CLOOP build landed in r211609: <http://trac.webkit.org/r211609>.
Comment on attachment 300459 [details] proposed patch with Fil's feedback + the feature is now disabled by default. View in context: https://bugs.webkit.org/attachment.cgi?id=300459&action=review > Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:172 > + sigaction(SIGILL, &oldSigIllAction, nullptr); Why call this here? What does this do when called from here? Why not just directly call the old handler?
Comment on attachment 300459 [details] proposed patch with Fil's feedback + the feature is now disabled by default. View in context: https://bugs.webkit.org/attachment.cgi?id=300459&action=review >> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:172 >> + sigaction(SIGILL, &oldSigIllAction, nullptr); > > Why call this here? What does this do when called from here? Why not just directly call the old handler? This uninstalls this crash handler and restores the old one. When we return from this handler, the SIGILL will fire again and the old one will be called.
(In reply to comment #20) > Thanks for the review. Landed in r211603: <http://trac.webkit.org/r211603>. It broke The Linux AArch64 build: ../../Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:36:24: fatal error: A64DOpcode.h: No such file or directory #include "A64DOpcode.h" ^ compilation terminated. fix landed in https://trac.webkit.org/changeset/211630