Skip to content

Fix #3108: don't rely on C arg eval order in PUTFIELD/MULTIANEWARRAY #4985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shai-almog merged 1 commit into from
May 19, 2026
+178 −31
Merged

Fix #3108: don't rely on C arg eval order in PUTFIELD/MULTIANEWARRAY #4985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/Field.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,35 +344,33 @@ public void appendInstruction(StringBuilder sbOut) {
valueOpAppended = true;
targetOpAppended = true;
} else {
// Pop semantics here matter: C does not specify the order of
// function argument evaluation, so emitting
// "set_field_X(POP_DOUBLE(), POP_OBJ())" can pop the operands
// in the wrong order on iOS (clang). This manifests when the
// value and target both come from the stack (e.g. after a
// chained assignment "a.x = a.y = expr" that produces a
// dup2_x1 + two putfields). Always read with PEEK and pop
// separately. See issue #3108.
switch(desc.charAt(0)) {
case 'L':
case '[':
b.append("PEEK_OBJ");
//if(useThis) {
// b.append("(1), __cn1ThisObject);\n SP--;\n");
//} else {
b.append("(1), PEEK_OBJ(2));\n POP_MANY(2);\n");
//}
sbOut.append(b);
b.append("PEEK_OBJ(1), PEEK_OBJ(2));\n POP_MANY(2);\n");
sbOut.append(b);
return;
case 'D':
b.append("POP_DOUBLE");
b.append("PEEK_DOUBLE(1), PEEK_OBJ(2));\n SP -= 2;\n");
break;
case 'F':
b.append("POP_FLOAT");
b.append("PEEK_FLOAT(1), PEEK_OBJ(2));\n SP -= 2;\n");
break;
case 'J':
b.append("POP_LONG");
b.append("PEEK_LONG(1), PEEK_OBJ(2));\n SP -= 2;\n");
break;
default:
b.append("POP_INT");
b.append("PEEK_INT(1), PEEK_OBJ(2));\n SP -= 2;\n");
break;
}
//if(useThis) {
// b.append("(), __cn1ThisObject);\n");
//} else {
b.append("(), POP_OBJ());\n");
//}
}
break;
}
Expand Down
44 changes: 29 additions & 15 deletions vm/ByteCodeTranslator/src/com/codename1/tools/translator/bytecodes/MultiArray.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,30 @@ public void appendInstruction(StringBuilder b, List<Instruction> l) {
break;

case 3:
b.append(" PUSH_OBJ(alloc3DArray(threadStateData, POP_INT(), ");
// alloc3DArray params are (innermost, middle, outermost).
// JVM stack: outermost at the bottom, innermost-of-specified at
// the top, so after "SP -= dims" SP[0]..SP[dims-1] hold the
// values outermost..innermost. Any unspecified inner dims are
// passed as -1.
//
// Emitting "alloc3DArray(td, POP_INT(), POP_INT(), POP_INT())"
// is unsafe -- C does not specify function argument evaluation
// order, so clang on iOS can pop them in reverse and silently
// swap the dimensions. Use PEEK semantics with explicit SP
// adjustment instead (same pattern as the 2D case above and
// the PUTFIELD fix from issue #3108).
b.append(" SP -= ").append(dims).append("; PUSH_OBJ(alloc3DArray(threadStateData, ");
switch(dims) {
case 1:
b.append("-1, -1");
b.append("-1, -1, (*SP).data.i");
break;

case 2:
b.append("POP_INT(), -1");
b.append("-1, (*(SP+1)).data.i, (*SP).data.i");
break;

case 3:
b.append("POP_INT(), POP_INT()");
b.append("(*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i");
break;
}
b.append(", &class_array3__");
Expand All @@ -152,24 +164,26 @@ public void appendInstruction(StringBuilder b, List<Instruction> l) {
}
b.append("))); /* MULTIANEWARRAY */\n");
break;

case 4:
b.append(" PUSH_OBJ(alloc4DArray(threadStateData, POP_INT(), ");
// See comment on case 3 -- same rationale. alloc4DArray params
// are (innermost, depth-2, depth-3, outermost).
b.append(" SP -= ").append(dims).append("; PUSH_OBJ(alloc4DArray(threadStateData, ");
switch(dims) {
case 1:
b.append("-1, -1, -1");
b.append("-1, -1, -1, (*SP).data.i");
break;

case 2:
b.append("POP_INT(), -1, -1");
b.append("-1, -1, (*(SP+1)).data.i, (*SP).data.i");
break;

case 3:
b.append("POP_INT(), POP_INT(), -1");
b.append("-1, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i");
break;

case 4:
b.append("POP_INT(), POP_INT(), POP_INT()");
b.append("(*(SP+3)).data.i, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i");
break;
}
b.append(", &class_array4__");
Expand Down
135 changes: 135 additions & 0 deletions ...ests/src/test/java/com/codename1/tools/translator/BytecodeInstructionIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,69 @@ void multiArrayAddsDependenciesAndRegistersArrayTypes() throws Exception {
}
}

