`opr_expander` pass generates ill typed code for `if-ne` instruction

[kmicinski]

In the "wedding planner" app @eldr4d linked, there was a specific issue that caused badly formed code after rewriting. The original error thrown by the verifier is:

W/dalvikvm( 2658): VFY: copy1 v1<-v18 type=-1531563544 cat=1
W/dalvikvm( 2658): VFY:  rejecting opcode 0x02 at 0x0091
W/dalvikvm( 2658): VFY:  rejected
Lorg/codehaus/jackson/map/ser/std/MapSerializer;.serializeTypedFields
 (Ljava/util/Map;Lorg/codehaus/jackson/JsonGenerator;Lorg/codehaus/jackson/map/SerializerProvider;)V
W/dalvikvm( 2658): Verifier rejected class
Lorg/codehaus/jackson/map/ser/std/MapSerializer;

The reason why this is happening is that in the instrumented bytecode, the method org.codehaus.jackson.map.ser.std.MapSerializer.serializeTypedFields contains malformed bytecode at the address 0x004CEEDE. The offending instruction at that address is:

0x004CEEDE    op: move/from16, opr [v1], opr [v18]

This comes as the result of a "cleanup" pass within redexer: the opr_expander pass in the Modify module. The purpose of this pass is to fix up bytecode instructions that have been thrown out of range by instrumentation. After instrumenting redexer to print the instructions that are replaced as a result of this phase, I have realized that the original instruction replaced is:

if-ne v9 v18 0x004D06CE

The instruction sequence is replaced with

move/from16 v1 v18
if-ne v9 v1 0x004D06CE

This is a necessary step, because if-ne only works on four bit operands. But the problem is that v1 is somehow causing a type error? I'm not sure what's happening, so I need to look into the machinery that's being used to allocate registers in this phase.

[kmicinski]

I chatted with @jsjeon about this bug this morning. He agrees that redexer is incorrectly calculating the type of the opcode, and subsequently putting a move/from16 rather than a move-object/from16. The bug here is somewhere around this line of code:

https://github.com/plum-umd/redexer/blob/master/src/modify.ml#L1296

I'm looking into this bug now.

[jsjeon]

As mentioned in person, the opr_expander pass not only adjusts opcodes according to the new range of operands, but also insert prologue/epilogue code, if necessary. if-ne is one of those examples: it uses only for bit operands, and thus we need an instruction to move around the operand.

According to bytecode dump @kmicinski sent to me, there is a backward control-flow, where v18 holes an object. At the same time, in a normal pass, v18 may have an integer constant. Therefore, data-flow analysis concluded that the type of v18 is just top I guess, and chose move/from16, which is somewhat general.

I wonder replacing it with move-object/from16 still doesn't resolve this issue, since there are multiple paths that use the same register with different types. The workaround I can come up with is actually using the other free register whose type (inferred via data-flow analysis) is more accurate than just top.

[kmicinski]

It sounds like the correct solution is to modify the instrumentation to attempt to use another register of the right sort when the analysis calculates top for the category, yes?

[jsjeon]

Right, but as you pointed out, that data-flow analysis (reaching definition) should be fixed, too.

[kmicinski]

This issue should be merged with #19.

The current fix is going to happen after white/blacklist configuration for various methods in redexer, but after #19 is implemented correctly this should go away