Bug 141730 – CrashTracer: DFG_CRASH beneath JSC::FTL::LowerDFGToLLVM::compileNode

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

at the default case of the switch in compileNode():

        switch (m_node->op()) {
        case Upsilon:
            compileUpsilon();
            break;
...
        default:
            DFG_CRASH(m_graph, m_node, "Unrecognized node in FTL backend");  // Crash here
            break;
        }

Will change the DFG_CRASH to report the inconsistency and then fail the compile, but not crash the app.

[Michael Saboff]

Created attachment 246777 [details]
Patch

I tested this by manually turning of some cases in LowerDFGToLLVM::compileNode() as well as the various LowerDFGToLLVM::compileXXX().  I was able to run the JS regression tests without crashes, although some of the tests failed due to mismatched output.  Similarly, the WK tests ran normally.

[WebKit Commit Bot]

Attachment 246777 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1601:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1601:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2466:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

(In reply to comment #2)
> Attachment 246777 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1601:  Wrong number
> of spaces before statement. (expected: 24)  [whitespace/indent] [4]
> ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1601:  Wrong number
> of spaces before statement. (expected: 24)  [whitespace/indent] [4]
> ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2466:  Wrong number
> of spaces before statement. (expected: 24)  [whitespace/indent] [4]
> Total errors found: 3 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

No change to the existing indentation.

[Radar WebKit Bug Importer]

<rdar://problem/19868347>

[Geoffrey Garen]

Comment on attachment 246777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246777&action=review

Please find out what happens when we dataLog in the WebProcess.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7037
> +        dataLog("DFG ASSERTION FAILED: ", assertion, "\n");

This should say "FTL".

[Michael Saboff]

Created attachment 246778 [details]
Fixed typo and changed "DFG" to "FTL" as suggested

[WebKit Commit Bot]

Attachment 246778 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1601:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1601:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2466:  Wrong number of spaces before statement. (expected: 24)  [whitespace/indent] [4]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

(In reply to comment #5)
> Comment on attachment 246777 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246777&action=review
> 
> Please find out what happens when we dataLog in the WebProcess.

By default, the errors go to stderr.  It appears that stderr for the WebProcess is thrown away as I can't find it in any of the standard places like Console and syslog.

I will look for a better place to send the logged information.

> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7037
> > +        dataLog("DFG ASSERTION FAILED: ", assertion, "\n");
> 
> This should say "FTL".

Done.

[Geoffrey Garen]

Comment on attachment 246778 [details]
Fixed typo and changed "DFG" to "FTL" as suggested

View in context: https://bugs.webkit.org/attachment.cgi?id=246778&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7040
> +#else
> +        dataLog("FTL ASSERTION FAILED: ", assertion, "\n");
> +        dataLog(file, "(", line, ") : ", function, "\n");
> +        dataLog("While handling node ", node, "\n");
> +#endif

Let's remove the else clause here since the web process will swallow this output.

[Michael Saboff]

Committed r180247: <http://trac.webkit.org/changeset/180247>

[Alexey Proskuryakov]

This patch broke the build:

/Volumes/Data/slave/yosemite-debug/build/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7033:5: error: function 'loweringFailed' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]

[Alexey Proskuryakov]

Attempted a fix with <http://trac.webkit.org/r180249>.

[Filip Pizlo]

Comment on attachment 246778 [details]
Fixed typo and changed "DFG" to "FTL" as suggested

View in context: https://bugs.webkit.org/attachment.cgi?id=246778&action=review

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7040
>> +    void loweringFailed(Node* node, const char* file, int line, const char* function, const char* assertion)
>> +    {
>> +#ifndef NDEBUG
>> +        m_graph.handleAssertionFailure(node, file, line, function, (assertion));
>> +#else
>> +        dataLog("FTL ASSERTION FAILED: ", assertion, "\n");
>> +        dataLog(file, "(", line, ") : ", function, "\n");
>> +        dataLog("While handling node ", node, "\n");
>> +#endif
> 
> Let's remove the else clause here since the web process will swallow this output.

I'm opposed to this change.

It appears that this indiscriminately changes release assertions to debug assertions, which is quite the opposite of what we have been doing in the DFG recently.

Also, this uses NDEBUG to detect behavior, rather than ASSERT_DISABLED.

Ideally, this change should just be rolled out of trunk.  If there is some good reason to have this, then we need to rationalize some things:

1) Should all DFG_ASSERT/DFG_CRASH macros throughout the compiler be replaced with this fail-silent behavior?  (I would disagree with that, strongly.)

2) Which of the DFG_ASSERT/DFG_CRASHes in the FTL lowering should actually be loweringFailed()?  Most of these are covered by FTLCapabilities.  I'm strongly opposed to having a debug-only assert for guarding consistency between FTLCapabilities and FTLLower, especially given that we already run FTLCapabilities just before lowering.

3) Why are we using compile-time macros for this stuff?  The right solution is to use something like Options::crashOnFTLFailure(), which should be set to true.  Of course, if you ship WebKit, you could set it to false.  But it makes no sense to have it set to false for nightlies.

But, ideally, this change should just be rolled out of trunk.  It lowered our test coverage and doesn't fix any trunk bug, since trunk already runs FTLCapabilities before lowering.