Bug 75595 - Default HashTraits for Opcode don't work for Opcode = 0
Summary: Default HashTraits for Opcode don't work for Opcode = 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-01-04 18:24 PST by Michael Saboff
Modified: 2012-01-06 14:15 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch (1.95 KB, patch)
2012-01-04 18:44 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch with indent change for stylebot (1.89 KB, patch)
2012-01-04 18:51 PST, Michael Saboff
ggaren: review-
Details | Formatted Diff | Diff
Updated patch as discussed (2.80 KB, patch)
2012-01-05 18:12 PST, Michael Saboff
oliver: 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 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
rdar://10649745
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>