From d9fc28f04ba37944fe9c382d09eac83de76edafa Mon Sep 17 00:00:00 2001 From: Matyrobbrt Date: Sun, 21 Jan 2024 19:06:27 +0200 Subject: [PATCH 1/2] Tests for swapping and substitution --- definition/build.gradle.kts | 12 +- .../patch/transformer/LVTSnapshot.java | 102 +++++++++++++ .../patch/transformer/ModifyMethodParams.java | 62 ++++---- .../patch/test/mixin/MixinPatchTest.java | 136 ++++++++++++++++++ .../test/mixin/ParameterSubstitutionTest.java | 47 ++++++ .../patch/test/mixin/ParameterSwapTest.java | 51 +++++++ .../test/classes/ParameterSubstitution.java | 15 ++ .../adapter/test/classes/ParameterSwap.java | 15 ++ .../mixins/ParameterSubstitutionMixin.java | 38 +++++ .../test/mixins/ParameterSwapMixin.java | 39 +++++ 10 files changed, 491 insertions(+), 26 deletions(-) create mode 100644 definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java create mode 100644 definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java create mode 100644 definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSubstitutionTest.java create mode 100644 definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSwapTest.java create mode 100644 definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSubstitution.java create mode 100644 definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSwap.java create mode 100644 definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSubstitutionMixin.java create mode 100644 definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSwapMixin.java diff --git a/definition/build.gradle.kts b/definition/build.gradle.kts index 09ae490..e98d90d 100644 --- a/definition/build.gradle.kts +++ b/definition/build.gradle.kts @@ -21,6 +21,8 @@ java { withSourcesJar() } +val testClasses = sourceSets.create("testClasses") + repositories { mavenCentral() maven { @@ -38,7 +40,7 @@ dependencies { implementation(group = "com.mojang", name = "logging", version = "1.1.1") implementation(group = "com.google.guava", "guava", version = "32.1.2-jre") implementation(group = "org.slf4j", "slf4j-api", "2.0.0") - implementation(group = "net.fabricmc", name = "sponge-mixin", version = "0.12.5+mixin.0.8.5") + "testClassesImplementation"(implementation(group = "net.fabricmc", name = "sponge-mixin", version = "0.12.5+mixin.0.8.5")) compileOnly(group = "org.jetbrains", name = "annotations", version = "24.0.1") implementation(group = "io.github.llamalad7", name = "mixinextras-common", version = "0.3.1") @@ -51,6 +53,9 @@ dependencies { testImplementation(platform("org.junit:junit-bom:5.9.1")) testImplementation("org.junit.jupiter:junit-jupiter") + testImplementation("org.assertj:assertj-core:3.25.1") + + "testRuntimeOnly"(testClasses.output) } tasks { @@ -63,6 +68,11 @@ tasks { test { useJUnitPlatform() systemProperty("adapter.definition.paramdiff.debug", true) + outputs.upToDateWhen { false } + } + + named("compileTestClassesJava", JavaCompile::class.java) { + options.compilerArgs = listOf("-parameters") } } diff --git a/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java new file mode 100644 index 0000000..07bbdab --- /dev/null +++ b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java @@ -0,0 +1,102 @@ +package dev.su5ed.sinytra.adapter.patch.transformer; + +import dev.su5ed.sinytra.adapter.patch.util.AdapterUtil; +import dev.su5ed.sinytra.adapter.patch.util.SingleValueHandle; +import it.unimi.dsi.fastutil.ints.Int2IntArrayMap; +import it.unimi.dsi.fastutil.ints.Int2IntMap; +import it.unimi.dsi.fastutil.ints.IntArraySet; +import it.unimi.dsi.fastutil.ints.IntSet; +import org.jetbrains.annotations.NotNull; +import org.objectweb.asm.Type; +import org.objectweb.asm.tree.MethodNode; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; + +public class LVTSnapshot { + private final List locals; + private final int[] vars; + + public LVTSnapshot(List locals, int[] vars) { + this.locals = locals; + this.vars = vars; + } + + public void applyDifference(MethodNode newNode) { + final LVTSnapshot newLVT = take(newNode); + + // A new local var was removed + // Shift all other vars after it + final List removed = new ArrayList<>(this.locals); + removed.removeIf(newLVT.locals::contains); + int[] newVars = Arrays.copyOf(this.vars, this.vars.length); + for (LocalVar local : removed) { + for (int i = 0; i < newVars.length; i++) { + if (newVars[i] > local.index) { + newVars[i] -= local.desc.getSize(); + } + } + newNode.localVariables.forEach(node -> { + if (node.index > local.index) { + node.index -= local.desc.getSize(); + } + }); + } + + // A new local var was added + // Shift all other vars after it, including the one that was replaced + final List added = new ArrayList<>(newLVT.locals); + added.removeIf(this.locals::contains); + for (LocalVar local : added) { + for (int i = 0; i < newVars.length; i++) { + if (newVars[i] >= local.index) { + newVars[i] += local.desc.getSize(); + } + } + newNode.localVariables.forEach(node -> { + if (node.index >= local.index) { + node.index += local.desc.getSize(); + } + }); + } + + final Int2IntMap old2New = new Int2IntArrayMap(); + for (int i = 0; i < this.vars.length; i++) { + old2New.put(this.vars[i], newVars[i]); + } + + newNode.instructions.forEach(insn -> { + SingleValueHandle handle = AdapterUtil.handleLocalVarInsnValue(insn); + if (handle != null) { + final int idx = handle.get(); + handle.set(old2New.getOrDefault(idx, idx)); + } + }); + } + + public static LVTSnapshot take(MethodNode node) { + final List locals = new ArrayList<>(); + final IntSet vars = new IntArraySet(); + node.localVariables.forEach(local -> locals.add(new LocalVar(local.name, Type.getType(local.desc), local.index))); + node.instructions.forEach(insn -> { + SingleValueHandle handle = AdapterUtil.handleLocalVarInsnValue(insn); + if (handle != null) { + vars.add((int) handle.get()); + } + }); + final int[] varsArray = vars.toIntArray(); + Arrays.sort(varsArray); + Collections.sort(locals); + return new LVTSnapshot(locals, varsArray); + } + + public record LocalVar(String name, Type desc, int index) implements Comparable { + @Override + public int compareTo(@NotNull LVTSnapshot.LocalVar o) { + return Comparator.naturalOrder().compare(this.index, o.index); + } + } +} diff --git a/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java index a7a92d0..0707ad4 100644 --- a/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java +++ b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java @@ -110,6 +110,8 @@ public Result apply(ClassNode classNode, MethodNode methodNode, MethodContext me continue; } + final LVTSnapshot snapshot = LVTSnapshot.take(methodNode); + int lvtOrdinal = offset + index; int lvtIndex; if (index > offset) { @@ -123,8 +125,8 @@ public Result apply(ClassNode classNode, MethodNode methodNode, MethodContext me newParameterTypes.add(paramOrdinal, type); methodNode.parameters.add(paramOrdinal, newParameter); - int varOffset = AdapterUtil.getLVTOffsetForType(type); - offsetLVT(methodNode, lvtIndex, varOffset); + snapshot.applyDifference(methodNode); + offsetParameters(methodNode, paramOrdinal); offsetSwaps.replaceAll(integerIntegerPair -> integerIntegerPair.mapFirst(j -> j >= paramOrdinal ? j + 1 : j)); @@ -185,19 +187,23 @@ public Result apply(ClassNode classNode, MethodNode methodNode, MethodContext me this.context.substitutes.forEach(pair -> { int paramIndex = pair.getFirst(); int substituteParamIndex = pair.getSecond(); - int localIndex = offset + paramIndex; - int substituteIndex = offset + substituteParamIndex; + int localIndex = calculateLVTIndex(newParameterTypes, isNonStatic, paramIndex); + LVTSnapshot lvtSnapshot = LVTSnapshot.take(methodNode); if (methodNode.parameters.size() > paramIndex) { LOGGER.info("Substituting parameter {} for {} in {}.{}", paramIndex, substituteParamIndex, classNode.name, methodNode.name); methodNode.parameters.remove(paramIndex); newParameterTypes.remove(paramIndex); + int substituteIndex = calculateLVTIndex(newParameterTypes, isNonStatic, substituteParamIndex); methodNode.localVariables.removeIf(lvn -> lvn.index == localIndex); for (AbstractInsnNode insn : methodNode.instructions) { SingleValueHandle handle = AdapterUtil.handleLocalVarInsnValue(insn); - if (handle != null && handle.get() == localIndex) { + if (handle == null) continue; + + if (handle.get() == localIndex) { handle.set(substituteIndex); } } + lvtSnapshot.applyDifference(methodNode); } }); for (Pair swapPair : offsetSwaps) { @@ -206,9 +212,8 @@ public Result apply(ClassNode classNode, MethodNode methodNode, MethodContext me ParameterNode fromNode = methodNode.parameters.get(from); ParameterNode toNode = methodNode.parameters.get(to); - final String fromName = fromNode.name; - fromNode.name = toNode.name; - toNode.name = fromName; + int fromOldLVT = calculateLVTIndex(newParameterTypes, isNonStatic, from); + int toOldLVT = calculateLVTIndex(newParameterTypes, isNonStatic, to); methodNode.parameters.set(from, toNode); methodNode.parameters.set(to, fromNode); @@ -216,10 +221,15 @@ public Result apply(ClassNode classNode, MethodNode methodNode, MethodContext me Type toType = newParameterTypes.get(to); newParameterTypes.set(from, toType); newParameterTypes.set(to, fromType); - LOGGER.info(MIXINPATCH, "Swapped parameters at positions {} and {}", from, to); + LOGGER.info(MIXINPATCH, "Swapped parameters at positions {}({}) and {}({}) in {}.{}", from, fromNode.name, to, toNode.name, classNode.name, methodNode.name); - swapLVT(methodNode, offset, to, from) - .andThen(swapLVT(methodNode, offset, from, to)) + int fromNewLVT = calculateLVTIndex(newParameterTypes, isNonStatic, from); + int toNewLVT = calculateLVTIndex(newParameterTypes, isNonStatic, to); + + // Account for "big" LVT variables (like longs and doubles) + // Uses of the old parameter need to be the new parameter and vice versa + swapLVT(methodNode, fromOldLVT, toNewLVT) + .andThen(swapLVT(methodNode, toOldLVT, fromNewLVT)) .accept(null); } @@ -278,19 +288,28 @@ public Result apply(ClassNode classNode, MethodNode methodNode, MethodContext me return this.context.shouldComputeFrames() ? Result.COMPUTE_FRAMES : Result.APPLY; } - private Consumer swapLVT(MethodNode methodNode, int offset, int from, int to) { + private int calculateLVTIndex(List parameters, boolean nonStatic, int index) { + int lvt = nonStatic ? 1 : 0; + for (int i = 0; i < index; i++) { + lvt += parameters.get(i).getSize(); + } + return lvt; + } + + private Consumer swapLVT(MethodNode methodNode, int from, int to) { Consumer r = v -> {}; for (LocalVariableNode lvn : methodNode.localVariables) { - if (lvn.index == offset + from) { - r = r.andThen(v -> lvn.index = offset + to); + if (lvn.index == from) { + r = r.andThen(v -> lvn.index = to); } } for (AbstractInsnNode insn : methodNode.instructions) { SingleValueHandle handle = AdapterUtil.handleLocalVarInsnValue(insn); if (handle != null) { - if (handle.get() == offset + from) { - r = r.andThen(v -> handle.set(offset + to)); + if (handle.get() == from) { + LOGGER.info("Swapping in LVT: {} to {}", from, to); + r = r.andThen(v -> handle.set(to)); } } } @@ -299,29 +318,22 @@ private Consumer swapLVT(MethodNode methodNode, int offset, int from, int } private static Pair<@Nullable ParameterNode, @Nullable LocalVariableNode> removeLocalVariable(MethodNode methodNode, int paramIndex, int lvtOffset, int replaceIndex, List newParameterTypes) { + final LVTSnapshot snapshot = LVTSnapshot.take(methodNode); ParameterNode parameter = paramIndex < methodNode.parameters.size() ? methodNode.parameters.remove(paramIndex) : null; methodNode.localVariables.sort(Comparator.comparingInt(lvn -> lvn.index)); LocalVariableNode lvn = methodNode.localVariables.remove(paramIndex + lvtOffset); if (lvn != null) { - int varOffset = AdapterUtil.getLVTOffsetForType(Type.getType(lvn.desc)); - for (LocalVariableNode local : methodNode.localVariables) { - if (local.index > lvn.index) { - local.index -= varOffset; - } - } for (AbstractInsnNode insn : methodNode.instructions) { SingleValueHandle handle = AdapterUtil.handleLocalVarInsnValue(insn); if (handle != null) { if (handle.get() == lvn.index) { handle.set(replaceIndex); } - if (handle.get() > lvn.index) { - handle.set(handle.get() - varOffset); - } } } } newParameterTypes.remove(paramIndex); + snapshot.applyDifference(methodNode); return Pair.of(parameter, lvn); } diff --git a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java new file mode 100644 index 0000000..780ed0d --- /dev/null +++ b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java @@ -0,0 +1,136 @@ +package dev.su5ed.sinytra.adapter.patch.test.mixin; + +import dev.su5ed.sinytra.adapter.patch.api.Patch; +import dev.su5ed.sinytra.adapter.patch.api.PatchEnvironment; +import dev.su5ed.sinytra.adapter.patch.api.RefmapHolder; +import org.assertj.core.api.Assertions; +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.ClassNode; +import org.objectweb.asm.tree.LineNumberNode; +import org.objectweb.asm.tree.LocalVariableNode; +import org.objectweb.asm.tree.MethodNode; +import org.objectweb.asm.tree.ParameterNode; + +import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.Field; +import java.util.Comparator; +import java.util.List; +import java.util.Optional; +import java.util.function.Predicate; +import java.util.stream.Collectors; +import java.util.stream.StreamSupport; + +public abstract class MixinPatchTest { + protected LoadResult load(String className, Patch patch) throws Exception { + final ClassNode patched = load(className); + final PatchEnvironment env = PatchEnvironment.create( + new RefmapHolder() { + @Override + public String remap(String cls, String reference) { + return reference; + } + + @Override + public void copyEntries(String from, String to) { + + } + }, + name -> { + try { + final ClassNode node = new ClassNode(); + new ClassReader(name).accept(node, 0); + return Optional.of(node); + } catch (Exception exception) { + return Optional.empty(); + } + }, + null + ); + patch.apply(patched, env); + return new LoadResult(patched, load(className)); + } + + protected void assertSameCode( + String className, + String testName, + Patch.ClassPatchBuilder patch + ) throws Exception { + final LoadResult result = load(className, patch.build()); + final MethodNode patched = result.patched.methods + .stream().filter(m -> m.name.equals(testName)) + .findFirst().orElseThrow(); + final MethodNode expected = result.expected.methods + .stream().filter(m -> m.name.equals(testName + "Expected")) + .findFirst().orElseThrow(); + + Assertions.assertThat(patched.parameters) + .as("Parameters") + .usingRecursiveFieldByFieldElementComparator() + .withRepresentation(object -> ((List) object) + .stream().map(par -> par.name) + .collect(Collectors.joining("\n"))) + .isEqualTo(expected.parameters); + + final Predicate dontTest = i -> i instanceof LineNumberNode; + Assertions.assertThat(patched.instructions.iterator()) + .toIterable() + .as("Instructions") + .filteredOn(dontTest.negate()) + .usingElementComparator(new InsnComparator()) + .isEqualTo(StreamSupport.stream(expected.instructions.spliterator(), false) + .filter(dontTest.negate()).toList()); + + Assertions.assertThat(patched.localVariables) + .as("LVT") + .usingElementComparator(Comparator.comparingInt(n -> n.index) + .thenComparing(n -> n.name) + .thenComparing(n -> n.desc)) + .withRepresentation(object -> { + if (object instanceof LocalVariableNode[] lvn) { + object = List.of(lvn); + } + return ((List) object).stream() + .map(n -> n.index + ": " + n.name + " (" + n.desc + ")") + .collect(Collectors.joining("\n")); + }) + .containsExactlyInAnyOrder(expected.localVariables.toArray(LocalVariableNode[]::new)); + } + + public static class InsnComparator implements Comparator { + @Override + public int compare(AbstractInsnNode o1, AbstractInsnNode o2) { + if (o1.getClass() != o2.getClass() || o1.getType() != o2.getType()) { + return -1; + } + + for (final Field field : o1.getClass().getDeclaredFields()) { + field.setAccessible(true); + if (!field.getType().isPrimitive() && field.getType() != String.class) { + continue; + } + try { + if (((Comparable) field.get(o1)).compareTo(field.get(o2)) != 0) { + return -1; + } + } catch (Exception exception) { + throw new RuntimeException(exception); + } + } + + return 0; + } + } + + public record LoadResult(ClassNode patched, ClassNode expected) {} + + protected ClassNode load(String name) throws IOException { + final ClassNode n = new ClassNode(); + try (final InputStream is = MixinPatchTest.class.getResourceAsStream("/" + name + ".class")) { + new ClassReader(is).accept(n, 0); + } + return n; + } +} diff --git a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSubstitutionTest.java b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSubstitutionTest.java new file mode 100644 index 0000000..23ecbcf --- /dev/null +++ b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSubstitutionTest.java @@ -0,0 +1,47 @@ +package dev.su5ed.sinytra.adapter.patch.test.mixin; + +import dev.su5ed.sinytra.adapter.patch.api.MixinConstants; +import dev.su5ed.sinytra.adapter.patch.api.Patch; +import org.junit.jupiter.api.Test; + +public class ParameterSubstitutionTest extends MixinPatchTest { + @Test + void testSimpleSubstitution() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterSubstitutionMixin", + "testSubstitution", + Patch.builder() + .targetInjectionPoint("") + .targetMethod("injectTarget") + .targetMixinType(MixinConstants.INJECT) + .modifyParams(params -> params.substitute(1, 0)) + ); + } + + @Test + void testBigSubstitution() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterSubstitutionMixin", + "testBigSubstitution", + Patch.builder() + .targetInjectionPoint("") + .targetMethod("injectTarget2") + .targetMixinType(MixinConstants.INJECT) + .modifyParams(params -> params.substitute(2, 0)) + ); + } + + @Test + void testComplexSubstitution() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterSubstitutionMixin", + "testComplexSubstitution", + Patch.builder() + .targetInjectionPoint("") + .targetMethod("injectTarget3") + .targetMixinType(MixinConstants.INJECT) + .modifyParams(params -> params.substitute(1, 2)) + ); + } + +} diff --git a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSwapTest.java b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSwapTest.java new file mode 100644 index 0000000..68192fb --- /dev/null +++ b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterSwapTest.java @@ -0,0 +1,51 @@ +package dev.su5ed.sinytra.adapter.patch.test.mixin; + +import dev.su5ed.sinytra.adapter.patch.api.MixinConstants; +import dev.su5ed.sinytra.adapter.patch.api.Patch; +import org.junit.jupiter.api.Test; + +public class ParameterSwapTest extends MixinPatchTest { + + // Test swap between two parameters of size 1 + @Test + void testSimpleSwap() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterSwapMixin", + "testSwap", + Patch.builder() + .targetInjectionPoint("") + .targetMethod("injectTarget") + .targetMixinType(MixinConstants.INJECT) + .modifyParams(params -> params.swap(0, 1)) + ); + } + + // Test multiple sequential swaps + @Test + void testComplexSwap() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterSwapMixin", + "testComplexSwap", + Patch.builder() + .targetInjectionPoint("") + .targetMethod("injectTarget2") + .targetMixinType(MixinConstants.INJECT) + .modifyParams(params -> params.swap(2, 1) + .swap(1, 0)) + ); + } + + // Test to make sure that longs and doubles (which use 2 LVT spaces) are accounted for + @Test + void testBigSwap() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterSwapMixin", + "testBigSwap", + Patch.builder() + .targetInjectionPoint("") + .targetMethod("injectTarget3") + .targetMixinType(MixinConstants.INJECT) + .modifyParams(params -> params.swap(0, 1)) + ); + } +} diff --git a/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSubstitution.java b/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSubstitution.java new file mode 100644 index 0000000..c2fead3 --- /dev/null +++ b/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSubstitution.java @@ -0,0 +1,15 @@ +package org.sinytra.adapter.test.classes; + +public class ParameterSubstitution { + public void injectTarget(int a) { + + } + + public void injectTarget2(long a, String b) { + + } + + public void injectTarget3(long a, double b, String c) { + + } +} diff --git a/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSwap.java b/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSwap.java new file mode 100644 index 0000000..8d216a9 --- /dev/null +++ b/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterSwap.java @@ -0,0 +1,15 @@ +package org.sinytra.adapter.test.classes; + +public class ParameterSwap { + public void injectTarget(String p1, int p2) { + + } + + public void injectTarget2(String p1, short p2, long p3) { + + } + + public void injectTarget3(String p1, long p2) { + + } +} diff --git a/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSubstitutionMixin.java b/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSubstitutionMixin.java new file mode 100644 index 0000000..1412888 --- /dev/null +++ b/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSubstitutionMixin.java @@ -0,0 +1,38 @@ +package org.sinytra.adapter.test.mixins; + +import org.sinytra.adapter.test.classes.ParameterSubstitution; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.injection.At; +import org.spongepowered.asm.mixin.injection.Inject; +import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; + +@Mixin(ParameterSubstitution.class) +public class ParameterSubstitutionMixin { + @Inject(at = @At("HEAD"), method = "injectTarget(I)V") + private void testSubstitution(int a, int a1, CallbackInfo ci) { + System.out.println(a + a1); + } + + private void testSubstitutionExpected(int a, CallbackInfo ci) { + System.out.println(a + a); + } + + @Inject(at = @At("HEAD"), method = "injectTarget2(JLjava/lang/String;)V") + private void testBigSubstitution(long a, String b, long a1, CallbackInfo ci) { + System.out.println(b + ": " + a + a1); + } + + private void testBigSubstitutionExpected(long a, String b, CallbackInfo ci) { + System.out.println(b + ": " + a + a); + } + + @Inject(at = @At("HEAD"), method = "injectTarget3(JDLjava/lang/String;)V") + private void testComplexSubstitution(long a, double b, double b1, String c, CallbackInfo ci) { + System.out.println(c + ": " + a + (b + b1 / 2)); + } + + // Substitute b with b1 + private void testComplexSubstitutionExpected(long a, double b1, String c, CallbackInfo ci) { + System.out.println(c + ": " + a + (b1 + b1 / 2)); + } +} diff --git a/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSwapMixin.java b/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSwapMixin.java new file mode 100644 index 0000000..22b3df9 --- /dev/null +++ b/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterSwapMixin.java @@ -0,0 +1,39 @@ +package org.sinytra.adapter.test.mixins; + +import org.sinytra.adapter.test.classes.ParameterSwap; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.injection.At; +import org.spongepowered.asm.mixin.injection.Inject; +import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; + +@Mixin(ParameterSwap.class) +public class ParameterSwapMixin { + @Inject(method = "injectTarget(Ljava/lang/String;I)V", at = @At(value = "HEAD")) + private void testSwap(int theInt, String theString, CallbackInfo ci) { + System.out.println("Int: " + theInt); + System.out.println("Str: " + theString); + } + + private void testSwapExpected(String theString, int theInt, CallbackInfo ci) { + System.out.println("Int: " + theInt); + System.out.println("Str: " + theString); + } + + @Inject(method = "injectTarget2(Ljava/lang/String;SJ)V", at = @At("HEAD")) + private void testComplexSwap(int theInt, short theShort, String theString, CallbackInfo ci) { + System.out.println(theString.repeat(theInt + theShort)); + } + + private void testComplexSwapExpected(String theString, int theInt, short theShort, CallbackInfo ci) { + System.out.println(theString.repeat(theInt + theShort)); + } + + @Inject(method = "injectTarget3(Ljava/lang/String;J)V", at = @At("HEAD")) + private void testBigSwap(long theLong, String theString, CallbackInfo ci) { + System.out.println(theString.repeat((int)theLong / 2)); + } + + private void testBigSwapExpected(String theString, long theLong, CallbackInfo ci) { + System.out.println(theString.repeat((int)theLong / 2)); + } +} From 97802306e118b6c38ad3bccef15e3ffc6d733181 Mon Sep 17 00:00:00 2001 From: Matyrobbrt Date: Sun, 21 Jan 2024 19:47:00 +0200 Subject: [PATCH 2/2] Add test for injection --- definition/build.gradle.kts | 4 +++ .../patch/transformer/LVTSnapshot.java | 2 +- .../patch/transformer/ModifyMethodParams.java | 16 +++------- .../patch/test/ParameterComparisonTest.java | 2 +- .../patch/test/mixin/MixinPatchTest.java | 2 +- .../test/mixin/ParameterInjectionTest.java | 32 +++++++++++++++++++ .../test/classes/ParameterInjection.java | 11 +++++++ .../test/mixins/ParameterInjectionMixin.java | 29 +++++++++++++++++ 8 files changed, 83 insertions(+), 15 deletions(-) create mode 100644 definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterInjectionTest.java create mode 100644 definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterInjection.java create mode 100644 definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterInjectionMixin.java diff --git a/definition/build.gradle.kts b/definition/build.gradle.kts index e98d90d..b374940 100644 --- a/definition/build.gradle.kts +++ b/definition/build.gradle.kts @@ -74,6 +74,10 @@ tasks { named("compileTestClassesJava", JavaCompile::class.java) { options.compilerArgs = listOf("-parameters") } + + named("testClasses") { + dependsOn("compileTestClassesJava") + } } publishing { diff --git a/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java index 07bbdab..3074a5f 100644 --- a/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java +++ b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/LVTSnapshot.java @@ -57,7 +57,7 @@ public void applyDifference(MethodNode newNode) { } } newNode.localVariables.forEach(node -> { - if (node.index >= local.index) { + if (!node.name.equals(local.name) && node.index >= local.index) { node.index += local.desc.getSize(); } }); diff --git a/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java index 0707ad4..1246227 100644 --- a/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java +++ b/definition/src/main/java/dev/su5ed/sinytra/adapter/patch/transformer/ModifyMethodParams.java @@ -112,27 +112,19 @@ public Result apply(ClassNode classNode, MethodNode methodNode, MethodContext me final LVTSnapshot snapshot = LVTSnapshot.take(methodNode); - int lvtOrdinal = offset + index; - int lvtIndex; - if (index > offset) { - List lvt = methodNode.localVariables.stream().sorted(Comparator.comparingInt(lvn -> lvn.index)).toList(); - lvtIndex = lvt.get(lvtOrdinal).index; - } else { - lvtIndex = lvtOrdinal; - } + int lvtIndex = calculateLVTIndex(newParameterTypes, isNonStatic, (needsOffset ? 1 : 0) + index); int paramOrdinal = isNonStatic && needsOffset ? index + 1 : index; - ParameterNode newParameter = new ParameterNode(null, Opcodes.ACC_SYNTHETIC); + ParameterNode newParameter = new ParameterNode("adapter_injected_" + paramOrdinal, Opcodes.ACC_SYNTHETIC); newParameterTypes.add(paramOrdinal, type); methodNode.parameters.add(paramOrdinal, newParameter); - snapshot.applyDifference(methodNode); - offsetParameters(methodNode, paramOrdinal); offsetSwaps.replaceAll(integerIntegerPair -> integerIntegerPair.mapFirst(j -> j >= paramOrdinal ? j + 1 : j)); offsetMoves.replaceAll(integerIntegerPair -> integerIntegerPair.mapFirst(j -> j >= paramOrdinal ? j + 1 : j).mapSecond(j -> j >= paramOrdinal ? j + 1 : j)); - methodNode.localVariables.add(new LocalVariableNode("adapter_injected_" + paramOrdinal, type.getDescriptor(), null, self.start, self.end, lvtIndex)); + methodNode.localVariables.add(paramOrdinal + (isNonStatic ? 1 : 0), new LocalVariableNode(newParameter.name, type.getDescriptor(), null, self.start, self.end, lvtIndex)); + snapshot.applyDifference(methodNode); } LocalVariableLookup lvtLookup = new LocalVariableLookup(methodNode.localVariables); BytecodeFixerUpper bfu = context.environment().bytecodeFixerUpper(); diff --git a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/ParameterComparisonTest.java b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/ParameterComparisonTest.java index a4a36ad..d5aa9cb 100644 --- a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/ParameterComparisonTest.java +++ b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/ParameterComparisonTest.java @@ -112,7 +112,7 @@ public void testMethodParameterNameComparison() throws IOException { System.out.println("Insertions:"); diff.insertions().forEach(param -> System.out.println("AT " + param.getFirst() + " TYPE " + param.getSecond())); assertEquals(1, diff.insertions().size()); - assertEquals(7, diff.insertions().get(0).getFirst()); + assertEquals(9, diff.insertions().get(0).getFirst()); assertEquals(Type.FLOAT_TYPE, diff.insertions().get(0).getSecond()); System.out.println("Replacements:"); diff --git a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java index 780ed0d..c71c806 100644 --- a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java +++ b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/MixinPatchTest.java @@ -68,7 +68,7 @@ protected void assertSameCode( Assertions.assertThat(patched.parameters) .as("Parameters") - .usingRecursiveFieldByFieldElementComparator() + .usingElementComparator(Comparator.comparing(p -> p.name)) .withRepresentation(object -> ((List) object) .stream().map(par -> par.name) .collect(Collectors.joining("\n"))) diff --git a/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterInjectionTest.java b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterInjectionTest.java new file mode 100644 index 0000000..2b903ca --- /dev/null +++ b/definition/src/test/java/dev/su5ed/sinytra/adapter/patch/test/mixin/ParameterInjectionTest.java @@ -0,0 +1,32 @@ +package dev.su5ed.sinytra.adapter.patch.test.mixin; + +import dev.su5ed.sinytra.adapter.patch.api.Patch; +import org.junit.jupiter.api.Test; +import org.objectweb.asm.Type; + +public class ParameterInjectionTest extends MixinPatchTest { + @Test + void testRedirectInjection() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterInjectionMixin", + "testRedirectInjection", + Patch.builder() + .targetMethod("testRedirect") + .targetInjectionPoint("Ljava/lang/String;repeat()Ljava/lang/String;") + .modifyInjectionPoint("Ljava/lang/String;repeat(I)Ljava/lang/String;") + // This is with auto offset + .modifyParams(p -> p.insert(0, Type.INT_TYPE)) + ); + } + + @Test + void testBigParameterInjection() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixins/ParameterInjectionMixin", + "testBigParameterInjection", + Patch.builder() + .targetMethod("testTargetBig") + .modifyParams(p -> p.insert(1, Type.DOUBLE_TYPE)) + ); + } +} diff --git a/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterInjection.java b/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterInjection.java new file mode 100644 index 0000000..ca2957b --- /dev/null +++ b/definition/src/testClasses/java/org/sinytra/adapter/test/classes/ParameterInjection.java @@ -0,0 +1,11 @@ +package org.sinytra.adapter.test.classes; + +public class ParameterInjection { + public void testRedirect(String a, int b) { + a.repeat(b); + } + + public void testTargetBig(long a, double b, float c) { + + } +} diff --git a/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterInjectionMixin.java b/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterInjectionMixin.java new file mode 100644 index 0000000..2f050f9 --- /dev/null +++ b/definition/src/testClasses/java/org/sinytra/adapter/test/mixins/ParameterInjectionMixin.java @@ -0,0 +1,29 @@ +package org.sinytra.adapter.test.mixins; + +import org.sinytra.adapter.test.classes.ParameterInjection; +import org.spongepowered.asm.mixin.Mixin; +import org.spongepowered.asm.mixin.injection.At; +import org.spongepowered.asm.mixin.injection.Inject; +import org.spongepowered.asm.mixin.injection.Redirect; +import org.spongepowered.asm.mixin.injection.callback.CallbackInfo; + +@Mixin(ParameterInjection.class) +public class ParameterInjectionMixin { + @Redirect(method = "testRedirect(Ljava/lang/String;I)V", at = @At(value = "INVOKE", target = "Ljava/lang/String;repeat()Ljava/lang/String;")) + private String testRedirectInjection(String owner) { + return owner.repeat(1); + } + + private String testRedirectInjectionExpected(String owner, int adapter_injected_1) { + return owner.repeat(1); + } + + @Inject(method = "testTargetBig(JDF)V", at = @At("HEAD")) + private void testBigParameterInjection(long a, float c, CallbackInfo ci) { + System.out.println(c / a); + } + + private void testBigParameterInjectionExpected(long a, double adapter_injected_1, float c, CallbackInfo ci) { + System.out.println(c / a); + } +}