Bug 167714 – Add a SIGILL crash analyzer to make debugging SIGILLs easier.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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.

[Radar WebKit Bug Importer]

<rdar://problem/30318237>

[Mark Lam]

Created attachment 300361 [details]
proposed patch.

Let's try this on the EWSs to make sure it doesn't hurt other platforms.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 300370 [details]
proposed patch.

Let's try the EWS again.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 300371 [details]
proposed patch.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 300373 [details]
proposed patch.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 300375 [details]
proposed patch: with speculative fix for Windows build.

[WebKit Commit Bot]

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.

[Mark Lam]

Created attachment 300389 [details]
proposed patch: with another windows fix.

[WebKit Commit Bot]

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.

[Mark Lam]

Comment on attachment 300389 [details]
proposed patch: with another windows fix.

Ready for a review.

[Filip Pizlo]

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()

[Mark Lam]

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.

[Mark Lam]

Created attachment 300459 [details]
proposed patch with Fil's feedback + the feature is now disabled by default.

[WebKit Commit Bot]

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.

[Filip Pizlo]

Comment on attachment 300459 [details]
proposed patch with Fil's feedback + the feature is now disabled by default.

Nice!

[Mark Lam]

Thanks for the review.  Landed in r211603: <http://trac.webkit.org/r211603>.

[Mark Lam]

Follow up build fix for CLOOP build landed in r211609: <http://trac.webkit.org/r211609>.

[Saam Barati]

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?

[Mark Lam]

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.

[Csaba Osztrogonác]

(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