Type parameter constraint in patched class breaks on newer Mono.Cecil version

Kalobi commented

2024-01-10 19:04:31 -06:00

(Migrated from github.com)

We encountered this issue while working on Everest, the modding API for the video game Celeste. Everest patches the game using MonoMod, which relies heavily on Mono.Cecil. We've been depending on Mono.Cecil version 0.11.4 so far. When changing the dependency to the latest version 0.11.5 (with no other changes to the project), in the patched UserIO class (using this patch), the type parameter constraint of the generic Save<T> method changes from where T : class (what it is in vanilla and should remain) to where T : class, Entity. Entity is a class that is used a lot in Celeste, but appears neither in the vanilla version of the UserIO class nor anywhere in our patch. Since the only change is the Mono.Cecil version, some kind of bug in Mono.Cecil introduced between versions 0.11.4 and 0.11.5 must be causing this type parameter constraint to bleed over from some earlier point where Mono.Cecil interacts with the Entity class.

We encountered this issue while working on [Everest](https://github.com/EverestAPI/Everest), the modding API for the video game Celeste. Everest patches the game using [MonoMod](https://github.com/MonoMod/MonoMod/), which relies heavily on Mono.Cecil. We've been depending on Mono.Cecil version 0.11.4 so far. When changing the dependency to the latest version 0.11.5 (with no other changes to the project), in the patched UserIO class (using [this patch](https://github.com/EverestAPI/Everest/blob/dev/Celeste.Mod.mm/Patches/UserIO.cs)), the type parameter constraint of the generic `Save<T>` method changes from `where T : class` (what it is in vanilla and should remain) to `where T : class, Entity`. `Entity` is a class that is used a lot in Celeste, but appears neither in the vanilla version of the `UserIO` class nor anywhere in our patch. Since the only change is the Mono.Cecil version, some kind of bug in Mono.Cecil introduced between versions 0.11.4 and 0.11.5 must be causing this type parameter constraint to bleed over from some earlier point where Mono.Cecil interacts with the `Entity` class.

jbevain commented

2024-01-10 19:09:21 -06:00

(Migrated from github.com)

Could you extract the offending code in a small dll that we could write a test around? Thanks!

Kalobi commented

2024-01-10 19:31:58 -06:00

(Migrated from github.com)

I will look into it, but unfortunately this is quite a complex project and I'm not sure I'll be able to craft a small standalone version that exhibits this bug.

jbevain commented

2024-01-11 15:47:07 -06:00

(Migrated from github.com)

That would however provide the best chances at this being diagnosed and fixed :)

Kalobi commented

2024-01-12 16:45:53 -06:00

(Migrated from github.com)

I did manage to use git bisect to determine that the issue is introduced by commit c4cfe16845.

I did manage to use git bisect to determine that the issue is introduced by commit c4cfe168450ad7c14d881c1388e981564a06eab7.

Kalobi commented

2024-01-12 16:55:20 -06:00

(Migrated from github.com)

Specifically, readding the metadata.RemoveGenericConstraintMapping (generic_parameter); line to MetadataReader.ReadGenericConstraints (and readding that method of course) fixes the issue.

Specifically, readding the `metadata.RemoveGenericConstraintMapping (generic_parameter);` line to `MetadataReader.ReadGenericConstraints` (and readding that method of course) fixes the issue.

RedFlames commented

2024-03-15 17:12:57 -05:00

(Migrated from github.com)

So, I've found something, or at least how to trigger this or make it go away.

Some of it is part of MonoMod though. Also I'm very much at my wits end, but I'll just write down what it is that I found.

So, Everest's installer patches Celeste.exe with MonoMod.

Within MonoMod you at some point end up in MonoMod.MonoModder.PatchRefsInType(TypeDefinition type) where you got this

            foreach (MethodDefinition method in type.Methods)
                PatchRefsInMethod(method);

And then in PatchRefsInMethod(MethodDefinition method) when on the Celeste.UserIO type's method which looks like this inside Celeste

public static bool Save<T>(string path, byte[] data) where T : class
{
[...]
}

if you now simply just call the getter of method.CustomDebugInformations or even just method.HasCustomDebugInformations -- you will now end up with a new Constraint, specifically I've been checking method.GenericParameters[0].Constraints.Count

But also the extra fun thing is if you touch the getter of Constraints on that first generic parameter first, it will also be fine and not get the extra constraint.

The part within MonoModder that triggers it actually looks like this, but you only need to "get" method.CustomDebugInformations, the collection it return is empty on this method, so this loop is irrelevant.

            foreach (CustomDebugInformation dbgInfo in method.CustomDebugInformations)
                if (dbgInfo is AsyncMethodBodyDebugInformation asyncDbgInfo)
                    for (var i = 0; i < asyncDbgInfo.ResumeMethods.Count; i++)
                        asyncDbgInfo.ResumeMethods[i] = ((MethodReference) asyncDbgInfo.ResumeMethods[i].Relink(Relinker, method)).Resolve();

