Bug 111871 - Cleanup of DFG and Baseline JIT debugging code
Summary: Cleanup of DFG and Baseline JIT debugging code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-08 10:35 PST by Michael Saboff
Modified: 2013-03-15 13:26 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.00 KB, patch)
2013-03-08 11:54 PST, Michael Saboff
fpizlo: review-
Details | Formatted Diff | Diff
Updated patch addressing reviewer comments (6.25 KB, patch)
2013-03-12 15:42 PDT, Michael Saboff
buildbot: commit-queue-
Details | Formatted Diff | Diff
Added: changed handling of calleeCodeBlock to use pointerDump() in dfg/DFGRepatch.cpp::dfgLinkClosureCall (7.20 KB, patch)
2013-03-15 10:51 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2013-03-08 10:35:49 PST
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
Comment 1 Michael Saboff 2013-03-08 11:54:36 PST
Created attachment 192260 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-08 12:02:55 PST
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.
Comment 3 Filip Pizlo 2013-03-08 14:44:42 PST
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
Comment 4 Michael Saboff 2013-03-12 15:42:37 PDT
Created attachment 192826 [details]
Updated patch addressing reviewer comments
Comment 5 Build Bot 2013-03-12 21:45:38 PDT
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
Comment 6 Michael Saboff 2013-03-15 10:51:44 PDT
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.
Comment 7 Geoffrey Garen 2013-03-15 10:54:44 PDT
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
Comment 8 Michael Saboff 2013-03-15 13:26:56 PDT
Committed r145933: <http://trac.webkit.org/changeset/145933>