/**
* Regression test for the same class of bug as issue #3108, applied to
* MultiArray. The translator used to emit
* alloc3DArray(td, POP_INT(), POP_INT(), POP_INT())
* and the 4D equivalent. C does not specify function-argument evaluation
* order, so clang on iOS would evaluate the POPs in reverse and silently
* swap the array dimensions -- "new int[a][b][c]" could be allocated as if
* it were "new int[c][b][a]".
*
* Asserts the emitted C reads each dimension with PEEK semantics
* ((*SP).data.i, (*(SP+1)).data.i, ...) after a single SP decrement, so
* the dimensions land in the right alloc?DArray slots regardless of how
* the C compiler reorders the function-argument evaluation.
*/
@Test
void multiArrayEmissionIsArgumentOrderSafe() throws Exception {
Set<String> snapshot = snapshotArrayTypes();
try {
// 3D, fully-specified ("new int[a][b][c]").
MultiArray ma3 = new MultiArray("[[[I", 3);
List<String> deps3 = new ArrayList<>();
ma3.addDependencies(deps3);
StringBuilder out3 = new StringBuilder();
ma3.appendInstruction(out3, new ArrayList<>());
String c3 = out3.toString();
assertTrue(c3.contains("SP -= 3"),
"3D MULTIANEWARRAY should decrement SP once for all dims:\n" + c3);
assertTrue(c3.contains("alloc3DArray(threadStateData, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i"),
"3D dims=3 must read dims with PEEK in (innermost, middle, outermost) order:\n" + c3);
assertFalse(c3.contains("POP_INT(), POP_INT()"),
"3D MULTIANEWARRAY must not chain multiple POP_INT calls:\n" + c3);

// 4D, fully-specified ("new int[a][b][c][d]").
MultiArray ma4 = new MultiArray("[[[[I", 4);
List<String> deps4 = new ArrayList<>();
ma4.addDependencies(deps4);
StringBuilder out4 = new StringBuilder();
ma4.appendInstruction(out4, new ArrayList<>());
String c4 = out4.toString();
assertTrue(c4.contains("SP -= 4"),
"4D MULTIANEWARRAY should decrement SP once for all dims:\n" + c4);
assertTrue(c4.contains("alloc4DArray(threadStateData, (*(SP+3)).data.i, (*(SP+2)).data.i, (*(SP+1)).data.i, (*SP).data.i"),
"4D dims=4 must read dims with PEEK in innermost-to-outermost order:\n" + c4);
assertFalse(c4.contains("POP_INT(), POP_INT()"),
"4D MULTIANEWARRAY must not chain multiple POP_INT calls:\n" + c4);

// 2D was already safe; verify the existing pattern is intact.
MultiArray ma2 = new MultiArray("[[Ljava/lang/String;", 2);
List<String> deps2 = new ArrayList<>();
ma2.addDependencies(deps2);
StringBuilder out2 = new StringBuilder();
ma2.appendInstruction(out2, new ArrayList<>());
String c2 = out2.toString();
assertTrue(c2.contains("SP -= 2") &&
c2.contains("(*(SP+1)).data.i, (*SP).data.i"),
"2D MULTIANEWARRAY must keep the existing PEEK pattern:\n" + c2);
assertFalse(c2.contains("POP_INT(), POP_INT()"),
"2D MULTIANEWARRAY must not chain multiple POP_INT calls:\n" + c2);
} finally {
restoreArrayTypes(snapshot);
}
}