I just tried to see if I could trigger this without MonoMod, by just putting this in place of the MonoModder execution:

                if (module.Name == "Celeste.exe") {

                    var method = module.Types.First(t => t.FullName.Contains("Celeste.UserIO")).Methods.First(m => m.FullName.Contains("Save("));

                    // if I comment this out, I should somehow get "1" on the second log statement, otherwise "0" on both...
                    //logVerboseCb($"[UserIOLogger] Count = {method.GenericParameters[0].Constraints.Count}");

                    foreach (var cdi in method.CustomDebugInformations)
                        logVerboseCb($"{cdi}");

                    logVerboseCb($"[UserIOLogger] Count = {method.GenericParameters[0].Constraints.Count}");

                }

But it doesn't want to trigger the bug at all. Must be something else that MonoMod is doing.

The place in MonoMod is here
github.com/MonoMod/MonoMod@878928553e/src/MonoMod.Patcher/MonoModder.cs (L1949)

although I was working inside Everest where it subclasses MonoModder and I copied the base implementation of void PatchRefsInMethod(MethodDefinition method) into an override and worked there because that was easier than (re-)building MonoMod all the time. 😅

github.com/EverestAPI/Everest@464dea2c34/NETCoreifier/NetFrameworkModder.cs (L13)

I suppose I could try chipping away at MonoMod to actually reproduce the bug without MonoMod... anyways this doesn't make any sense :)

PS: What even is Mono.Cecil.Mixin.Read

So, I've found something, or at least how to trigger this or make it go away. Some of it is part of MonoMod though. Also I'm very much at my wits end, but I'll just write down what it is that I found. So, Everest's installer patches Celeste.exe with MonoMod. Within MonoMod you at some point end up in `MonoMod.MonoModder.PatchRefsInType(TypeDefinition type)` where you got this ```cs foreach (MethodDefinition method in type.Methods) PatchRefsInMethod(method); ``` And then in `PatchRefsInMethod(MethodDefinition method)` when on the `Celeste.UserIO` type's method which looks like this inside Celeste ```cs public static bool Save<T>(string path, byte[] data) where T : class { [...] } ``` if you now simply just call the getter of `method.CustomDebugInformations` or even just `method.HasCustomDebugInformations` -- you will now end up with a new Constraint, specifically I've been checking `method.GenericParameters[0].Constraints.Count` **But** also the extra fun thing is if you touch the getter of **Constraints** on that first generic parameter first, it will also be fine and not get the extra constraint. The part within MonoModder that triggers it actually looks like this, but you only need to "get" `method.CustomDebugInformations`, the collection it return is empty on this method, so this loop is irrelevant. ```cs foreach (CustomDebugInformation dbgInfo in method.CustomDebugInformations) if (dbgInfo is AsyncMethodBodyDebugInformation asyncDbgInfo) for (var i = 0; i < asyncDbgInfo.ResumeMethods.Count; i++) asyncDbgInfo.ResumeMethods[i] = ((MethodReference) asyncDbgInfo.ResumeMethods[i].Relink(Relinker, method)).Resolve(); ``` --- I just tried to see if I could trigger this without MonoMod, by just putting this in place of the MonoModder execution: ```cs if (module.Name == "Celeste.exe") { var method = module.Types.First(t => t.FullName.Contains("Celeste.UserIO")).Methods.First(m => m.FullName.Contains("Save(")); // if I comment this out, I should somehow get "1" on the second log statement, otherwise "0" on both... //logVerboseCb($"[UserIOLogger] Count = {method.GenericParameters[0].Constraints.Count}"); foreach (var cdi in method.CustomDebugInformations) logVerboseCb($"{cdi}"); logVerboseCb($"[UserIOLogger] Count = {method.GenericParameters[0].Constraints.Count}"); } ``` But it doesn't want to trigger the bug at all. Must be something else that MonoMod is doing. The place in MonoMod is here https://github.com/MonoMod/MonoMod/blob/878928553ef775bd7814ae5d208771f0d4ec29be/src/MonoMod.Patcher/MonoModder.cs#L1949 although I was working inside Everest where it subclasses `MonoModder` and I copied the base implementation of `void PatchRefsInMethod(MethodDefinition method)` into an override and worked there because that was easier than (re-)building MonoMod all the time. 😅 https://github.com/EverestAPI/Everest/blob/464dea2c34aac90d0cbb4d7c733c88e2f6bf9d68/NETCoreifier/NetFrameworkModder.cs#L13 --- I suppose I could try chipping away at MonoMod to actually reproduce the bug without MonoMod... anyways this doesn't make any sense :) PS: What even is `Mono.Cecil.Mixin.Read`

Kalobi commented

2024-07-09 16:08:50 -05:00

(Migrated from github.com)

Closing this in favor of the better-informed #953

🎉 1

Rows
Columns

Type parameter constraint in patched class breaks on newer Mono.Cecil version #931