From 74a1696d993ce782609874c7d8cbfaf271c31c02 Mon Sep 17 00:00:00 2001 From: LlamaLad7 Date: Fri, 19 Jul 2024 18:40:44 +0100 Subject: [PATCH 1/9] Fix: Fix `NEW` descriptor gating. Bafflingly, Mixin fails to match a `NEW` instruction whose `` call is outside the slice range, *iff the user specified a descriptor*. I'm even more surprised that a mod relied on this. Fixes #152. --- .../asm/mixin/injection/points/BeforeNew.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java b/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java index 14850b2d3..8e8b13d3e 100644 --- a/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java +++ b/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java @@ -34,6 +34,7 @@ import org.objectweb.asm.tree.InsnList; import org.objectweb.asm.tree.MethodInsnNode; import org.objectweb.asm.tree.TypeInsnNode; +import org.spongepowered.asm.mixin.FabricUtil; import org.spongepowered.asm.mixin.injection.InjectionPoint; import org.spongepowered.asm.mixin.injection.InjectionPoint.AtCode; import org.spongepowered.asm.mixin.injection.selectors.ITargetSelector; @@ -106,6 +107,11 @@ public class BeforeNew extends InjectionPoint { */ private final int ordinal; + /** + * Fabric: Whether to ignore the supplied descriptor in keeping with 0.8.5 and below. + */ + private final boolean ignoreDesc; + public BeforeNew(InjectionPointData data) { super(data); @@ -118,7 +124,8 @@ public BeforeNew(InjectionPointData data) { } ITargetSelectorConstructor targetSelector = (ITargetSelectorConstructor)member; this.target = targetSelector.toCtorType(); - this.desc = org.spongepowered.asm.mixin.FabricUtil.getCompatibility(data.getContext()) >= org.spongepowered.asm.mixin.FabricUtil.COMPATIBILITY_0_14_0 ? targetSelector.toCtorDesc() : null; + this.desc = targetSelector.toCtorDesc(); + this.ignoreDesc = FabricUtil.getCompatibility(data.getContext()) < FabricUtil.COMPATIBILITY_0_14_0; } /** @@ -132,7 +139,7 @@ public boolean hasDescriptor() { * Gets the descriptor from the injection point, can return null */ public String getDescriptor() { - return this.desc; + return this.ignoreDesc ? null : this.desc; } @SuppressWarnings("unchecked") @@ -159,7 +166,7 @@ public boolean find(String desc, InsnList insns, Collection no if (this.desc != null) { for (TypeInsnNode newNode : newNodes) { - if (BeforeNew.findInitNodeFor(insns, newNode, this.desc) != null) { + if (BeforeNew.findInitNodeFor(insns, newNode, this.getDescriptor()) != null) { nodes.add(newNode); found = true; } From 81284c8401b657f5805b670f4628a734b4561807 Mon Sep 17 00:00:00 2001 From: LlamaLad7 Date: Sat, 13 Jul 2024 18:56:08 +0100 Subject: [PATCH 2/9] Fix: Remove incorrect `PUTFIELD` owner check when inspecting initialisers. These will never match because the LHS is the mixin name and the RHS is the target name. This restores the 0.8.5 behaviour which does not check the owner at all. We have never supported assignment expressions in initialisers until now anyway, and even if we check the owner we cannot determine whether the instruction is operating on the current *instance* without proper static analysis, so IMO there's not much point bothering. --- .../spongepowered/asm/mixin/injection/struct/Constructor.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/org/spongepowered/asm/mixin/injection/struct/Constructor.java b/src/main/java/org/spongepowered/asm/mixin/injection/struct/Constructor.java index 943edadf2..615b6c28d 100644 --- a/src/main/java/org/spongepowered/asm/mixin/injection/struct/Constructor.java +++ b/src/main/java/org/spongepowered/asm/mixin/injection/struct/Constructor.java @@ -113,10 +113,6 @@ public void inspect(Initialiser initialiser) { for (AbstractInsnNode initialiserInsn : initialiser.getInsns()) { if (initialiserInsn.getOpcode() == Opcodes.PUTFIELD) { - FieldInsnNode fieldInsn = (FieldInsnNode)initialiserInsn; - if (!fieldInsn.owner.equals(this.targetName)) { - continue; - } this.mixinInitialisedFields.add(Constructor.fieldKey((FieldInsnNode)initialiserInsn)); } } From a4aaaa92c6ef6bc6140e0fb8f3de71e404e03fcc Mon Sep 17 00:00:00 2001 From: modmuss Date: Tue, 13 Aug 2024 08:40:50 +0100 Subject: [PATCH 3/9] Bump version --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index f1e4b6e39..9f9cee66f 100644 --- a/gradle.properties +++ b/gradle.properties @@ -4,7 +4,7 @@ packaging=jar description=Mixin (Fabric fork) url=https://fabricmc.net organization=FabricMC -buildVersion=0.15.0 +buildVersion=0.15.1 upstreamMixinVersion=0.8.7 buildType=RELEASE asmVersion=9.6 From 2307a333b0cf1ae360e9f35ccf6db78e740c5f25 Mon Sep 17 00:00:00 2001 From: LlamaLad7 Date: Thu, 15 Aug 2024 11:51:36 +0100 Subject: [PATCH 4/9] Revert "Fix: Fix `NEW` descriptor gating." This reverts commit 74a1696d993ce782609874c7d8cbfaf271c31c02. --- .../asm/mixin/injection/points/BeforeNew.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java b/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java index 8e8b13d3e..14850b2d3 100644 --- a/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java +++ b/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java @@ -34,7 +34,6 @@ import org.objectweb.asm.tree.InsnList; import org.objectweb.asm.tree.MethodInsnNode; import org.objectweb.asm.tree.TypeInsnNode; -import org.spongepowered.asm.mixin.FabricUtil; import org.spongepowered.asm.mixin.injection.InjectionPoint; import org.spongepowered.asm.mixin.injection.InjectionPoint.AtCode; import org.spongepowered.asm.mixin.injection.selectors.ITargetSelector; @@ -107,11 +106,6 @@ public class BeforeNew extends InjectionPoint { */ private final int ordinal; - /** - * Fabric: Whether to ignore the supplied descriptor in keeping with 0.8.5 and below. - */ - private final boolean ignoreDesc; - public BeforeNew(InjectionPointData data) { super(data); @@ -124,8 +118,7 @@ public BeforeNew(InjectionPointData data) { } ITargetSelectorConstructor targetSelector = (ITargetSelectorConstructor)member; this.target = targetSelector.toCtorType(); - this.desc = targetSelector.toCtorDesc(); - this.ignoreDesc = FabricUtil.getCompatibility(data.getContext()) < FabricUtil.COMPATIBILITY_0_14_0; + this.desc = org.spongepowered.asm.mixin.FabricUtil.getCompatibility(data.getContext()) >= org.spongepowered.asm.mixin.FabricUtil.COMPATIBILITY_0_14_0 ? targetSelector.toCtorDesc() : null; } /** @@ -139,7 +132,7 @@ public boolean hasDescriptor() { * Gets the descriptor from the injection point, can return null */ public String getDescriptor() { - return this.ignoreDesc ? null : this.desc; + return this.desc; } @SuppressWarnings("unchecked") @@ -166,7 +159,7 @@ public boolean find(String desc, InsnList insns, Collection no if (this.desc != null) { for (TypeInsnNode newNode : newNodes) { - if (BeforeNew.findInitNodeFor(insns, newNode, this.getDescriptor()) != null) { + if (BeforeNew.findInitNodeFor(insns, newNode, this.desc) != null) { nodes.add(newNode); found = true; } From 4254da4433b03d58209b49810434e99977461539 Mon Sep 17 00:00:00 2001 From: LlamaLad7 Date: Thu, 15 Aug 2024 11:55:59 +0100 Subject: [PATCH 5/9] Revert "Compat: Gate `NEW` descriptor filtering." --- .../org/spongepowered/asm/mixin/injection/points/BeforeNew.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java b/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java index 14850b2d3..845d6e781 100644 --- a/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java +++ b/src/main/java/org/spongepowered/asm/mixin/injection/points/BeforeNew.java @@ -118,7 +118,7 @@ public BeforeNew(InjectionPointData data) { } ITargetSelectorConstructor targetSelector = (ITargetSelectorConstructor)member; this.target = targetSelector.toCtorType(); - this.desc = org.spongepowered.asm.mixin.FabricUtil.getCompatibility(data.getContext()) >= org.spongepowered.asm.mixin.FabricUtil.COMPATIBILITY_0_14_0 ? targetSelector.toCtorDesc() : null; + this.desc = targetSelector.toCtorDesc(); } /** From 023e39334850e839c283be413257bf459f40a5d6 Mon Sep 17 00:00:00 2001 From: LlamaLad7 Date: Thu, 15 Aug 2024 12:18:01 +0100 Subject: [PATCH 6/9] Build: Bump version. --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 9f9cee66f..4a4130468 100644 --- a/gradle.properties +++ b/gradle.properties @@ -4,7 +4,7 @@ packaging=jar description=Mixin (Fabric fork) url=https://fabricmc.net organization=FabricMC -buildVersion=0.15.1 +buildVersion=0.15.2 upstreamMixinVersion=0.8.7 buildType=RELEASE asmVersion=9.6 From adcd9526d2b045b88ba727b05d44ff39febdc985 Mon Sep 17 00:00:00 2001 From: LlamaLad7 Date: Mon, 26 Aug 2024 22:08:58 +0100 Subject: [PATCH 7/9] Fix: Rewrite argMap allocation algorithm. The old one did not properly account for discontinuities in the list, leading to double-width types clobbering meaningful locals and causing mysterious VerifyErrors. --- .../asm/mixin/injection/struct/Target.java | 69 +++++++++++-------- 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/src/main/java/org/spongepowered/asm/mixin/injection/struct/Target.java b/src/main/java/org/spongepowered/asm/mixin/injection/struct/Target.java index 912b833e5..87f44b565 100644 --- a/src/main/java/org/spongepowered/asm/mixin/injection/struct/Target.java +++ b/src/main/java/org/spongepowered/asm/mixin/injection/struct/Target.java @@ -44,6 +44,7 @@ import org.spongepowered.asm.mixin.transformer.ClassInfo; import org.spongepowered.asm.util.Bytecode; import org.spongepowered.asm.util.Constants; +import org.spongepowered.asm.util.Counter; import org.spongepowered.asm.util.Locals.SyntheticLocalVariableNode; /** @@ -460,12 +461,17 @@ public int[] generateArgMap(Type[] args, int start, boolean fresh) { if (this.argMapVars == null) { this.argMapVars = new ArrayList(); } - + int[] argMap = new int[args.length]; - for (int arg = start, index = 0; arg < args.length; arg++) { + Counter index = new Counter(); + for (int arg = start; arg < args.length; arg++) { int size = args[arg].getSize(); - argMap[arg] = fresh ? this.allocateLocals(size) : this.allocateArgMapLocal(index, size); - index += size; + if (fresh) { + argMap[arg] = this.allocateLocals(size); + index.value += size; + } else { + argMap[arg] = this.allocateArgMapLocal(index, size); + } } return argMap; } @@ -481,35 +487,40 @@ public int[] generateArgMap(Type[] args, int start, boolean fresh) { * @param size Size of variable * @return local offset to use */ - private int allocateArgMapLocal(int index, int size) { - // Allocate extra space if we've reached the end - if (index >= this.argMapVars.size()) { - int base = this.allocateLocals(size); - for (int offset = 0; offset < size; offset++) { - this.argMapVars.add(Integer.valueOf(base + offset)); + private int allocateArgMapLocal(Counter index, int size) { + boolean isLastSlotFree = index.value < this.argMapVars.size(); + while (index.value < this.argMapVars.size()) { + int local = this.argMapVars.get(index.value); + if (size == 1) { + index.value++; + return local; } - return base; - } - - int local = this.argMapVars.get(index); - - // Allocate extra space if the variable is oversize - if (size > 1 && index + size > this.argMapVars.size()) { - int nextLocal = this.allocateLocals(1); - if (nextLocal == local + 1) { - // Next allocated was contiguous, so we can continue - this.argMapVars.add(Integer.valueOf(nextLocal)); + int nextIndex = index.value + 1; + if (nextIndex < this.argMapVars.size() && local + 1 == this.argMapVars.get(nextIndex)) { + // We own the next slot so this is a safe place to store a double-width type. + // We've consumed the next slot so increment the next available index. + index.value += 2; return local; } - - // Next allocated was not contiguous, allocate a new local slot so - // that indexes are contiguous - this.argMapVars.set(index, Integer.valueOf(nextLocal)); - this.argMapVars.add(Integer.valueOf(this.allocateLocals(1))); - return nextLocal; + index.value++; } - - return local; + // We don't have anywhere to store the arg. + int newLocal = this.allocateLocal(); + this.argMapVars.add(newLocal); + index.value++; + if (size == 1) { + return newLocal; + } + if (isLastSlotFree && newLocal == this.argMapVars.get(this.argMapVars.size() - 2) + 1) { + // We own the preceding local as well, and it is not yet used by this argMap, so we can store our + // double-width type there. + return newLocal - 1; + } + // Allocate 1 more local so we can fit the double-width type. We know it will follow directly from the + // previous one. + this.argMapVars.add(this.allocateLocal()); + index.value++; + return newLocal; } /** From c3e6d660027c5410cd5ddc41c698b82f4de233ae Mon Sep 17 00:00:00 2001 From: LlamaLad7 Date: Tue, 3 Sep 2024 09:12:09 +0100 Subject: [PATCH 8/9] Fix: Fix interface injector visibility checks. (#156) `InjectionInfo.parse` can be very expensive and in the case of some MixinExtras features is not pure. The correct approach would be to use `InjectionInfo.getInjectorAnnotation`, but the override is no longer needed anyway since we only support Java 8+ so Mixin will already make injector methods private and synthetic. Except, Mixin does it based on the current `COMPATIBILITY_LEVEL`, which is wrong, because we could be on JAVA_17 and yet a class compiled with JAVA_8 had no choice but to use public methods. The original implementation was also incomplete because it forgot about default methods. --- .../asm/mixin/MixinEnvironment.java | 12 +++++++++++- .../transformer/MixinApplicatorInterface.java | 16 ---------------- .../asm/mixin/transformer/MixinInfo.java | 4 ++++ .../transformer/MixinPreProcessorInterface.java | 7 ++++--- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/spongepowered/asm/mixin/MixinEnvironment.java b/src/main/java/org/spongepowered/asm/mixin/MixinEnvironment.java index 3c9633971..17c1151e0 100644 --- a/src/main/java/org/spongepowered/asm/mixin/MixinEnvironment.java +++ b/src/main/java/org/spongepowered/asm/mixin/MixinEnvironment.java @@ -1080,7 +1080,17 @@ static String getSupportedVersions() { } return sb.toString(); } - + + public static CompatibilityLevel forClassVersion(int version) { + CompatibilityLevel latest = null; + for (CompatibilityLevel level : values()) { + if (level.getClassVersion() >= version) { + return level; + } + latest = level; + } + return latest; + } } /** diff --git a/src/main/java/org/spongepowered/asm/mixin/transformer/MixinApplicatorInterface.java b/src/main/java/org/spongepowered/asm/mixin/transformer/MixinApplicatorInterface.java index 99aec8db3..830736fff 100644 --- a/src/main/java/org/spongepowered/asm/mixin/transformer/MixinApplicatorInterface.java +++ b/src/main/java/org/spongepowered/asm/mixin/transformer/MixinApplicatorInterface.java @@ -24,12 +24,10 @@ */ package org.spongepowered.asm.mixin.transformer; -import java.lang.reflect.Modifier; import java.util.Map.Entry; import org.objectweb.asm.tree.FieldNode; import org.objectweb.asm.tree.MethodNode; -import org.spongepowered.asm.mixin.MixinEnvironment; import org.spongepowered.asm.mixin.MixinEnvironment.Feature; import org.spongepowered.asm.mixin.injection.struct.InjectionInfo; import org.spongepowered.asm.mixin.injection.throwables.InvalidInjectionException; @@ -37,7 +35,6 @@ import org.spongepowered.asm.mixin.transformer.throwables.InvalidInterfaceMixinException; import org.spongepowered.asm.util.Annotations; import org.spongepowered.asm.util.Constants; -import org.spongepowered.asm.util.LanguageFeatures; /** * Applicator for interface mixins, mainly just disables things which aren't @@ -166,17 +163,4 @@ protected void applyInjections(MixinTargetContext mixin, int injectorOrder) { return; } } - - @Override - protected void checkMethodVisibility(MixinTargetContext mixin, MethodNode mixinMethod) { - //Allow injecting into static interface methods where it isn't possible to control the access of the injection method - if (Modifier.isStatic(mixinMethod.access) && !MixinEnvironment.getCompatibilityLevel().supports(LanguageFeatures.PRIVATE_METHODS_IN_INTERFACES)) { - InjectionInfo injectInfo = InjectionInfo.parse(mixin, mixinMethod); - if (injectInfo != null) { - return; - } - } - - super.checkMethodVisibility(mixin, mixinMethod); - } } diff --git a/src/main/java/org/spongepowered/asm/mixin/transformer/MixinInfo.java b/src/main/java/org/spongepowered/asm/mixin/transformer/MixinInfo.java index 980bcd26a..41701876b 100644 --- a/src/main/java/org/spongepowered/asm/mixin/transformer/MixinInfo.java +++ b/src/main/java/org/spongepowered/asm/mixin/transformer/MixinInfo.java @@ -1268,6 +1268,10 @@ List getTargets() { Set getInterfaces() { return this.getState().getInterfaces(); } + + int getClassVersion() { + return this.getState().getClassNode().version; + } /** * Get transformer extensions diff --git a/src/main/java/org/spongepowered/asm/mixin/transformer/MixinPreProcessorInterface.java b/src/main/java/org/spongepowered/asm/mixin/transformer/MixinPreProcessorInterface.java index a01aa57fd..ae17ed222 100644 --- a/src/main/java/org/spongepowered/asm/mixin/transformer/MixinPreProcessorInterface.java +++ b/src/main/java/org/spongepowered/asm/mixin/transformer/MixinPreProcessorInterface.java @@ -108,10 +108,11 @@ protected void prepareMethod(MixinMethodNode mixinMethod, Method method) { method, this.mixin)); } - // Make injectors private synthetic if the current runtime supports it + CompatibilityLevel classLevel = CompatibilityLevel.forClassVersion(mixin.getClassVersion()); + // Make injectors private synthetic if the current class version supports it if (isPublic - && !currentLevel.supports(LanguageFeatures.PRIVATE_METHODS_IN_INTERFACES) - && currentLevel.supports(LanguageFeatures.PRIVATE_SYNTHETIC_METHODS_IN_INTERFACES)) { + && !classLevel.supports(LanguageFeatures.PRIVATE_METHODS_IN_INTERFACES) + && classLevel.supports(LanguageFeatures.PRIVATE_SYNTHETIC_METHODS_IN_INTERFACES)) { Bytecode.setVisibility(mixinMethod, Bytecode.Visibility.PRIVATE); mixinMethod.access |= Opcodes.ACC_SYNTHETIC; } From c0676344c40c52dbe1bf6dd1624ae3dd5103b390 Mon Sep 17 00:00:00 2001 From: modmuss Date: Tue, 3 Sep 2024 09:12:51 +0100 Subject: [PATCH 9/9] Bump version --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 4a4130468..bc5e6fa65 100644 --- a/gradle.properties +++ b/gradle.properties @@ -4,7 +4,7 @@ packaging=jar description=Mixin (Fabric fork) url=https://fabricmc.net organization=FabricMC -buildVersion=0.15.2 +buildVersion=0.15.3 upstreamMixinVersion=0.8.7 buildType=RELEASE asmVersion=9.6