Calls to static @Shadow methods do not get remapped in dev environment #431
Comments
|
Thanks for the detailed write up. I now understand why I was getting spurious reports of calls to undefined static accessor methods pertaining to fabric's fork. I will look into this. |
|
After throwing a breakpoint on this line in It seems like the only conditions where Not sure if it's really intensional that it gets to that point before it renaming, but for static fields at least it does so as a fix it is consistent with that at least. For reference the set of Mixins I ran with are here. It jumps a bit between the Mixin classes as an accident from trying to cover all the situations when |
Pretty sure that simply removing the check will incur the same issue that Fabric's workaround had, as discussed above (FabricMC#28). |
|
Perhaps it might, let's export the results with Starting with the interface @Mixin({ TestTarget.class })
public interface TestMixinA {
@Accessor
String getField();
@Accessor
void setField(final String p0);
@Invoker
String callViewField();
@Invoker
@MixinProxy(sessionId = "45e702f8-fb08-4973-a363-caa26a9bc918")
static /* synthetic */ void callLoad() {
load();
}
@Invoker("<init>")
@MixinProxy(sessionId = "45e702f8-fb08-4973-a363-caa26a9bc918")
static /* synthetic */ TestTarget make() {
return TestTarget.make_$md$9bc918$1();
}
}All looks well, let's contrast this to Fabric's 0.8.0 fork: @Mixin({ TestTarget.class })
public interface TestMixinA {
@Accessor
String getField();
@Accessor
void setField(final String p0);
@Invoker
String callViewField();
@Invoker
@MixinProxy(sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
static /* synthetic */ void callLoad() {
load();
}
@Invoker("<init>")
@MixinProxy(sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
static /* synthetic */ TestTarget make() {
return TestTarget.make_$md$a327a9$1();
}
}Essentially the same, allowing for mangling difference.
public abstract class TestParent {
public TestParent() {
super();
}
void squish() {
}
@Unique
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinZ", priority = 1000, sessionId = "45e702f8-fb08-4973-a363-caa26a9bc918")
protected void gain() {
System.out.println("Root gain");
}
}And Fabric's 0.8.0 fork: public abstract class TestParent {
public TestParent() {
super();
}
void squish() {
}
@Unique
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinZ", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
protected void gain() {
System.out.println("Root gain");
}
}Completely the same, not much to see here either. Now let's look at the real meat, the Mixin 0.8.1public class TestTarget extends TestParent implements TestMixinA, Iterable {
static boolean lock;
private String field;
public TestTarget() {
super();
}
String viewField() {
this.handler$zdg000$inject(new CallbackInfoReturnable("viewField", false));
return this.field;
}
public static void load() {
handler$zdh000$check(new CallbackInfo("load", false));
}
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
private void handler$zdg000$inject(final CallbackInfoReturnable call) {
shadow£$load();
}
@Unique
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
private void gain() {
this.iterator();
this.field = this.viewField();
}
@Overwrite
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
@Override
void squish() {
this.gain();
}
@MixinRenamed(originalName = "inter£$iterator", isInterfaceMember = true)
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
@Override
public Iterator iterator() {
return Collections.<Object>emptyIterator();
}
@Unique
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinC", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
protected void md59d823$gain$2() {
super.gain();
System.out.println("Child gain");
this.squish();
}
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinC", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
private static void handler$zdh000$check(final CallbackInfo call) {
if (TestTarget.lock) {
return;
}
System.out.println("Testing application");
TestTarget.lock = true;
TestMixinA.make().md59d823$gain$2();
TestTarget.lock = false;
System.out.println("Applied!");
}
@Overwrite
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinC", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
public static void go() {
TestMixinA.callLoad();
}
@Accessor(target = "field:Ljava/lang/String;")
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
@Override
public /* synthetic */ String getField() {
return this.field;
}
@Accessor(target = "field:Ljava/lang/String;")
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
@Override
public /* synthetic */ void setField(final String field) {
this.field = field;
}
@Invoker
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
@Override
public /* synthetic */ String callViewField() {
return this.viewField();
}
@Invoker
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
public static /* synthetic */ void callLoad_$md$59d823$0() {
load();
}
@Invoker("<init>")
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "75a46860-cda2-41dd-abc0-fada0259d823")
public static /* synthetic */ TestTarget make_$md$59d823$1() {
return new TestTarget();
}
}Fabric's 0.8.0 forkpublic class TestTarget extends TestParent implements TestMixinA, Iterable {
static boolean lock;
private String field;
public TestTarget() {
super();
}
String viewField() {
this.handler$zzi000$inject(new CallbackInfoReturnable("viewField", false));
return this.field;
}
public static void load() {
handler$zzj000$check(new CallbackInfo("load", false));
}
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
private void handler$zzi000$inject(final CallbackInfoReturnable call) {
load();
}
@Unique
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
private void gain() {
this.iterator();
this.field = this.viewField();
}
@Overwrite
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
@Override
void squish() {
this.gain();
}
@MixinRenamed(originalName = "inter£$iterator", isInterfaceMember = true)
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinB", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
@Override
public Iterator iterator() {
return Collections.<Object>emptyIterator();
}
@Unique
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinC", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
protected void mda327a9$gain$2() {
super.gain();
System.out.println("Child gain");
this.squish();
}
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinC", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
private static void handler$zzj000$check(final CallbackInfo call) {
if (TestTarget.lock) {
return;
}
System.out.println("Testing application");
TestTarget.lock = true;
TestMixinA.make_$md$a327a9$1().mda327a9$gain$2();
TestTarget.lock = false;
System.out.println("Applied!");
}
@Overwrite
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinC", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
public static void go() {
TestMixinA.callLoad_$md$a327a9$0();
}
@Accessor(target = "field:Ljava/lang/String;")
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
@Override
public /* synthetic */ String getField() {
return this.field;
}
@Accessor(target = "field:Ljava/lang/String;")
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
@Override
public /* synthetic */ void setField(final String field) {
this.field = field;
}
@Invoker
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
@Override
public /* synthetic */ String callViewField() {
return this.viewField();
}
@Invoker
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
public static /* synthetic */ void callLoad_$md$a327a9$0() {
load();
}
@Invoker("<init>")
@MixinMerged(mixin = "com.chocohead.test.mixin.TestMixinA", priority = 1000, sessionId = "7956230c-1a1c-4882-be6c-df13aba327a9")
public static /* synthetic */ TestTarget make_$md$a327a9$1() {
return new TestTarget();
}
}Now they're both pretty tall classes once Mixin is finished with them, so let's summarise the relevant changes to save playing spot-the-difference. private void handler$zdg000$inject(final CallbackInfoReturnable call) {
- shadow£$load();
+ load();
}
private static void handler$zdh000$check(final CallbackInfo call) {
if (TestTarget.lock) {
return;
}
System.out.println("Testing application");
TestTarget.lock = true;
- TestMixinA.make().md59d823$gain$2();
+ TestMixinA.make_$md$a327a9$1().mda327a9$gain$2();
TestTarget.lock = false;
System.out.println("Applied!");
}
public static void go() {
- TestMixinA.callLoad();
+ TestMixinA.callLoad_$md$a327a9$0();
}So now we can see the effects of Fabric's changes. Because it finds the shadow methods in Neither Mixin version successfully applies the set of Mixins properly, stock Mixin fails to fix the static shadow whilst Fabric's does the static shadow correctly but fails with the static invokers. The question then falls to why my suggested fix wouldn't do the same as Fabric's. The key difference is Fabric's tries to remap all static references whilst Potentially this means if you extended a Mixin then tried to use a static shadow in the child Mixin it would fail to remap too; it seems there are attempts to tape up similar sounding situations just below: Unfortunately as it stands these are insufficient, as they don't cover the case when |
* Experimental fix for SpongePowered#431
|
I've experienced the same problem and can verify the PR FabricMC#39 solve the problem. Should we move this PR upstream? |
|
I can confirm that this bug still occurs with fabric, with 'final' attributes that have pointers, like a List. |
If an obfuscated mod dependency contains a static
@Shadowmethod that gets unmapped in dev environment, then calls to it from other mixed in methods won't get remapped and will still reference the obfuscated name. The shadow itself will however apply correctly in the deobfuscated environment, causingNoSuchMethodErrors once some method tries to access it with the original obfuscated name.In the following I will present a quite related but simpler version of this general bug that relies on the
aliasesproperty of@Shadowin order to force some sort of remapping. Hence the following does not need an obfuscated dependency and in particular does not rely on external obfuscation. Nevertheless, the main motivation for this is still with obfuscated mod dependencies in mind.Consider the following mixin
which throws the following runtime error, showing that the call to
loadTexturesAsync_testindeed does not get remapped toloadTexturesAsyncwhereas the shadow itself does apply without complaints:Looking through the code there are two places where method bodies get remapped. The actual remapping of the shadow itself is done elsewhere (during shadow discovery) and attaches the necessary metadata to the shadow methods which is employed by these two remapping phases.
The first one is located in
org.spongepowered.asm.mixin.transformer.MixinPreProcessorStandardand is applied during the pre processing phase:As can be seen here,
owner.findMethodInHierarchy(methodNode, SearchType.ALL_CLASSES, ClassInfo.INCLUDE_PRIVATE);only includes private methods but not static methods. Hence this remapping phase will only remap calls to non-static shadows, but not to static ones, so in particular this does not handle the call to the static shadow in the example.The second remapping is loacted in
org.spongepowered.asm.mixin.transformer.MixinTargetContextand is applied during the post processing phase. It is responsible for re-targeting method calls to the mixin class and point them to the target class. But it also handles renames for synthetic and conformal methods, i.e., renames introduced by the mixin processor itself due to static@Accessors and@Uniques:In this case the condition
if (md != null && md.isRenamed() && md.getOriginalName().equals(methodRef.getName()) && (md.isSynthetic() || md.isConformed()))does not handle the call to the static shadow in the example either.So in the end, the reference to the static shadow will not be remapped by either method, hence the original obfuscated name is retained as seen in the exception.
There was an attempt to fix this on Fabric's mixin fork by including static methods in the pre processor remapping phase, see FabricMC@ec491e3. However, this caused issue FabricMC#28 and was reverted again lately. The issue with this naive approach is that there is a clash of concepts for accessor mixins containing static
@Accessors:getActiveProgram_$md$2f8dd9$0in the example of the bug report.getActiveProgramin the example.This is special to accessor mixins since the resulting interface can be loaded like a usual class, in contrast to all other mixins. The issue is now that these two concepts, i.e., the synthetic method in the resulting interface retaining its name and the conformal bridge method mixed into the target class, are internally represented by the same
MethodRefobject. So the naive approach of just adding static methods to the lookup had the issue that it did find this ambiguous methodgetActiveProgramand erronously renamed it to its conformal counterpartgetActiveProgram_$md$2f8dd9$0.One way to avoid this issue would be to include static methods in the lookup but explicitly exclude conformed methods, so in particular this conformal bridge method would be excluded. However, this would also cause calls to other conformed methods, i.e.,
@Uniquemethods, not to be renamed in this phase and defer the renaming to the post processor remapping phase. Note that in fact calls to static@Uniquemethods are currently already remapped in the post processor phase due to the same bug of not including static methods.Whether or not this would be desired I cannot tell, since I don't know the reasoning behind splitting the remapping into the two phases, so I don't know which kind of remapping should belong to which phase.
Another approach would be to somehow disambiguate the two concepts or mark the conformal counterpart of static accessors, so it could be explicitly excluded.
Furthermore, note that some similar scenario exists for static shadowed fields, as
MixinPreProcessorStandard.transformField(...)does not include static fields either. However, in this case its post processor counterpartMixinTargetContext.transformFieldRef(...)contains the clausewhich does handle these shadowed static fields, intentional or not. So shadowed static fields do actually not cause a
NoSuchFieldException.Finally, let me point out also another related issue. Note that the pre processor remapping
MixinPreProcessorStandard.transform(MixinTargetContext context)only processesMethodInsnNodeandFieldInsnNode, whereas the post processor remappingMixinTargetContext.transformMethod(MethodNode method)furthermore processesTypeInsnNode,LdcInsnNodeandInvokeDynamicInsnNode. So in particular the post processor remapping does process Java's method references, i.e.,Clas::method, whereas the pre processor remapping does not. Hence such method references to obfuscated shadows will not be unmapped, regardless of whether they are static or not, simply because they are not processed in the pre processor phase and they are not conformed methods, hence not handled in the post prcocessor phase either.This issue is in fact already reported as #406.
Best,
PhiPro
The text was updated successfully, but these errors were encountered: