Bug 111871 – Cleanup of DFG and Baseline JIT debugging code

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

[Michael Saboff]

While debugging a problem in the JSC JIT code, I came across three changes that needed to be made for the code to compile and/or run.

The issues were
    An unsafe cast to a double* in dfg/DFGOperations.cpp::debugOperationPrintSpeculationFailure()
    Missing case labels in dfg/DFGSpeculativeJIT.cpp:SpeculativeJIT::checkConsistency()
    Possible dereferencing a null pointer in jit/JITCall32_64.cpp:JIT::privateCompileClosureCall()

These need to be fixed, with the last issue also needing to be addressed in jit/JITCall.cpp

[Michael Saboff]

Created attachment 192260 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp']" exit_code: 1
Source/JavaScriptCore/jit/JITCall.cpp:260:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


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

[Filip Pizlo]

Comment on attachment 192260 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:1634
> -        double value = *reinterpret_cast_ptr<double*>(scratchPointer);
> +        double value = static_cast<double>(bits);

Should fix the scratch buffer to allocate 8-byte aligned.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1124
> +        case DataFormatOSRMarker:
> +        case DataFormatDead:
> +        case DataFormatArguments:

These should be RELEASE_ASSERT_NOT_REACHED().

> Source/JavaScriptCore/jit/JITCall32_64.cpp:340
> -                toCString(*calleeCodeBlock).data())),
> +                calleeCodeBlock ? toCString(*calleeCodeBlock).data() : "Null")),

Can also be written as: toCString(PointerDump(calleeCodeBlock)).data()

>> Source/JavaScriptCore/jit/JITCall.cpp:260
>> -                toCString(*calleeCodeBlock).data())),
>> +             calleeCodeBlock ? toCString(*calleeCodeBlock).data() : "Null")),
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Ditto

[Michael Saboff]

Created attachment 192826 [details]
Updated patch addressing reviewer comments

[Build Bot]

Comment on attachment 192826 [details]
Updated patch addressing reviewer comments

Attachment 192826 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17190214

New failing tests:
editing/selection/selection-modify-crash.html

[Michael Saboff]

Created attachment 193337 [details]
Added: changed handling of calleeCodeBlock to use pointerDump() in dfg/DFGRepatch.cpp::dfgLinkClosureCall

Couldn't get editing/selection/selection-modify-crash.html to crash in my environment.

[Geoffrey Garen]

Comment on attachment 193337 [details]
Added: changed handling of calleeCodeBlock to use pointerDump() in dfg/DFGRepatch.cpp::dfgLinkClosureCall

Looks like Phil's comments have been addressed.

r=me

[Michael Saboff]

Committed r145933: <http://trac.webkit.org/changeset/145933>