Problem with patching virtual method that returns UnityEngine.Color

[boformer]

Describe the bug I got various kinds of errors while trying to patch a certain method with 2.0.0.9.

The same patch works with 1.2.0.1.

One very suspicious thing is that in the harmony.log IL code of the replacement method, there seem to be 27 local vars defined (index 0 to 26).

In the harmony-generated IL after // end original, there are statements like hat reference local var with index 27 (doesn't exist?????):

Edit: I checked again and it seems var with index 27 is just not listed because the list is generated while the original method IL is printed. So I'm pretty clueless what is happening...

I don't really get why you need var 27 when you already have var 25 which stores exactly the same value

IL_07C1: Label0
IL_07C1: ldloc      25 (UnityEngine.Color)
IL_07C5: ldloca     27 (UnityEngine.Color)
IL_07C9: initobj    UnityEngine.Color
IL_07CF: stloc      27 (UnityEngine.Color)
IL_07D3: ldarg.1
IL_07D4: ldloc      27 (UnityEngine.Color)
IL_07D8: stobj      UnityEngine.Color
IL_07DD: ret

To Reproduce Steps to reproduce the behavior:

1. The original method and its signature and class:

// public class CommonBuildingAI : BuildingAI
public override Color GetColor(ushort buildingID, ref Building data, InfoManager.InfoMode infoMode)

2. The patch (applied with PatchAll):

    [HarmonyPatch]
    public static class CommonBuildingAI_GetColor {
        public static MethodBase TargetMethod() {

            return typeof(CommonBuildingAI).GetMethod(
                "GetColor",
                BindingFlags.Instance | BindingFlags.Public,
                null,
                new[] { typeof(ushort), typeof(Building).MakeByRefType(), typeof(InfoManager.InfoMode) },
                new ParameterModifier[0]);
        }

        public static bool Prefix(CommonBuildingAI __instance, ushort buildingID, ref Building data, InfoManager.InfoMode infoMode, ref Color __result) {
            __result = Color.red;
            return false;
        }
    }

3. The output of the Harmony debug log

harmony.log.txt

4. The stacktrace or other errors

Pretty random errors, mostly NullRefExceptions that occur outside or within the patched method, or "attempted to access invalid address":

========== OUTPUTING STACK TRACE ==================

0x0000000004FD1B57 (Mono JIT Code) (wrapper dynamic-method) CommonBuildingAI:DMD<DMD<GetColor_Patch1>?1297982592::GetColor_Patch1> (CommonBuildingAI,intptr,uint16,Building&,InfoManager/InfoMode)
0x0000000070915A70 (Mono JIT Code) PlayerBuildingAI:GetColor (uint16,Building&,InfoManager/InfoMode)
0x00000000708E933A (Mono JIT Code) DepotAI:GetColor (uint16,Building&,InfoManager/InfoMode)
0x000000005492DF47 (Mono JIT Code) BuildingManager:UpdateColorMap (UnityEngine.Texture2D)
0x000000005492DBFB (Mono JIT Code) RenderManager:UpdateColorMap ()
0x000000001A5EC8BD (Mono JIT Code) RenderManager:LateUpdate ()
0x00000000064B050B (Mono JIT Code) (wrapper runtime-invoke) object:runtime_invoke_void__this__ (object,intptr,intptr,intptr)
0x00007FFA70E45CDF (mono) mono_set_defaults
0x00007FFA70D98499 (mono) mono_runtime_invoke
  ERROR: SymGetSymFromAddr64, GetLastError: 'Es wurde versucht, auf eine unzulässige Adresse zuzugreifen.' (Address: 00007FF6BF9E4954)
...
0x00007FF6BFDC6A0C (Cities) 
0x00007FFAC2837BD4 (KERNEL32) BaseThreadInitThunk
0x00007FFAC34ACED1 (ntdll) RtlUserThreadStart

========== END OF STACKTRACE ===========

**** Crash! ****

Runtime environment (please complete the following information):

• OS: Windows 10, 64bit
• .NET version 3.5
• Harmony version 2.0.0.9
  • Name of game or host application: Cities Skylines

[boformer]

After looking a bit more, I found out what causes the issue:

StructReturnBuffer.NeedsFix returns true, but it should return false for a method that returns UnityEngine.Color!

I've hacked the method to return false for type UnityEngine.Color by adding 16 to the specialSizes field:

var specialSizesField = typeof(Harmony).Assembly.GetType("HarmonyLib.StructReturnBuffer").GetField("specialSizes", BindingFlags.Static | BindingFlags.NonPublic);
var specialSizes = (HashSet<int>)specialSizesField.GetValue(null);
specialSizes.Add(16);

When applying the patch after this, the game no longer crashes and all buildings become red as expected.

I don't know why no struct return buffer is used in this particular case though...

[pardeike]

Then why does this test (or its static variant) not fail: https://github.com/pardeike/Harmony/blob/3e5c707020e7ff66cb539e9f2c6aee17c35a18bc/HarmonyTests/Patching/Assets/ReturningStructMethods.cs#L106

[pcfantasy]

@boformer I use harmony 2.0.0.9 to patch CommonBuildingAI.GetColor. it seems work well.

https://github.com/pcfantasy/RealCity/blob/master/Patch/CommonBuildingAIGetColorPatch.cs

[boformer]

@pardeike Idk. Maybe that Unity version uses an outdated and custom version of mono that does some special optimizations for Color structs? or maybe it has something to do with virtual/override methods?

@pcfantasy why does it say using Harmony; then?

[boformer]

@pardeike I did some further tests with interesting results. In Cities: Skylines environment, it seems that only static methods need the struct return buffer fix, instance methods are not "optimized" in that way. This affects all methods where sizeof(returnType) == 16. I didn't test any other sizes.

