[
https://issues.apache.org/jira/browse/BCEL-195?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14710415#comment-14710415
]
Sebb commented on BCEL-195:
---------------------------
InstructionHandle.addTargeter(t) currently has the code:
{code}
//if(!targeters.contains(t)) // (A)
targeters.add(t); // (B)
{code}
The patch proposes to enable (A) above.
According to the Javadoc for HashSet, this should make no difference to the
set, as it will only add the value if it is not already contained in it.
So this code already prevents duplicate Targeters, at least in the Set
contained in the InstructionHandle.
Duplicate here means that the hashcodes are the same and the objects must
compare equal.
And of course equal objects must have equal hashcodes. (The reverse is not true)
If either the hashcode or the equals() changes whilst an object is in a set,
then the object may not be found.
Now the hashcode is currently the opcode.
One might think that the opcode was immutable once set up, but GOTO may change
it to GOTO_W and similarly for other instructions (JSR => JSR_W, perhaps
others).
I think it's impossible to support changing an Instruction whilst it is in any
kind of set.
If an Instruction can only be changed when it is not in a Hash, then of course
the problem does not occur, but that may be tricky to do (and probably
impossible to enforce).
There are other sets that use equals() and not hashes, but they will be equally
compromised by changes to the instruction - if the change does not affect the
equals() method, then there's no point doing the change!
I assume that the InstructionList is supposed to contain all distinct
InstructionHandle instances (and the search uses == rather than equals) but I
don't know how that is guaranteed.
The way that IH instances are generated is rather convoluted once the ih_list
cache has been initialised.
One change which should help in debugging would be to ensure that the opcode
and other mutable fields are only accessed via getter/setters.
I will make a start on that.
> addition of hashCode() to generic/Instruction.java breaks Targeters
> -------------------------------------------------------------------
>
> Key: BCEL-195
> URL: https://issues.apache.org/jira/browse/BCEL-195
> Project: Commons BCEL
> Issue Type: Bug
> Components: Main
> Affects Versions: 6.0
> Reporter: Mark Roberts
> Attachments: bcel195.diff, targeters.diff
>
>
> [Revision 1532198|http://svn.apache.org/r1532198] added a {{hashCode()}}
> function to the Instruction class. Unfortunately, this breaks the
> Instruction targeting mechanism. I understand the goal of trying to reuse
> instructions - an 'iadd' is the same as any other 'iadd'. However, one
> 'goto 50' is not the same as another 'goto 50' due to the way Targeters are
> implemented. If branch instructions are reused, then only one entry gets put
> on the Targeter list. So when some api is used to modify the instruction
> list and location 50 becomes location 52 ONLY ONE of the branches gets
> updated. A very bad thing. So unless you modify the hash to special case
> branch instructions (and there might be other instructions needing special
> treatment as well) its broken. We fixed it by simply commenting the hash out
> to make things like they used to be and all works great.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
