Bug 75595

Summary: Default HashTraits for Opcode don't work for Opcode = 0
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Severity: Normal CC: ggaren, harrison, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Proposed patch
Patch with indent change for stylebot
ggaren: review-
Updated patch as discussed oliver: review+

Description Michael Saboff 2012-01-04 18:24:17 PST
Opcode is defined as a void* and the value '0' is used in a HaspMap when COMPUTED_GOTO_INTERPRETER is enabled.

An Opcode specific HashTraits needs to be created to account for this.
Comment 1 Michael Saboff 2012-01-04 18:44:04 PST
Created attachment 121202 [details]
Proposed patch
Comment 2 WebKit Review Bot 2012-01-04 18:46:25 PST
Attachment 121202 [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/interpreter/Interpreter.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 2 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Michael Saboff 2012-01-04 18:51:21 PST
Created attachment 121203 [details]
Patch with indent change for stylebot
Comment 4 Geoffrey Garen 2012-01-05 11:21:49 PST
Comment on attachment 121203 [details]
Patch with indent change for stylebot

A few comments:

1. I believe you can just use WTF::UnsignedWithZeroKeyHashTraits. You can do this by putting UnsignedWithZeroKeyHashTraits in the declaration of any hash table using Opcode as key, or by declaring HashTraits<Opcode> to inherit from UnsignedWithZeroKeyHashTraits. This is better than duplicating UnsignedWithZeroKeyHashTraits.

2. This code should not be conditional on #if ENABLE(COMPUTED_GOTO_INTERPRETER). That feature define does not have any bearing on whether the Opcode type has 0 in its domain.

3. The best place to put HashTraits for Opcode is Opcode.h. That way, it's impossible to put Opcode into a hash table inocrrectly.
Comment 5 Geoffrey Garen 2012-01-05 11:50:23 PST
Even better solution: fix up Interpreter::initialize, as we discussed.
Comment 6 Michael Saboff 2012-01-05 18:12:28 PST
Created attachment 121379 [details]
Updated patch as discussed
Comment 7 David Harrison 2012-01-06 10:11:07 PST
Comment 8 Geoffrey Garen 2012-01-06 10:53:57 PST
I think you can remove the whole "if (canUseJIT)" case from Interpreter::initialize. m_opcodeTable isn't used at all if Opcodes are not pointers.
Comment 9 Michael Saboff 2012-01-06 14:15:46 PST
Committed r104338: <http://trac.webkit.org/changeset/104338>