Test Code:

var harmony = new Harmony("boformer.Harmony2Example");
harmony.Patch(typeof(MyClass).GetMethod("GetSt16Static"), null, new HarmonyMethod(typeof(Patches).GetMethod("GetSt16_Postfix")));
harmony.Patch(typeof(MyClass).GetMethod("GetColorStatic"), null, new HarmonyMethod(typeof(Patches).GetMethod("GetColor_Postfix")));
harmony.Patch(typeof(MyClass).GetMethod("GetSt16"), null, new HarmonyMethod(typeof(Patches).GetMethod("GetSt16_Postfix")));
harmony.Patch(typeof(MyClass).GetMethod("GetColor"), null, new HarmonyMethod(typeof(Patches).GetMethod("GetColor_Postfix")));

var st16static = MyClass.GetSt16Static();
UnityEngine.Debug.Log("st16static.b8: " + st16static.b8);

var colorStatic = MyClass.GetColorStatic();
UnityEngine.Debug.Log("colorStatic.g: " + colorStatic.g);
UnityEngine.Debug.Log(colorStatic.ToString());

var myClass = new MyClass();

var st16 = myClass.GetSt16();
UnityEngine.Debug.Log("st16.b8: " + st16.b8);

var color = myClass.GetColor();
UnityEngine.Debug.Log("color.g: " + color.g);
UnityEngine.Debug.Log(color.ToString());

public static class Patches {
    public static void GetSt16_Postfix(ref St16 __result) {
        UnityEngine.Debug.Log("GetSt16__Postfix __result.b8: " + __result.b8);
    }

    public static void GetColor_Postfix(ref UnityEngine.Color __result) {
        UnityEngine.Debug.Log("GetColor__Postfix: __result.g: " + __result.g);
    }
}

public class MyClass {
    public static St16 GetSt16Static() {
        UnityEngine.Debug.Log("GetSt16Static");
        return new St16 { b8 = 42 };
    }

    public static UnityEngine.Color GetColorStatic() {
        UnityEngine.Debug.Log("GetColorStatic");
        return new UnityEngine.Color(1f, 0.75f, 0.5f, 0.25f);
    }

    public St16 GetSt16() {
        UnityEngine.Debug.Log("GetSt16");
        return new St16 { b8 = 42 };
    }

    public UnityEngine.Color GetColor() {
        UnityEngine.Debug.Log("GetColor");
        return new UnityEngine.Color(1f, 0.75f, 0.5f, 0.25f);
    }
}

public struct St16 {
    public byte b1;
    public byte b2;
    public byte b3;
    public byte b4;
    public byte b5;
    public byte b6;
    public byte b7;
    public byte b8;
    public byte b9;
    public byte b10;
    public byte b11;
    public byte b12;
    public byte b13;
    public byte b14;
    public byte b15;
    public byte b16;
}

Unity Log:

Static methods are patched correctly by Harmony 2.0.0.9:

GetSt16Static
GetSt16__Postfix __result.b8: 42
st16static.b8: 42

GetColorStatic
GetColor__Postfix: __result.g: 0.75
colorStatic.g: 0.75
RGBA(1.000, 0.750, 0.500, 0.250)

The instance method results get scrambled by Harmony 2.0.0.9:

GetSt16
GetSt16__Postfix __result.b8: 42
st16.b8: 0

GetColor
GetColor__Postfix: __result.g: 0.75
color.g: 0
RGBA(0.000, 0.000, 0.000, 0.000)

harmony.log.txt

[boformer]

Then why does this test (or its static variant) not fail:

I investigated this as well. I modified Test_Patch_Returning_Structs a bit so I can run it ingame: All the tests are passing, but only because you are not actually checking the returned values (just if the type matches)

[pcfantasy]

@boformer @pardeike

Sorry，I forgot to switch to my harmony2.0 branch.

Yes, I can confirm that harmony2.0.0.9 can patch unity color now, but function is wrong, the same situation as boformer said.

[boformer]

I wonder if it only affects Cities' mono version or all of them. We could of course build a custom version of Harmony that has the correct behavior.

[pcfantasy]

I wonder if it only affects Cities' mono version or all of them. We could of course build a custom version of Harmony that has the correct behavior.

If you build a local version of Harmony named harmony 2.0.0.10. Other modder use harmony 2.0.0.9. Two mods patch the same method with prefix. Does harmony priority still work for two mods? I mean the highest priority still excute first.

[boformer]

If you build a local version of Harmony named harmony 2.0.0.10. Other modder use harmony 2.0.0.9. Two mods patch the same method with prefix. Does harmony priority still work for two mods? I mean the highest priority still excute first.

My central Harmony mod makes sure that all mods use the one shipped with the mod. (it patches the assembly resolver of the game)

I will probably keep 2.0.0.9 version number

[pardeike]

This needs to be verified with master. I will close this issue without more feedback.

[boformer]

Sorry for the inactivity on the issue.

By now the Harmony mod for Cities: Skylines uses a custom Harmony fork because the official Harmony package is no longer compatible with the outdated mono runtime of the game.

If I remember correctly, this issue was fixed by adding an additional condition to StructReturnBufferCheck.NeedsFix in the fork:

if (AccessTools.IsMonoRuntime && method.IsStatic is false) return false;

https://github.com/boformer/Harmony/blob/e156ea3cf02a41d94de3ab027d404b83293b7d57/Harmony/Internal/StructReturnBufferCheck.cs#L104

I don't really have the time or energy to finish my pull request #280 at this point (I would probably have to redo it from scratch), but if I remember correctly the tests show that the error described in this issue occurs in various mono runtimes, not only in the outdated runtime used by CSL.