NewVM: Incorrect DUP2_X1 instruction

[GoogleCodeExporter]

Currently DUP_X1 and DUP2_X1 are implemented by the same macro:

#define BC_DUP2_X1() {\
    stack[stackPointer].data.l = stack[stackPointer - 1].data.l; \
    stack[stackPointer - 1].data.l = stack[stackPointer - 2].data.l; \
    stack[stackPointer - 2].data.l = stack[stackPointer].data.l; \
    stack[stackPointer].type = stack[stackPointer - 1].type; \
    stack[stackPointer - 1].type = stack[stackPointer - 2].type; \
    stack[stackPointer - 2].type = stack[stackPointer].type; \
    safeRetain(&stack[stackPointer]); \
    safeRetain(&stack[stackPointer - 1]); \
    stackPointer++; \

DUP_X1 should work fine with this because it should only be used for single 
word items anyways.  DUP2_X1 might be problematic though.

From The docs for DUP2_X1:
http://cs.au.dk/~mis/dOvs/jvmspec/ref--12.html

"""
Duplicates the top two-word item on the stack and inserts the duplicate before 
the previous (single-word) item on the stack. Alternatively, this instruction 
could also be used to duplicate two single-word items and insert them before 
the third single-word item on the stack.
"""

This macro may fail in the case where it is duplicating two single-word items.  
 I think we need to check the types and copy two items instead of just one if 
the top item is a single-word item.

This means that we should break DUP2_X1 and DUP_X1 into separate macros again 
also.

Original issue reported on code.google.com by steve.ha...@codenameone.com on 28 Jan 2015 at 6:37

[GoogleCodeExporter]

Same goes for DUP2_X2

Original comment by steve.ha...@codenameone.com on 28 Jan 2015 at 7:48

[GoogleCodeExporter]

I don't understand the scenario where this will fail?
The only case I can see is if the method uses the DUP and we have a long on the 
top of the stack which the JVM expects us to split in the middle into two ints. 
I highly doubt a compiler will generate code like that so I don't see where 
this can fail?

Original comment by shai.almog on 1 Feb 2015 at 7:07

[GoogleCodeExporter]

Yes.  That is the case that doesn't work properly.  I have checked the builds 
of several apps and we don't currently have any uses of it like that (it's easy 
to check).  But this is still a lurking edge case.  And if it ever were to show 
up, it would be hard to debug.  It should be fixed.

Original comment by steve.ha...@codenameone.com on 1 Feb 2015 at 1:21

[GoogleCodeExporter]

Checking the type in runtime would incur a performance penalty on something 
that seams very unlikely. Do you have a different idea?

Original comment by shai.almog on 1 Feb 2015 at 2:59

[GoogleCodeExporter]

I suggest separating DUP_X1 and DUP2_X1 into separate functions.  Then 
implement DUP2_X1 correctly.  In the core (on all the apps I have built), there 
is only one instance of DUP2_X1 that originated from a DUP2_X1 bytecode (it 
occurs in TextField).  All the rest originated from DUP_X1 bytecodes.  
Therefore, it seems that DUP2_X1 is a rarely used bytecode anyways so 
performance penalty for the runtime check would be a non-issue.  Hence, it is 
more important that the bytecode conversion be correct, than performant in this 
case.

Original comment by steve.ha...@codenameone.com on 1 Feb 2015 at 3:21

[GoogleCodeExporter]

OK.

Original comment by shai.almog on 1 Feb 2015 at 3:29