From 0302e00d99d063ee658553daf727291d96060906 Mon Sep 17 00:00:00 2001 From: Su5eD Date: Tue, 16 Jul 2024 14:02:10 +0200 Subject: [PATCH] Improve mixin extraction with generated injectors --- .../adapter/patch/MethodContextImpl.java | 3 +- .../sinytra/adapter/patch/PatchInstance.java | 79 +------ .../patch/analysis/InstructionMatcher.java | 14 ++ .../patch/analysis/MethodCallAnalyzer.java | 34 +++ .../adapter/patch/api/MethodContext.java | 5 + .../adapter/patch/api/MethodTransform.java | 4 +- .../patch/api/MethodTransformBuilder.java | 41 ++++ .../org/sinytra/adapter/patch/api/Patch.java | 38 +--- .../adapter/patch/fixes/MethodUpgrader.java | 15 +- .../transformer/BundledMethodTransform.java | 36 +++- .../patch/transformer/ExtractMixin.java | 4 +- .../transformer/ModifyInjectionPoint.java | 2 +- .../transformer/ModifyInjectionTarget.java | 2 +- .../transformer/dynamic/DynamicLVTPatch.java | 2 +- .../DynamicModifyVarAtReturnPatch.java | 3 +- .../dynfix/DynFixArbitraryInjectionPoint.java | 8 +- .../dynfix/DynFixAtVariableAssignStore.java | 8 +- .../dynfix/DynFixMethodComparison.java | 194 +++++++++++++----- .../dynfix/DynFixResolveAmbigousTarget.java | 2 +- .../transformer/dynfix/DynFixSplitMethod.java | 2 +- .../dynfix/DynamicInjectionPointPatch.java | 4 +- .../adapter/patch/util/AdapterUtil.java | 16 +- .../util/MethodTransformBuilderImpl.java | 92 +++++++++ .../adapter/patch/util/OpcodeUtil.java | 11 +- .../gradle/analysis/ReplacedMethodCalls.java | 1 - .../test/mixin/DynamicMixinPatchTest.java | 21 +- .../test/mixin/MinecraftMixinPatchTest.java | 47 +++-- .../adapter/test/mixin/LivingEntityMixin.java | 13 +- 28 files changed, 490 insertions(+), 211 deletions(-) create mode 100644 definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransformBuilder.java create mode 100644 definition/src/main/java/org/sinytra/adapter/patch/util/MethodTransformBuilderImpl.java diff --git a/definition/src/main/java/org/sinytra/adapter/patch/MethodContextImpl.java b/definition/src/main/java/org/sinytra/adapter/patch/MethodContextImpl.java index c231065..8388c0e 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/MethodContextImpl.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/MethodContextImpl.java @@ -172,7 +172,8 @@ public List getTargetMethodLocals(TargetPair target, int startPos return AdapterUtil.summariseLocals(locals, startPos); } - private List computeInjectionTargetInsns(@Nullable TargetPair target) { + @Nullable + public List computeInjectionTargetInsns(@Nullable TargetPair target) { if (target == null) { return List.of(); } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/PatchInstance.java b/definition/src/main/java/org/sinytra/adapter/patch/PatchInstance.java index 7e8d0c3..f0c1757 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/PatchInstance.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/PatchInstance.java @@ -9,8 +9,8 @@ import org.sinytra.adapter.patch.api.*; import org.sinytra.adapter.patch.selector.AnnotationHandle; import org.sinytra.adapter.patch.selector.AnnotationValueHandle; -import org.sinytra.adapter.patch.transformer.*; -import org.sinytra.adapter.patch.transformer.param.TransformParameters; +import org.sinytra.adapter.patch.transformer.ModifyTargetClasses; +import org.sinytra.adapter.patch.util.MethodTransformBuilderImpl; import org.slf4j.Marker; import org.slf4j.MarkerFactory; @@ -143,12 +143,11 @@ public static Optional> findAnnotationValue(@Nullab private record ClassTarget(@Nullable AnnotationValueHandle handle, List targetTypes) {} - protected abstract static class BaseBuilder> implements Builder { + protected abstract static class BaseBuilder> extends MethodTransformBuilderImpl implements Builder { protected final Set targetClasses = new HashSet<>(); protected final Set targetAnnotations = new HashSet<>(); protected Predicate targetAnnotationValues; protected final List classTransforms = new ArrayList<>(); - protected final List transforms = new ArrayList<>(); @Override public T targetClass(String... targets) { @@ -173,60 +172,6 @@ public T modifyTargetClasses(Consumer> consumer) { return transform(new ModifyTargetClasses(consumer)); } - @Override - public T modifyParams(Consumer consumer) { - ModifyMethodParams.Builder builder = ModifyMethodParams.builder(); - consumer.accept(builder); - return transform(builder.build()); - } - - @Override - public T transformParams(Consumer consumer) { - final var builder = new TransformParameters.Builder(); - consumer.accept(builder); - return transform(builder.build()); - } - - @Override - public T modifyTarget(String... methods) { - return transform(new ModifyInjectionTarget(List.of(methods))); - } - - @Override - public T modifyTarget(ModifyInjectionTarget.Action action, String... methods) { - return transform(new ModifyInjectionTarget(List.of(methods), action)); - } - - @Override - public T modifyVariableIndex(int start, int offset) { - return transform(new ChangeModifiedVariableIndex(start, offset)); - } - - @Override - public T modifyMethodAccess(ModifyMethodAccess.AccessChange... changes) { - return transform(new ModifyMethodAccess(List.of(changes))); - } - - @Override - public T extractMixin(String targetClass) { - return transform(ModifyVarUpgradeToModifyExprVal.INSTANCE).transform(new ExtractMixin(targetClass)); - } - - @Override - public T splitMixin(String targetClass) { - return transform(new SplitMixinTransform(targetClass)); - } - - @Override - public T improveModifyVar() { - return transform(ModifyVarUpgradeToModifyExprVal.INSTANCE); - } - - @Override - public T modifyMixinType(String newType, Consumer consumer) { - return transform(new ModifyMixinType(newType, consumer)); - } - @Override public T transform(List classTransforms) { this.classTransforms.addAll(classTransforms); @@ -239,24 +184,6 @@ public T transform(ClassTransform transformer) { return coerce(); } - @Override - public T transform(MethodTransform transformer) { - this.transforms.add(transformer); - return coerce(); - } - - @Override - public T transformMethods(List transformers) { - transformers.forEach(this::transform); - return coerce(); - } - - @Override - public T chain(Consumer consumer) { - consumer.accept(coerce()); - return coerce(); - } - @SuppressWarnings("unchecked") private T coerce() { return (T) this; diff --git a/definition/src/main/java/org/sinytra/adapter/patch/analysis/InstructionMatcher.java b/definition/src/main/java/org/sinytra/adapter/patch/analysis/InstructionMatcher.java index 928e081..80c5147 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/analysis/InstructionMatcher.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/analysis/InstructionMatcher.java @@ -92,6 +92,20 @@ public static boolean test(InsnList first, InsnList second, int flags) { return false; } + public static boolean test(List first, List second, int flags) { + if (first.size() == second.size()) { + for (int i = 0; i < first.size(); i++) { + AbstractInsnNode insn = first.get(i); + AbstractInsnNode otherInsn = second.get(i); + if (!InsnComparator.instructionsEqual(insn, otherInsn, flags)) { + return false; + } + } + return true; + } + return false; + } + public static int count(List list, T item) { return (int) list.stream().filter(item::equals).count(); } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/analysis/MethodCallAnalyzer.java b/definition/src/main/java/org/sinytra/adapter/patch/analysis/MethodCallAnalyzer.java index b94c648..3388501 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/analysis/MethodCallAnalyzer.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/analysis/MethodCallAnalyzer.java @@ -125,6 +125,12 @@ public static List> getInvocationInsns(MethodNode methodN }); } + @Nullable + public static List findMethodCallParamInsns(MethodNode methodNode, MethodInsnNode insn) { + MethodCallInterpreter interpreter = MethodCallAnalyzer.analyzeInterpretMethod(methodNode, new MethodCallInterpreter(insn)); + return interpreter.getTargetArgs(); + } + public static List analyzeMethod(MethodNode methodNode, NaryOperationHandler handler) { return analyzeMethod(methodNode, (insn, values) -> true, handler); } @@ -171,6 +177,34 @@ public static MethodNode findUniqueMethod(Multimap methods, throw new NullPointerException("Method " + name + " not found"); } + private static class MethodCallInterpreter extends SourceInterpreter { + private final MethodInsnNode targetInsn; + private List targetArgs; + + public MethodCallInterpreter(MethodInsnNode targetInsn) { + super(Opcodes.ASM9); + this.targetInsn = targetInsn; + } + + @javax.annotation.Nullable + public List getTargetArgs() { + return this.targetArgs; + } + + @Override + public SourceValue naryOperation(AbstractInsnNode insn, List values) { + if (insn == this.targetInsn && this.targetArgs == null) { + List targetArgs = values.stream() + .map(v -> v.insns.size() == 1 ? v.insns.iterator().next() : null) + .toList(); + if (!targetArgs.contains(null)) { + this.targetArgs = targetArgs; + } + } + return super.naryOperation(insn, values); + } + } + private static class AnalysingSourceInterpreter extends SourceInterpreter { private final BiPredicate> filter; private final NaryOperationHandler handler; diff --git a/definition/src/main/java/org/sinytra/adapter/patch/api/MethodContext.java b/definition/src/main/java/org/sinytra/adapter/patch/api/MethodContext.java index f1620fc..28fa786 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/api/MethodContext.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/api/MethodContext.java @@ -49,6 +49,11 @@ public interface MethodContext { List findInjectionTargetInsns(@Nullable TargetPair target); + /** + * Uncached variant of {@link #findInjectionTargetInsns(TargetPair)} + */ + List computeInjectionTargetInsns(@Nullable TargetPair target); + @Nullable Pair> findInjectionTargetCandidates(ClassLookup lookup); diff --git a/definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransform.java b/definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransform.java index d6eb2b9..bf116ef 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransform.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransform.java @@ -16,8 +16,8 @@ default Collection getAcceptedAnnotations() { return Set.of(); } - default Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodContext methodContext) { - return apply(classNode, methodNode, methodContext, methodContext.patchContext()); + default Patch.Result apply(MethodContext methodContext) { + return apply(methodContext.getMixinClass(), methodContext.getMixinMethod(), methodContext, methodContext.patchContext()); } Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodContext methodContext, PatchContext context); diff --git a/definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransformBuilder.java b/definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransformBuilder.java new file mode 100644 index 0000000..1319722 --- /dev/null +++ b/definition/src/main/java/org/sinytra/adapter/patch/api/MethodTransformBuilder.java @@ -0,0 +1,41 @@ +package org.sinytra.adapter.patch.api; + +import org.jetbrains.annotations.ApiStatus; +import org.sinytra.adapter.patch.transformer.ModifyInjectionTarget; +import org.sinytra.adapter.patch.transformer.ModifyMethodAccess; +import org.sinytra.adapter.patch.transformer.ModifyMethodParams; +import org.sinytra.adapter.patch.transformer.ModifyMixinType; +import org.sinytra.adapter.patch.transformer.param.TransformParameters; + +import java.util.List; +import java.util.function.Consumer; + +public interface MethodTransformBuilder> { + @Deprecated + T modifyParams(Consumer consumer); + + @ApiStatus.Experimental + T transformParams(Consumer consumer); + + T modifyTarget(String... methods); + + T modifyTarget(ModifyInjectionTarget.Action action, String... methods); + + T modifyVariableIndex(int start, int offset); + + T modifyMethodAccess(ModifyMethodAccess.AccessChange... changes); + + T extractMixin(String targetClass); + + T splitMixin(String targetClass); + + T improveModifyVar(); + + T modifyMixinType(String newType, Consumer consumer); + + T transform(MethodTransform transformer); + + T transformMethods(List transformers); + + T chain(Consumer consumer); +} diff --git a/definition/src/main/java/org/sinytra/adapter/patch/api/Patch.java b/definition/src/main/java/org/sinytra/adapter/patch/api/Patch.java index 860cdbe..b56fd19 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/api/Patch.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/api/Patch.java @@ -1,7 +1,6 @@ package org.sinytra.adapter.patch.api; import com.mojang.serialization.Codec; -import org.jetbrains.annotations.ApiStatus; import org.objectweb.asm.Type; import org.objectweb.asm.commons.InstructionAdapter; import org.objectweb.asm.tree.ClassNode; @@ -12,11 +11,6 @@ import org.sinytra.adapter.patch.PatchInstance; import org.sinytra.adapter.patch.selector.AnnotationHandle; import org.sinytra.adapter.patch.selector.AnnotationValueHandle; -import org.sinytra.adapter.patch.transformer.ModifyInjectionTarget; -import org.sinytra.adapter.patch.transformer.ModifyMethodAccess; -import org.sinytra.adapter.patch.transformer.ModifyMethodParams; -import org.sinytra.adapter.patch.transformer.ModifyMixinType; -import org.sinytra.adapter.patch.transformer.param.TransformParameters; import java.util.List; import java.util.function.BiConsumer; @@ -52,7 +46,7 @@ public Result or(Result other) { } } - interface Builder> { + interface Builder> extends MethodTransformBuilder { T targetClass(String... targets); T targetMixinType(String... annotationDescs); @@ -61,38 +55,10 @@ interface Builder> { T modifyTargetClasses(Consumer> consumer); - @Deprecated - T modifyParams(Consumer consumer); - - @ApiStatus.Experimental - T transformParams(Consumer consumer); - - T modifyTarget(String... methods); - - T modifyTarget(ModifyInjectionTarget.Action action, String... methods); - - T modifyVariableIndex(int start, int offset); - - T modifyMethodAccess(ModifyMethodAccess.AccessChange... changes); - - T extractMixin(String targetClass); - - T splitMixin(String targetClass); - - T improveModifyVar(); - - T modifyMixinType(String newType, Consumer consumer); - T transform(List classTransforms); T transform(ClassTransform transformer); - T transform(MethodTransform transformer); - - T transformMethods(List transformers); - - T chain(Consumer consumer); - PatchInstance build(); } @@ -112,7 +78,7 @@ default ClassPatchBuilder targetConstant(double doubleValue) { .orElseGet(() -> handle.getNested("at") .flatMap(at -> at.getValue("value").map(s -> s.get().equals("CONSTANT") && at.>getValue("args").map(AnnotationValueHandle::get).map(t -> t.size() == 1 - && (t.getFirst().equals("doubleValue=" + doubleValue + "D") || t.getFirst().equals("doubleValue=" + doubleValue))) + && (t.getFirst().equals("doubleValue=" + doubleValue + "D") || t.getFirst().equals("doubleValue=" + doubleValue))) .orElse(false))) .orElse(false))); } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/fixes/MethodUpgrader.java b/definition/src/main/java/org/sinytra/adapter/patch/fixes/MethodUpgrader.java index cc63c0b..e1c1c07 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/fixes/MethodUpgrader.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/fixes/MethodUpgrader.java @@ -2,7 +2,6 @@ import com.google.common.collect.ImmutableList; import org.objectweb.asm.Type; -import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.MethodNode; import org.sinytra.adapter.patch.analysis.LocalVarAnalyzer; import org.sinytra.adapter.patch.analysis.params.EnhancedParamsDiff; @@ -22,7 +21,7 @@ public final class MethodUpgrader { - public static void upgradeMethod(ClassNode classNode, MethodNode methodNode, MethodContext methodContext, String originalDesc, String modifiedDesc) { + public static void upgradeMethod(MethodNode methodNode, MethodContext methodContext, String originalDesc, String modifiedDesc) { MethodQualifier cleanQualifier = MethodQualifier.create(originalDesc).orElse(null); if (cleanQualifier == null) { return; @@ -34,11 +33,11 @@ public static void upgradeMethod(ClassNode classNode, MethodNode methodNode, Met if (methodContext.methodAnnotation().matchesDesc(MixinConstants.MODIFY_ARGS)) { ModifyArgsOffsetTransformer.handleModifiedDesc(methodNode, cleanQualifier.desc(), dirtyQualifier.desc()); } else if (methodContext.methodAnnotation().matchesDesc(MixinConstants.WRAP_OPERATION)) { - upgradeWrapOperation(classNode, methodNode, methodContext, cleanQualifier, dirtyQualifier); + upgradeWrapOperation(methodNode, methodContext, cleanQualifier, dirtyQualifier); } } - public static void upgradeCapturedLocals(ClassNode classNode, MethodNode methodNode, MethodContext methodContext) { + public static void upgradeCapturedLocals(MethodNode methodNode, MethodContext methodContext) { AdapterUtil.CapturedLocals capturedLocals = AdapterUtil.getCapturedLocals(methodNode, methodContext); if (capturedLocals == null) { return; @@ -51,7 +50,7 @@ public static void upgradeCapturedLocals(ClassNode classNode, MethodNode methodN } LocalVarAnalyzer.CapturedLocalsTransform transform = LocalVarAnalyzer.analyzeCapturedLocals(capturedLocals, methodNode); - transform.remover().apply(classNode, methodNode, methodContext); + transform.remover().apply(methodContext); List expected = List.of(Type.getArgumentTypes(methodNode.desc)); List required = ImmutableList.builder() @@ -64,11 +63,11 @@ public static void upgradeCapturedLocals(ClassNode classNode, MethodNode methodN .map(LayeredParamsDiffSnapshot.ParamModification::asParameterTransformer) .toList(); MethodTransform patch = TransformParameters.builder().transform(transformers).withOffset().targetType(ParamTransformTarget.METHOD).build(); - patch.apply(classNode, methodNode, methodContext); + patch.apply(methodContext); } } - private static void upgradeWrapOperation(ClassNode classNode, MethodNode methodNode, MethodContext methodContext, MethodQualifier cleanQualifier, MethodQualifier dirtyQualifier) { + private static void upgradeWrapOperation(MethodNode methodNode, MethodContext methodContext, MethodQualifier cleanQualifier, MethodQualifier dirtyQualifier) { if (dirtyQualifier.owner() == null || cleanQualifier.desc() == null) { return; } @@ -87,7 +86,7 @@ private static void upgradeWrapOperation(ClassNode classNode, MethodNode methodN SimpleParamsDiffSnapshot diff = EnhancedParamsDiff.create(originalDesc, modifiedDesc); if (!diff.isEmpty()) { MethodTransform patch = diff.asParameterTransformer(ParamTransformTarget.ALL, false, false); - patch.apply(classNode, methodNode, methodContext); + patch.apply(methodContext); } } } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/BundledMethodTransform.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/BundledMethodTransform.java index e9772e3..b6258f2 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/BundledMethodTransform.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/BundledMethodTransform.java @@ -6,16 +6,48 @@ import org.sinytra.adapter.patch.api.MethodTransform; import org.sinytra.adapter.patch.api.Patch; import org.sinytra.adapter.patch.api.PatchContext; +import org.sinytra.adapter.patch.util.MethodTransformBuilderImpl; +import java.util.Collections; import java.util.List; -public record BundledMethodTransform(List transforms) implements MethodTransform { +public record BundledMethodTransform(List transforms, boolean failFast) implements MethodTransform { + public BundledMethodTransform(List transforms) { + this(transforms, false); + } + @Override public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodContext methodContext, PatchContext context) { Patch.Result result = Patch.Result.PASS; for (MethodTransform transform : this.transforms) { - result = result.or(transform.apply(classNode, methodNode, methodContext, context)); + Patch.Result transformResult = transform.apply(classNode, methodNode, methodContext, context); + if (transformResult == Patch.Result.PASS && this.failFast) { + return result; + } + result = result.or(transformResult); } return result; } + + public static Builder builder() { + return new Builder(); + } + + public static class Builder extends MethodTransformBuilderImpl { + private Builder() { + + } + + public BundledMethodTransform build() { + return build(false); + } + + public BundledMethodTransform build(boolean failFast) { + return new BundledMethodTransform(Collections.unmodifiableList(this.transforms), failFast); + } + + public Patch.Result apply(MethodContext methodContext) { + return build().apply(methodContext); + } + } } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/ExtractMixin.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/ExtractMixin.java index 55f8988..b6b5961 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/ExtractMixin.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/ExtractMixin.java @@ -164,13 +164,13 @@ private static boolean isInheritedMethod(ClassNode cls, MethodInsnNode minsn, bo } private static int fixAccess(int access) { - int visibility = access & 0x7; + int visibility = OpcodeUtil.getAccessVisibility(access); // Lower than protected if (visibility == Opcodes.ACC_PRIVATE || visibility == 0) { // Widen to protected // Add synthetic to avoid mixin complaining about non-private static members being present // Setting the access to public will prevent the member from being renamed - return access & ~0x7 | Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC; + return OpcodeUtil.setAccessVisibility(visibility, Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC); } return access; } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionPoint.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionPoint.java index c2acbcc..fdc9801 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionPoint.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionPoint.java @@ -54,7 +54,7 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont String original = handle.get(); handle.set(this.target); if (!this.dontUpgrade) { - MethodUpgrader.upgradeMethod(classNode, methodNode, methodContext, original, this.target); + MethodUpgrader.upgradeMethod(methodNode, methodContext, original, this.target); } } else { annotation.appendValue("target", this.target); diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionTarget.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionTarget.java index 983acaa..c5f1a4d 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionTarget.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/ModifyInjectionTarget.java @@ -53,7 +53,7 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont } if (methodContext.capturesLocals()) { - MethodUpgrader.upgradeCapturedLocals(classNode, methodNode, methodContext); + MethodUpgrader.upgradeCapturedLocals(methodNode, methodContext); } return Patch.Result.APPLY; diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicLVTPatch.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicLVTPatch.java index 1d2d592..9c4406e 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicLVTPatch.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicLVTPatch.java @@ -87,7 +87,7 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont if (diff != null) { // Apply parameter patch MethodTransform transform = diff.asParameterTransformer(ParamTransformTarget.METHOD, true); - return transform.apply(classNode, methodNode, methodContext); + return transform.apply(methodContext); } } return Patch.Result.PASS; diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicModifyVarAtReturnPatch.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicModifyVarAtReturnPatch.java index 0c75eee..088af1c 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicModifyVarAtReturnPatch.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynamic/DynamicModifyVarAtReturnPatch.java @@ -112,8 +112,7 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont return Patch.Result.PASS; } // Get method call argument instructions - MethodCallInterpreter interpreter = MethodCallAnalyzer.analyzeInterpretMethod(dirtyTarget.methodNode(), new MethodCallInterpreter(dirtyMinsn)); - List args = interpreter.getTargetArgs(); + List args = MethodCallAnalyzer.findMethodCallParamInsns(dirtyTarget.methodNode(), dirtyMinsn); if (args == null) { return Patch.Result.PASS; } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixArbitraryInjectionPoint.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixArbitraryInjectionPoint.java index 788a83f..554bdeb 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixArbitraryInjectionPoint.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixArbitraryInjectionPoint.java @@ -60,19 +60,19 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont if (targetMethodCall != null) { // New method is in the same class? It's possible our target injection point was moved there if (targetMethodCall.owner.equals(data.dirtyTarget().classNode().name) && !methodContext.methodAnnotation().matchesDesc(MixinConstants.INJECT)) { - return tryMoveTargetMethod(classNode, methodNode, targetMethodCall, methodContext); + return tryMoveTargetMethod(targetMethodCall, methodContext); } String newInjectionPoint = Type.getObjectType(targetMethodCall.owner).getDescriptor() + targetMethodCall.name + targetMethodCall.desc; - return new ModifyInjectionPoint("INVOKE", newInjectionPoint, true, false).apply(classNode, methodNode, methodContext); + return new ModifyInjectionPoint("INVOKE", newInjectionPoint, true, false).apply(methodContext); } return Patch.Result.PASS; } - private static Patch.Result tryMoveTargetMethod(ClassNode classNode, MethodNode methodNode, MethodInsnNode insn, MethodContext methodContext) { + private static Patch.Result tryMoveTargetMethod(MethodInsnNode insn, MethodContext methodContext) { String newTarget = insn.name + insn.desc; - return new ModifyInjectionTarget(List.of(newTarget)).apply(classNode, methodNode, methodContext); + return new ModifyInjectionTarget(List.of(newTarget)).apply(methodContext); } private static AbstractInsnNode findCandidates(InstructionMatcher cleanMatcher, MethodNode dirtyTargetMethod, Function, AbstractInsnNode> selector) { diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixAtVariableAssignStore.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixAtVariableAssignStore.java index b574fa9..b5419c1 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixAtVariableAssignStore.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixAtVariableAssignStore.java @@ -77,20 +77,20 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont } if (methodContext.methodAnnotation().matchesDesc(MixinConstants.WRAP_OPERATION)) { - return handleWrapAnnotation(classNode, methodNode, methodContext, data, previousMethodCall); + return handleWrapAnnotation(methodContext, data, previousMethodCall); } // All checks have passed, proceed to patch method String newInjectionPoint = Type.getObjectType(previousMethodCall.owner).getDescriptor() + previousMethodCall.name + previousMethodCall.desc; return new ModifyInjectionPoint((String) null, newInjectionPoint, true, true) - .apply(classNode, methodNode, methodContext); + .apply(methodContext); } // In case the mixin is call-sensitive, we try to keep the orignal injection point if the method was moved - private static Patch.Result handleWrapAnnotation(ClassNode classNode, MethodNode methodNode, MethodContext methodContext, Data data, MethodInsnNode previousMethodCall) { + private static Patch.Result handleWrapAnnotation(MethodContext methodContext, Data data, MethodInsnNode previousMethodCall) { if (previousMethodCall.owner.equals(data.dirtyTarget().classNode().name)) { String newTarget = previousMethodCall.name + previousMethodCall.desc; - return new ModifyInjectionTarget(List.of(newTarget)).apply(classNode, methodNode, methodContext); + return new ModifyInjectionTarget(List.of(newTarget)).apply(methodContext); } return Patch.Result.PASS; } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixMethodComparison.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixMethodComparison.java index 1dfb4e8..58e6ef8 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixMethodComparison.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixMethodComparison.java @@ -1,27 +1,31 @@ package org.sinytra.adapter.patch.transformer.dynfix; +import com.google.common.collect.ImmutableList; import com.mojang.datafixers.util.Pair; import org.jetbrains.annotations.Nullable; import org.objectweb.asm.Opcodes; import org.objectweb.asm.Type; +import org.objectweb.asm.commons.GeneratorAdapter; +import org.objectweb.asm.commons.Method; import org.objectweb.asm.tree.*; import org.sinytra.adapter.patch.analysis.InsnComparator; import org.sinytra.adapter.patch.analysis.InstructionMatcher; +import org.sinytra.adapter.patch.analysis.MethodCallAnalyzer; import org.sinytra.adapter.patch.api.MethodContext; import org.sinytra.adapter.patch.api.MixinConstants; import org.sinytra.adapter.patch.api.Patch; -import org.sinytra.adapter.patch.transformer.ExtractMixin; +import org.sinytra.adapter.patch.selector.AnnotationValueHandle; +import org.sinytra.adapter.patch.transformer.BundledMethodTransform; import org.sinytra.adapter.patch.transformer.ModifyInjectionPoint; -import org.sinytra.adapter.patch.transformer.ModifyInjectionTarget; +import org.sinytra.adapter.patch.util.AdapterUtil; +import org.sinytra.adapter.patch.util.OpcodeUtil; import java.util.*; -import java.util.function.Function; -import java.util.stream.Collectors; import java.util.stream.Stream; -import java.util.stream.StreamSupport; public class DynFixMethodComparison implements DynamicFixer { - public record Data(AbstractInsnNode cleanInjectionInsn) {} + public record Data(AbstractInsnNode cleanInjectionInsn) { + } @Nullable @Override @@ -38,25 +42,24 @@ public Data prepare(MethodContext methodContext) { @Override public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodContext methodContext, Data data) { - List cleanLabels = getLabelsInMethod(methodContext.findCleanInjectionTarget().methodNode()); - List cleanLabelsOriginal = List.copyOf(cleanLabels); + List> cleanLabels = getLabelsInMethod(methodContext.findCleanInjectionTarget().methodNode()); + List> cleanLabelsOriginal = List.copyOf(cleanLabels); AbstractInsnNode cleanInjectionInsn = data.cleanInjectionInsn(); - List cleanMatchedLabels = cleanLabels.stream() - .filter(insns -> StreamSupport.stream(insns.spliterator(), false) - .anyMatch(insn -> InsnComparator.instructionsEqual(cleanInjectionInsn, insn))) + List> cleanMatchedLabels = cleanLabels.stream() + .filter(insns -> insns.contains(cleanInjectionInsn)) .toList(); if (cleanMatchedLabels.size() != 1) { return Patch.Result.PASS; } - InsnList cleanLabel = cleanMatchedLabels.getFirst(); + List cleanLabel = cleanMatchedLabels.getFirst(); - List dirtyLabels = getLabelsInMethod(methodContext.findDirtyInjectionTarget().methodNode()); - List dirtyLabelsOriginal = List.copyOf(dirtyLabels); + List> dirtyLabels = getLabelsInMethod(methodContext.findDirtyInjectionTarget().methodNode()); + List> dirtyLabelsOriginal = List.copyOf(dirtyLabels); - Map matchedLabels = new LinkedHashMap<>(); - for (InsnList cleanInsns : cleanLabelsOriginal) { - for (InsnList dirtyInsns : dirtyLabels) { + Map, List> matchedLabels = new LinkedHashMap<>(); + for (List cleanInsns : cleanLabelsOriginal) { + for (List dirtyInsns : dirtyLabels) { if (InstructionMatcher.test(cleanInsns, dirtyInsns, InsnComparator.IGNORE_VAR_INDEX | InsnComparator.IGNORE_LINE_NUMBERS)) { matchedLabels.put(cleanInsns, dirtyInsns); cleanLabels.remove(cleanInsns); @@ -66,39 +69,30 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont } } - Pair patchRange = findPatchHunkRange(cleanLabel, cleanLabelsOriginal, matchedLabels); + Pair, List> patchRange = findPatchHunkRange(cleanLabel, cleanLabelsOriginal, matchedLabels); if (patchRange == null) { return Patch.Result.PASS; } ClassNode cleanTargetClass = methodContext.findCleanInjectionTarget().classNode(); - List hunkLabels = dirtyLabelsOriginal.subList(dirtyLabelsOriginal.indexOf(patchRange.getFirst()) + 1, dirtyLabelsOriginal.indexOf(patchRange.getSecond())); - for (InsnList insns : hunkLabels) { + List> hunkLabels = dirtyLabelsOriginal.subList(dirtyLabelsOriginal.indexOf(patchRange.getFirst()) + 1, dirtyLabelsOriginal.indexOf(patchRange.getSecond())); + for (List insns : hunkLabels) { for (AbstractInsnNode insn : insns) { if (insn instanceof MethodInsnNode minsn && minsn.getOpcode() == Opcodes.INVOKESTATIC && !minsn.owner.equals(cleanTargetClass.name)) { - // Looks like some code was moved into a static method outside this class - // Attempt extracting mixin - // TODO Create universal transform builder class so that we don't need to reference transformer classes directly - ClassNode targetClass = methodContext.patchContext().environment().dirtyClassLookup().getClass(minsn.owner).orElse(null); - if (targetClass != null) { - MethodNode targetMethod = methodContext.patchContext().environment().dirtyClassLookup().findMethod(minsn.owner, minsn.name, minsn.desc).orElse(null); - if (targetMethod != null && !methodContext.findInjectionTargetInsns(new MethodContext.TargetPair(targetClass, targetMethod)).isEmpty()) { - Patch.Result extractResult = new ExtractMixin(minsn.owner).apply(classNode, methodNode, methodContext); - if (extractResult != Patch.Result.PASS) { - return extractResult.or(new ModifyInjectionTarget(List.of(minsn.name + minsn.desc), ModifyInjectionTarget.Action.OVERWRITE).apply(classNode, methodNode, methodContext)); - } - } + Patch.Result result = attemptExtractMixin(minsn, methodContext); + if (result != Patch.Result.PASS) { + return result; } } } } // If extraction fails, fall back to injecting at the nearest method call in dirty code - for (InsnList insns : hunkLabels) { + for (List insns : hunkLabels) { for (AbstractInsnNode insn : insns) { if (insn instanceof MethodInsnNode minsn) { String newInjectionPoint = Type.getObjectType(minsn.owner).getDescriptor() + minsn.name + minsn.desc; - return new ModifyInjectionPoint("INVOKE", newInjectionPoint, true, false).apply(classNode, methodNode, methodContext); + return new ModifyInjectionPoint("INVOKE", newInjectionPoint, true, false).apply(methodContext); } } } @@ -106,10 +100,122 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont return Patch.Result.PASS; } + private static Patch.Result attemptExtractMixin(MethodInsnNode minsn, MethodContext methodContext) { + // Looks like some code was moved into a static method outside this class + // Attempt extracting mixin + ClassNode targetClass = methodContext.patchContext().environment().dirtyClassLookup().getClass(minsn.owner).orElse(null); + if (targetClass == null) { + return Patch.Result.PASS; + } + MethodNode targetMethod = methodContext.patchContext().environment().dirtyClassLookup().findMethod(minsn.owner, minsn.name, minsn.desc).orElse(null); + if (targetMethod == null) { + return Patch.Result.PASS; + } + adjustInjectorOrdinalForNewMethod(minsn, methodContext); + List newTargetInsns = methodContext.findInjectionTargetInsns(new MethodContext.TargetPair(targetClass, targetMethod)); + if (newTargetInsns.isEmpty()) { + return Patch.Result.PASS; + } + Patch.Result res = BundledMethodTransform.builder() + .extractMixin(minsn.owner) + // Applied only if previous transform succeeds + .modifyTarget(minsn.name + minsn.desc) + .build(true) + .apply(methodContext); + if (res != Patch.Result.PASS) { + return res; + } + // Extraction failed? Let's try something else + return createInjectionPoint(targetClass, minsn, methodContext); + } + + private static Patch.Result createInjectionPoint(ClassNode newTargetClass, MethodInsnNode minsn, MethodContext methodContext) { + Type selfType = Type.getObjectType(methodContext.findDirtyInjectionTarget().classNode().name); + Type[] params = Type.getArgumentTypes(minsn.desc); + int selfIndex = Stream.of(Stream.iterate(0, i -> i < params.length, i -> i + 1) + .filter(i -> params[i].equals(selfType)) + .toList()) + .filter(list -> list.size() == 1) + .map(List::getFirst) + .findFirst() + .orElse(-1); + if (selfIndex == -1) { + return Patch.Result.PASS; + } + + List callInsns = MethodCallAnalyzer.findMethodCallParamInsns(methodContext.findDirtyInjectionTarget().methodNode(), minsn); + if (callInsns == null || callInsns.size() <= selfIndex) { + return Patch.Result.PASS; + } + AbstractInsnNode selfParamInsn = callInsns.get(selfIndex); + if (selfParamInsn instanceof VarInsnNode varInsn && varInsn.getOpcode() == Opcodes.ALOAD && varInsn.var == 0) { + // Cool, out instance is passed into the method. Now let's inject there and call the old mixin method + ClassNode generatedTarget = methodContext.patchContext().environment().classGenerator().getOrGenerateMixinClass(methodContext.getMixinClass(), newTargetClass.name, null); + methodContext.patchContext().environment().refmapHolder().copyEntries(methodContext.getMixinClass().name, generatedTarget.name); + // Generate a method with the same injector annotation + MethodNode originalMixinMethod = methodContext.getMixinMethod(); + String name = originalMixinMethod.name + "$adapter$mirror$" + AdapterUtil.randomString(5); + List originalParams = List.of(Type.getArgumentTypes(originalMixinMethod.desc)); + List newParams = ImmutableList.builder().add(Type.getArgumentTypes(minsn.desc)).add(AdapterUtil.CI_TYPE).build(); + // Make sure we have all required params + if (!new HashSet<>(newParams).containsAll(originalParams)) { + return Patch.Result.PASS; + } + + String desc = Type.getMethodDescriptor(Type.VOID_TYPE, newParams.toArray(Type[]::new)); + // Change target + BundledMethodTransform.builder().modifyTarget(minsn.name + minsn.desc).apply(methodContext); + MethodNode invokerMixinMethod = (MethodNode) generatedTarget.visitMethod(Opcodes.ACC_PRIVATE | Opcodes.ACC_STATIC, name, desc, null, null); + invokerMixinMethod.visibleAnnotations = new ArrayList<>(originalMixinMethod.visibleAnnotations); + // Make original mixin a unique public method + originalMixinMethod.access = OpcodeUtil.setAccessVisibility(originalMixinMethod.access, Opcodes.ACC_PUBLIC); + originalMixinMethod.visibleAnnotations.remove(methodContext.methodAnnotation().unwrap()); + originalMixinMethod.visitAnnotation(MixinConstants.UNIQUE, true); + // Now call the original mixin + GeneratorAdapter gen = new GeneratorAdapter(invokerMixinMethod, invokerMixinMethod.access, invokerMixinMethod.name, invokerMixinMethod.desc); + gen.newLabel(); + gen.loadArg(selfIndex); + for (Type type : originalParams) { + gen.loadArg(newParams.indexOf(type)); + } + gen.invokeVirtual(selfType, new Method(originalMixinMethod.name, originalMixinMethod.desc)); + gen.newLabel(); + gen.returnValue(); + gen.newLabel(); + gen.endMethod(); + return Patch.Result.APPLY; + } + return Patch.Result.PASS; + } + + // TODO This should be an automatic upgrade tbh + private static void adjustInjectorOrdinalForNewMethod(MethodInsnNode minsn, MethodContext methodContext) { + AnnotationValueHandle handle = methodContext.injectionPointAnnotationOrThrow().getValue("ordinal").orElse(null); + int originalOrdinal = handle.get(); + // Temporarily adjust ordinal to account for previous calls that have not been moved to the new class + if (handle != null) { + handle.set(-1); + List insns = methodContext.computeInjectionTargetInsns(methodContext.findDirtyInjectionTarget()); + handle.set(originalOrdinal); + int newOrdinal = originalOrdinal; + for (AbstractInsnNode insn : methodContext.findDirtyInjectionTarget().methodNode().instructions) { + if (insn == minsn) { + break; + } + if (insns.contains(insn)) { + newOrdinal--; + } + } + if (newOrdinal >= 0) { + handle.set(newOrdinal); + } + } + } + @Nullable - private static Pair findPatchHunkRange(InsnList cleanLabel, List cleanLabels, Map matchedLabels) { + private static Pair, List> findPatchHunkRange(List cleanLabel, List> cleanLabels, Map, List> matchedLabels) { // Find last matched dirty label BEFORE the injection point - InsnList dirtyLabelBefore = Stream.iterate(cleanLabels.indexOf(cleanLabel), i -> i > 0, i -> i - 1) + List dirtyLabelBefore = Stream.iterate(cleanLabels.indexOf(cleanLabel), i -> i > 0, i -> i - 1) .map(i -> matchedLabels.get(cleanLabels.get(i))) .filter(Objects::nonNull) .findFirst() @@ -119,7 +225,7 @@ private static Pair findPatchHunkRange(InsnList cleanLabel, } // Find first matched dirty label AFTER the injection point - InsnList dirtyLabelAfter = Stream.iterate(cleanLabels.indexOf(cleanLabel), i -> i < cleanLabels.size(), i -> i + 1) + List dirtyLabelAfter = Stream.iterate(cleanLabels.indexOf(cleanLabel), i -> i < cleanLabels.size(), i -> i + 1) .map(i -> matchedLabels.get(cleanLabels.get(i))) .filter(Objects::nonNull) .findFirst() @@ -131,13 +237,9 @@ private static Pair findPatchHunkRange(InsnList cleanLabel, return Pair.of(dirtyLabelBefore, dirtyLabelAfter); } - private static List getLabelsInMethod(MethodNode methodNode) { - Map labels = StreamSupport.stream(methodNode.instructions.spliterator(), false) - .map(a -> a instanceof LabelNode label ? label : null) - .filter(Objects::nonNull) - .collect(Collectors.toMap(Function.identity(), l -> new LabelNode())); - List list = new ArrayList<>(); - InsnList workingList = null; + private static List> getLabelsInMethod(MethodNode methodNode) { + List> list = new ArrayList<>(); + List workingList = null; for (AbstractInsnNode insn : methodNode.instructions) { if (insn instanceof FrameNode) { continue; @@ -146,9 +248,9 @@ private static List getLabelsInMethod(MethodNode methodNode) { if (workingList != null) { list.add(workingList); } - workingList = new InsnList(); + workingList = new ArrayList<>(); } - workingList.add(insn.clone(labels)); + workingList.add(insn); } return list; } diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixResolveAmbigousTarget.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixResolveAmbigousTarget.java index 447fa1c..58256bf 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixResolveAmbigousTarget.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixResolveAmbigousTarget.java @@ -41,7 +41,7 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont if (!insns.isEmpty()) { String newTarget = target.name + target.desc; LOGGER.debug(MIXINPATCH, "Resolving ambigous method selector of {}.{} to {}", classNode.name, methodNode.name, newTarget); - return new ModifyInjectionTarget(List.of(newTarget)).apply(classNode, methodNode, methodContext); + return new ModifyInjectionTarget(List.of(newTarget)).apply(methodContext); } } return Patch.Result.PASS; diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixSplitMethod.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixSplitMethod.java index cacc381..5d7cff3 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixSplitMethod.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynFixSplitMethod.java @@ -78,7 +78,7 @@ public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodCont MethodNode method = candidates.getFirst(); String newTarget = method.name + method.desc; LOGGER.debug(MIXINPATCH, "Adjusting split method target of {}.{} to {}", classNode.name, methodNode.name, newTarget); - return new ModifyInjectionTarget(List.of(newTarget)).apply(classNode, methodNode, methodContext); + return new ModifyInjectionTarget(List.of(newTarget)).apply(methodContext); } return Patch.Result.PASS; diff --git a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynamicInjectionPointPatch.java b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynamicInjectionPointPatch.java index 458b8f9..dc5a42e 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynamicInjectionPointPatch.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/transformer/dynfix/DynamicInjectionPointPatch.java @@ -11,6 +11,8 @@ import java.util.List; +import static org.sinytra.adapter.patch.PatchInstance.MIXINPATCH; + @SuppressWarnings({"rawtypes", "unchecked"}) public class DynamicInjectionPointPatch implements MethodTransform { private static final Logger LOGGER = LogUtils.getLogger(); @@ -28,7 +30,7 @@ public class DynamicInjectionPointPatch implements MethodTransform { public Patch.Result apply(ClassNode classNode, MethodNode methodNode, MethodContext methodContext, PatchContext context) { if (methodContext.failsDirtyInjectionCheck()) { // TODO Only show in tests - LOGGER.debug("Considering method {}.{}", classNode.name, methodNode.name); + LOGGER.debug(MIXINPATCH, "Considering method {}.{}", classNode.name, methodNode.name); for (DynamicFixer fix : FIXES) { Object data = fix.prepare(methodContext); diff --git a/definition/src/main/java/org/sinytra/adapter/patch/util/AdapterUtil.java b/definition/src/main/java/org/sinytra/adapter/patch/util/AdapterUtil.java index 962c505..085599b 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/util/AdapterUtil.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/util/AdapterUtil.java @@ -12,7 +12,9 @@ import org.objectweb.asm.util.Textifier; import org.objectweb.asm.util.TraceMethodVisitor; import org.sinytra.adapter.patch.analysis.LocalVariableLookup; -import org.sinytra.adapter.patch.api.*; +import org.sinytra.adapter.patch.api.MethodContext; +import org.sinytra.adapter.patch.api.MixinConstants; +import org.sinytra.adapter.patch.api.PatchEnvironment; import org.sinytra.adapter.patch.selector.AnnotationHandle; import org.sinytra.adapter.patch.selector.AnnotationValueHandle; import org.slf4j.Logger; @@ -59,6 +61,18 @@ public static int getLVTIndexForParam(MethodNode method, int paramIndex, Type ty return -1; } + public static String randomString(int length) { + int leftLimit = 48; // numeral '0' + int rightLimit = 122; // letter 'z' + Random random = new Random(); + + return random.ints(leftLimit, rightLimit + 1) + .filter(i -> (i <= 57 || i >= 65) && (i <= 90 || i >= 97)) + .limit(length) + .collect(StringBuilder::new, StringBuilder::appendCodePoint, StringBuilder::append) + .toString(); + } + public static boolean isAnonymousClass(String name) { // Regex: second to last char in class name must be '$', and the class name must end with a number return name.matches("^.+\\$\\d+$"); diff --git a/definition/src/main/java/org/sinytra/adapter/patch/util/MethodTransformBuilderImpl.java b/definition/src/main/java/org/sinytra/adapter/patch/util/MethodTransformBuilderImpl.java new file mode 100644 index 0000000..3358c7a --- /dev/null +++ b/definition/src/main/java/org/sinytra/adapter/patch/util/MethodTransformBuilderImpl.java @@ -0,0 +1,92 @@ +package org.sinytra.adapter.patch.util; + +import org.sinytra.adapter.patch.api.MethodTransform; +import org.sinytra.adapter.patch.api.MethodTransformBuilder; +import org.sinytra.adapter.patch.transformer.*; +import org.sinytra.adapter.patch.transformer.param.TransformParameters; + +import java.util.ArrayList; +import java.util.List; +import java.util.function.Consumer; + +public class MethodTransformBuilderImpl> implements MethodTransformBuilder { + protected final List transforms = new ArrayList<>(); + + @Override + public T modifyParams(Consumer consumer) { + ModifyMethodParams.Builder builder = ModifyMethodParams.builder(); + consumer.accept(builder); + return transform(builder.build()); + } + + @Override + public T transformParams(Consumer consumer) { + TransformParameters.Builder builder = new TransformParameters.Builder(); + consumer.accept(builder); + return transform(builder.build()); + } + + @Override + public T modifyTarget(String... methods) { + return transform(new ModifyInjectionTarget(List.of(methods))); + } + + @Override + public T modifyTarget(ModifyInjectionTarget.Action action, String... methods) { + return transform(new ModifyInjectionTarget(List.of(methods), action)); + } + + @Override + public T modifyVariableIndex(int start, int offset) { + return transform(new ChangeModifiedVariableIndex(start, offset)); + } + + @Override + public T modifyMethodAccess(ModifyMethodAccess.AccessChange... changes) { + return transform(new ModifyMethodAccess(List.of(changes))); + } + + @Override + public T extractMixin(String targetClass) { + return transform(ModifyVarUpgradeToModifyExprVal.INSTANCE) + .transform(new ExtractMixin(targetClass)); + } + + @Override + public T splitMixin(String targetClass) { + return transform(new SplitMixinTransform(targetClass)); + } + + @Override + public T improveModifyVar() { + return transform(ModifyVarUpgradeToModifyExprVal.INSTANCE); + } + + @Override + public T modifyMixinType(String newType, Consumer consumer) { + return transform(new ModifyMixinType(newType, consumer)); + } + + @Override + public T transform(MethodTransform transformer) { + this.transforms.add(transformer); + return coerce(); + } + + @Override + public T transformMethods(List transformers) { + transformers.forEach(this::transform); + return coerce(); + } + + @Override + public T chain(Consumer consumer) { + consumer.accept(coerce()); + return coerce(); + } + + @SuppressWarnings("unchecked") + private T coerce() { + return (T) this; + } +} diff --git a/definition/src/main/java/org/sinytra/adapter/patch/util/OpcodeUtil.java b/definition/src/main/java/org/sinytra/adapter/patch/util/OpcodeUtil.java index 6f85631..896bd38 100644 --- a/definition/src/main/java/org/sinytra/adapter/patch/util/OpcodeUtil.java +++ b/definition/src/main/java/org/sinytra/adapter/patch/util/OpcodeUtil.java @@ -22,7 +22,8 @@ Type.FLOAT_TYPE, new BoxedType(float.class, Float.class), Type.DOUBLE_TYPE, new BoxedType(double.class, Double.class) ); - public record BoxedType(Class primitiveClass, Class boxedClass) {} + public record BoxedType(Class primitiveClass, Class boxedClass) { + } public static boolean isStoreOpcode(int opcode) { return opcode >= Opcodes.ISTORE && opcode <= Opcodes.ASTORE; @@ -62,4 +63,12 @@ public static void castObjectType(Type to, MethodVisitor visitor) { visitor.visitMethodInsn(Opcodes.INVOKEVIRTUAL, boxedName, conversionMethod, conversionDesc, false); } } + + public static int getAccessVisibility(int access) { + return access & 0x7; + } + + public static int setAccessVisibility(int access, int visibility) { + return access & ~0x7 | visibility; + } } diff --git a/plugin/src/main/java/org/sinytra/adapter/gradle/analysis/ReplacedMethodCalls.java b/plugin/src/main/java/org/sinytra/adapter/gradle/analysis/ReplacedMethodCalls.java index 161fc4d..5bde0ca 100644 --- a/plugin/src/main/java/org/sinytra/adapter/gradle/analysis/ReplacedMethodCalls.java +++ b/plugin/src/main/java/org/sinytra/adapter/gradle/analysis/ReplacedMethodCalls.java @@ -146,7 +146,6 @@ private static boolean findOverloadedReplacement(AnalysisContext context, Method return false; } // Same owner, same name, different desc => expanded method - String owner = cleanQualifier.internalOwnerName(); if (cleanQualifier.owner().equals(dirtyQualifier.owner()) && cleanQualifier.name().equals(dirtyQualifier.name()) && !cleanQualifier.desc().equals(dirtyQualifier.desc())) { ParametersDiff diff = ParametersDiff.compareTypeParameters(Type.getArgumentTypes(cleanQualifier.desc()), Type.getArgumentTypes(dirtyQualifier.desc())); if (!diff.insertions().isEmpty() && diff.replacements().isEmpty() && diff.removals().isEmpty()) { diff --git a/test/src/test/java/org/sinytra/adapter/patch/test/mixin/DynamicMixinPatchTest.java b/test/src/test/java/org/sinytra/adapter/patch/test/mixin/DynamicMixinPatchTest.java index 09b0239..c434466 100644 --- a/test/src/test/java/org/sinytra/adapter/patch/test/mixin/DynamicMixinPatchTest.java +++ b/test/src/test/java/org/sinytra/adapter/patch/test/mixin/DynamicMixinPatchTest.java @@ -146,19 +146,28 @@ void testModifiedSliceTarget() throws Exception { @Test void testCompareModifiedMethod() throws Exception { - // TODO This can correctly determine the injection point in the extracted method now, - // but fails to extract because the mixin calls an injected unique method. assertSameCode( "org/sinytra/adapter/test/mixin/LivingEntityMixin", "onUnderwater", - assertTargetMethod(), - assertInjectionPoint() + assertUnique(), + assertHasGeneratedMethod("org/sinytra/adapter/test/mixin/adapter_generated_CommonHooks") + ); + } + + @Test + void testCompareModifiedMethod2() throws Exception { + assertSameCode( + "org/sinytra/adapter/test/mixin/LivingEntityMixin", + "testFrostWalker", + assertUnique(), + assertHasGeneratedMethod("org/sinytra/adapter/test/mixin/adapter_generated_CommonHooks") ); } @Override - protected LoadResult load(String className) throws Exception { + protected LoadResult load(String className, List allowedMethods) throws Exception { final ClassNode patched = loadClass(className); + patched.methods.removeIf(m -> !allowedMethods.contains(m.name)); final PatchEnvironment env = PatchEnvironment.create( new RefmapHolder() { @Override @@ -176,6 +185,6 @@ public void copyEntries(String from, String to) { FabricUtil.COMPATIBILITY_LATEST ); DYNAMIC_PATCHES.forEach(p -> p.apply(patched, env)); - return new LoadResult(patched, loadClass(className)); + return new LoadResult(env, patched, loadClass(className)); } } diff --git a/test/src/test/java/org/sinytra/adapter/patch/test/mixin/MinecraftMixinPatchTest.java b/test/src/test/java/org/sinytra/adapter/patch/test/mixin/MinecraftMixinPatchTest.java index 6e01450..e4a7a62 100644 --- a/test/src/test/java/org/sinytra/adapter/patch/test/mixin/MinecraftMixinPatchTest.java +++ b/test/src/test/java/org/sinytra/adapter/patch/test/mixin/MinecraftMixinPatchTest.java @@ -5,6 +5,9 @@ import org.assertj.core.api.Assertions; import org.objectweb.asm.ClassReader; import org.objectweb.asm.tree.*; +import org.sinytra.adapter.patch.api.MixinClassGenerator; +import org.sinytra.adapter.patch.api.MixinConstants; +import org.sinytra.adapter.patch.api.PatchEnvironment; import org.sinytra.adapter.patch.selector.AnnotationHandle; import org.sinytra.adapter.patch.selector.AnnotationValueHandle; import org.sinytra.adapter.patch.util.AdapterUtil; @@ -19,7 +22,6 @@ import java.util.Comparator; import java.util.List; import java.util.Optional; -import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Predicate; @@ -28,18 +30,24 @@ import java.util.stream.StreamSupport; import java.util.zip.ZipFile; +import static org.junit.jupiter.api.Assertions.assertNotNull; + public abstract class MinecraftMixinPatchTest { private static final Logger LOGGER = LogUtils.getLogger(); - protected abstract LoadResult load(String className) throws Exception; + public interface AssertCallback { + void accept(MethodNode patched, MethodNode expected, PatchEnvironment env); + } + + protected abstract LoadResult load(String className, List allowedMethods) throws Exception; @SafeVarargs protected final void assertSameCode( String className, String testName, - BiConsumer... assertions + AssertCallback... assertions ) throws Exception { - final LoadResult result = load(className); + final LoadResult result = load(className, List.of(testName)); final MethodNode patched = result.patched.methods .stream().filter(m -> m.name.equals(testName)) .findFirst().orElseThrow(); @@ -81,7 +89,7 @@ protected final void assertSameCode( }) .containsExactlyInAnyOrder(expected.localVariables.toArray(LocalVariableNode[]::new)); - Stream.of(assertions).forEach(c -> c.accept(patched, expected)); + Stream.of(assertions).forEach(c -> c.accept(patched, expected, result.env())); } public static class InsnComparator implements Comparator { @@ -109,7 +117,7 @@ public int compare(AbstractInsnNode o1, AbstractInsnNode o2) { } } - public record LoadResult(ClassNode patched, ClassNode expected) { + public record LoadResult(PatchEnvironment env, ClassNode patched, ClassNode expected) { } protected ClassNode loadClass(String name) throws IOException { @@ -145,10 +153,25 @@ protected static ClassLookup createDirtyLookup() { }; } - protected BiConsumer assertTargetMethod() { + protected AssertCallback assertUnique() { + return (patched, expected, env) -> { + Assertions.assertThat(patched.visibleAnnotations.stream().anyMatch(ann -> ann.desc.equals(MixinConstants.UNIQUE))) + .as("Unique method") + .isEqualTo(expected.visibleAnnotations.stream().anyMatch(ann -> ann.desc.equals(MixinConstants.UNIQUE))); + }; + } + + protected AssertCallback assertHasGeneratedMethod(String targetClass) { + return (patched, expected, env) -> { + MixinClassGenerator.GeneratedClass generatedClass = env.classGenerator().getGeneratedMixinClasses().get(targetClass); + assertNotNull(generatedClass, "Missing generated class for " + targetClass); + }; + } + + protected AssertCallback assertTargetMethod() { Function> targetMethodExtractor = node -> new AnnotationHandle(node).>getValue("method").map(AnnotationValueHandle::get).orElseThrow(); - return (patched, expected) -> { + return (patched, expected, env) -> { AnnotationNode patchedMethodAnn = patched.visibleAnnotations.getFirst(); AnnotationNode expectedMethodAnn = expected.visibleAnnotations.getFirst(); @@ -158,14 +181,14 @@ protected BiConsumer assertTargetMethod() { }; } - protected BiConsumer assertInjectionPoint() { + protected AssertCallback assertInjectionPoint() { Function> injectionPointExtractor = node -> new AnnotationHandle(node).getNested("at").map(h -> { String value = h.getValue("value").orElseThrow().get(); String target = h.getValue("target").orElseThrow().get(); return Pair.of(value, target); }).orElseThrow(); - return (patched, expected) -> { + return (patched, expected, env) -> { AnnotationNode patchedMethodAnn = patched.visibleAnnotations.getFirst(); AnnotationNode expectedMethodAnn = expected.visibleAnnotations.getFirst(); @@ -175,7 +198,7 @@ protected BiConsumer assertInjectionPoint() { }; } - protected BiConsumer assertSliceRange() { + protected AssertCallback assertSliceRange() { BiFunction> sliceExtractor = (name, node) -> new AnnotationHandle(node).getNested("slice") .flatMap(ann -> ann.getNested(name)) .map(h -> { @@ -185,7 +208,7 @@ protected BiConsumer assertSliceRange() { }) .orElse(null); - return (patched, expected) -> { + return (patched, expected, env) -> { AnnotationNode patchedMethodAnn = patched.visibleAnnotations.getFirst(); AnnotationNode expectedMethodAnn = expected.visibleAnnotations.getFirst(); diff --git a/test/src/testClasses/java/org/sinytra/adapter/test/mixin/LivingEntityMixin.java b/test/src/testClasses/java/org/sinytra/adapter/test/mixin/LivingEntityMixin.java index 934606c..966a171 100644 --- a/test/src/testClasses/java/org/sinytra/adapter/test/mixin/LivingEntityMixin.java +++ b/test/src/testClasses/java/org/sinytra/adapter/test/mixin/LivingEntityMixin.java @@ -54,11 +54,22 @@ public void onUnderwater(CallbackInfo ci) { ourUniqueMethod(); // Prevent extraction } - @Inject(method = "baseTick", at = @At(value = "INVOKE", target = "Lnet/minecraft/world/entity/LivingEntity;getAirSupply()I")) + @Unique public void onUnderwaterExpected(CallbackInfo ci) { ourUniqueMethod(); } + // https://github.com/Earthcomputer/clientcommands/blob/b5ed9155bdab5606498f6dc6538e5a6bdfad3b70/src/main/java/net/earthcomputer/clientcommands/mixin/rngevents/LivingEntityMixin.java#L83 + @Inject(method = "baseTick", at = @At(value = "FIELD", target = "Lnet/minecraft/world/level/Level;isClientSide:Z", ordinal = 2)) + public void testFrostWalker(CallbackInfo ci) { + ourUniqueMethod(); + } + + @Unique + public void testFrostWalkerExpected(CallbackInfo ci) { + ourUniqueMethod(); + } + @Unique private void ourUniqueMethod() { // Noop