Bug 103623 – It should be easy to find code blocks in debug dumps

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Currently debug dumps identify code blocks by their address in memory. This has loads of problems:

- The relationship between code block and executable is not explicitly stated. So, if a code block gets inlined, the inlined code will refer to it by the address of its executable and you won't be able to quickly figure out which code block it was.
- The relationship between code blocks corresponding to different tiers is not stated. So, if a code block gets optimized, or reoptimized, then it will have a different address. It's hard to search debug output for all of the "versions" of a code block.
- If you want to search debug output on a 64-bit machine for a code block, you have to search for the address, which can be quite large. This can mean more typing sometimes.

This can be fixed by giving code blocks some sort of relatively strong, but also relatively compact, hash. We can compute it lazily so that it only impacts run-time when debug support is enabled. We can stringify it smartly so that it's short and easy to type. We can base it on the source code so that the optimization level is irrelevant. And, we can use SHA1 since it's already in our code base.

As part of this, we should also get CodeBlock debugging to use the new world of print() and dataLog(). In particular, CodeBlock::dump() should correspond to the thing you want printed if you do:

dataLog("I heart ", *myCodeBlock);

Probably, you want to just print some identifying information at this point rather than the full bytecode dump. This implies renaming the existing CodeBlock::dump() to CodeBlock::dumpBytecode().

Finally, this new notion of CodeBlockHash should be available for other purposes like bisecting breakage. So it should have all of the comparisons.

[Filip Pizlo]

Created attachment 176689 [details]
work in progress

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/CodeOrigin.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:708:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:782:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:853:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:919:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:992:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:1070:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:1144:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/runtime/CodeSpecializationKind.h:48:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/jit/JITDisassembler.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:751:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:820:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:821:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:889:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:954:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1028:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1180:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/runtime/Executable.h:91:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/Executable.h:301:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/Executable.h:361:  The parameter name "kind" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:240:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:301:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:530:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:742:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:944:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 27 in 35 files


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

[EFL EWS Bot]

Comment on attachment 176689 [details]
work in progress

Attachment 176689 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15026796

[Early Warning System Bot]

Comment on attachment 176689 [details]
work in progress

Attachment 176689 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15023780

[Early Warning System Bot]

Comment on attachment 176689 [details]
work in progress

Attachment 176689 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15027775

[kov's GTK+ EWS bot]

Comment on attachment 176689 [details]
work in progress

Attachment 176689 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15018806

[Filip Pizlo]

Created attachment 176696 [details]
sample disassembly

Sample disassembly, of bitops-bitwise-and.  Note that all of the things related to the benchmark's sole code block can be found by searching for DiBSvy.  That's the code block hash of this benchmark's global code.  It will not change from execution to execution, and so can be used for both casual introspection as well as bisecting.

[WebKit Review Bot]

Comment on attachment 176689 [details]
work in progress

Attachment 176689 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15016902

[Build Bot]

Comment on attachment 176689 [details]
work in progress

Attachment 176689 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15028977

[Filip Pizlo]

Created attachment 176821 [details]
the patch

[EFL EWS Bot]

Comment on attachment 176821 [details]
the patch

Attachment 176821 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15060023

[kov's GTK+ EWS bot]

Comment on attachment 176821 [details]
the patch

Attachment 176821 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15025900

[Early Warning System Bot]

Comment on attachment 176821 [details]
the patch

Attachment 176821 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15057069

[Geoffrey Garen]

Comment on attachment 176821 [details]
the patch

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

r=me

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:72
> +    out.print(hash(), ":[", RawPointer(this), ", ", jitType, codeType());

I wonder if there's a better way to call this out as a hash. Prefix with "#"?

[Early Warning System Bot]

Comment on attachment 176821 [details]
the patch

Attachment 176821 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15056076

[Filip Pizlo]

(In reply to comment #14)
> (From update of attachment 176821 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176821&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:72
> > +    out.print(hash(), ":[", RawPointer(this), ", ", jitType, codeType());
> 
> I wonder if there's a better way to call this out as a hash. Prefix with "#"?

Yeah, that's a good idea.  I made all of the people who print CodeBlockHash's prefix them with #.  This preserves the invariant that

CodeBlockHash(toCString(foo).data()) == foo

and

!strcmp(toCString(CodeBlockHash(foo)).data(), foo)

[Filip Pizlo]

Created attachment 176828 [details]
patch for landing

[Early Warning System Bot]

Comment on attachment 176828 [details]
patch for landing

Attachment 176828 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15044477

[Early Warning System Bot]

Comment on attachment 176828 [details]
patch for landing

Attachment 176828 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15044476

[Filip Pizlo]

Created attachment 176877 [details]
try again

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/136199