commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sebb (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (BCEL-195) addition of hashCode() to generic/Instruction.java breaks Targeters
Date Tue, 25 Aug 2015 08:45:45 GMT

    [ https://issues.apache.org/jira/browse/BCEL-195?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14710415#comment-14710415
] 

Sebb edited comment on BCEL-195 at 8/25/15 8:45 AM:
----------------------------------------------------

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 HASH
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 with the current design (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!

[Later] however it does look like it might be possible to use something like TreeSet or ConcurrentArraySet,
which only use equals().

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.


was (Author: sebb@apache.org):
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)

Mime
View raw message