Bug 103623

Summary:

It should be easy to find code blocks in debug dumps

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, dglazkov, ggaren, gtk-ews, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, ojan, oliver, rakuco, sam, webkit-ews, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

102999

Attachments:

Description	Flags
work in progress	eflews.bot: commit-queue-
sample disassembly	none
the patch	ggaren: review+, eflews.bot: commit-queue-
patch for landing	webkit-ews: commit-queue-
try again	none

Filip Pizlo

Reported 2012-11-29 02:41:31 PST

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.

Attachments
work in progress (93.81 KB, patch) 2012-11-29 02:59 PST, Filip Pizlo	eflews.bot: commit-queue-	Details Formatted Diff Diff
sample disassembly (38.51 KB, text/plain) 2012-11-29 03:17 PST, Filip Pizlo	no flags	Details
the patch (101.69 KB, patch) 2012-11-29 14:44 PST, Filip Pizlo	ggaren: review+ eflews.bot: commit-queue-	Details Formatted Diff Diff
patch for landing (101.85 KB, patch) 2012-11-29 15:14 PST, Filip Pizlo	webkit-ews: commit-queue-	Details Formatted Diff Diff
try again (102.02 KB, patch) 2012-11-29 18:17 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2012-11-29 02:59:38 PST

Created attachment 176689 [details] work in progress

WebKit Review Bot

Comment 2 2012-11-29 03:02:45 PST

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 3 2012-11-29 03:05:45 PST

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 4 2012-11-29 03:10:22 PST

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 5 2012-11-29 03:12:12 PST

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 6 2012-11-29 03:13:20 PST

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

Comment 7 2012-11-29 03:17:15 PST

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 8 2012-11-29 09:22:19 PST

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 9 2012-11-29 12:49:30 PST

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

Comment 10 2012-11-29 14:44:50 PST

Created attachment 176821 [details] the patch

EFL EWS Bot

Comment 11 2012-11-29 14:52:37 PST

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 12 2012-11-29 14:53:25 PST

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 13 2012-11-29 14:58:08 PST

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 14 2012-11-29 15:01:04 PST

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 15 2012-11-29 15:01:25 PST

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

Comment 16 2012-11-29 15:11:01 PST

(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

Comment 17 2012-11-29 15:14:55 PST

Created attachment 176828 [details] patch for landing

Early Warning System Bot

Comment 18 2012-11-29 15:24:42 PST

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 19 2012-11-29 15:25:48 PST

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

Comment 20 2012-11-29 18:17:52 PST

Created attachment 176877 [details] try again

Filip Pizlo

Comment 21 2012-11-29 19:43:53 PST

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

Note You need to log in before you can comment on or make changes to this bug.