@Test
void arrayLengthExpressionReducesAndAssigns() throws Exception {
List<Instruction> instructions = new ArrayList<>();
Expand Down Expand Up @@ -1081,6 +1144,78 @@ void testBasicInstructionCoverage() {
}
}

/**
* Regression test for issue #3108: a chained assignment with widening
* conversion ("this.a = this.b = doubleExpr") emits dup2_x1 + two
* putfield's. The translator's unfolded PUTFIELD path used to emit
* "set_field_X(POP_DOUBLE(), POP_OBJ())" -- but C does not specify the
* order of evaluation of function arguments, so clang on iOS was popping
* the operands in the wrong order, causing the double's bits to be
* interpreted as an object reference and crashing with NPE.
*
* This test asserts that the unfolded primitive PUTFIELD path uses PEEK
* (which has no side effect on SP) followed by an explicit pop, mirroring
* the fix that was already in place for the object PUTFIELD path.
*/
@Test
void putfieldUnfoldedPathUsesPeekNotPop() {
// The unfolded path is taken when valueOp and targetOp are null,
// i.e. when tryReduce could not fold the operands into literals.
// Build a Field with PUTFIELD opcode for each primitive desc and
// check the emitted C.
String[][] cases = {
{"D", "PEEK_DOUBLE(1)"},
{"F", "PEEK_FLOAT(1)"},
{"J", "PEEK_LONG(1)"},
{"I", "PEEK_INT(1)"},
{"Z", "PEEK_INT(1)"},
{"B", "PEEK_INT(1)"},
{"C", "PEEK_INT(1)"},
{"S", "PEEK_INT(1)"},
};
for (String[] c : cases) {
String desc = c[0];
String expectedPeek = c[1];
com.codename1.tools.translator.bytecodes.Field f =
new com.codename1.tools.translator.bytecodes.Field(
Opcodes.PUTFIELD, "MyClass", "myField", desc);
StringBuilder out = new StringBuilder();
f.appendInstruction(out);
String c1 = out.toString();
assertTrue(c1.contains(expectedPeek),
"PUTFIELD desc=" + desc + " should use " + expectedPeek
+ " but emitted:\n" + c1);
assertTrue(c1.contains("PEEK_OBJ(2)"),
"PUTFIELD desc=" + desc + " should read target with PEEK_OBJ(2):\n" + c1);
// Explicit pop must follow (either SP -= 2 or POP_MANY).
assertTrue(c1.contains("SP -= 2") || c1.contains("POP_MANY"),
"PUTFIELD desc=" + desc + " should pop the consumed slots explicitly:\n" + c1);
// Most importantly: must NOT use POP_X() inside the call where
// argument evaluation order would matter.
assertFalse(c1.contains("POP_DOUBLE(), POP_OBJ()"),
"PUTFIELD double must not rely on C argument evaluation order:\n" + c1);
assertFalse(c1.contains("POP_FLOAT(), POP_OBJ()"),
"PUTFIELD float must not rely on C argument evaluation order:\n" + c1);
assertFalse(c1.contains("POP_LONG(), POP_OBJ()"),
"PUTFIELD long must not rely on C argument evaluation order:\n" + c1);
assertFalse(c1.contains("POP_INT(), POP_OBJ()"),
"PUTFIELD int must not rely on C argument evaluation order:\n" + c1);
}

// Object PUTFIELD already used PEEK + POP_MANY; cover it too so a
// future change can't silently regress it back to POP.
com.codename1.tools.translator.bytecodes.Field objField =
new com.codename1.tools.translator.bytecodes.Field(
Opcodes.PUTFIELD, "MyClass", "myField", "Ljava/lang/Object;");
StringBuilder objOut = new StringBuilder();
objField.appendInstruction(objOut);
String objC = objOut.toString();
assertTrue(objC.contains("PEEK_OBJ(1)") && objC.contains("PEEK_OBJ(2)"),
"Object PUTFIELD must read both operands with PEEK:\n" + objC);
assertFalse(objC.contains("POP_OBJ(), POP_OBJ()"),
"Object PUTFIELD must not rely on C argument evaluation order:\n" + objC);
}

@Test
void testArithmeticExpressionCoverage() {
// Use tryReduce to construct an ArithmeticExpression since constructor is private
Expand Down
Loading