search

pardeike / Harmony

A library for patching, replacing and decorating .NET and Mono methods during runtime
https://www.patreon.com/pardeike
MIT License
5.15k stars 485 forks source link

GetMethodFromStackframe fix (and .gitignore update) #598

Closed kohanis closed 5 months ago

pardeike commented 5 months ago

Why are the rather large changes to .gitignore necessary?

kohanis commented 5 months ago

They are not, it's just latest template from dotnet cli. It at least adds files for JetBrains IDEs (I mostly use Rider for .net). I can remove it // I just moved all repo-specific statements to the top. Everything below is the result of dotnet new gitignore

pardeike commented 5 months ago

So I played around with the change a bit but I am confused. What is the purpose of this change. Submitting a PR with no context usually only works for trivial changes but this areas is unexpectedly complicated due to how Mono works and the interactions around stacktraces. The risk of breaking things is high.

kohanis commented 5 months ago

Fair Currently there are: Harmony.GetOriginalMethod -> HarmonySharedState.GetOriginal Harmony.GetMethodFromStackframe -> HarmonySharedState.FindReplacement -> HarmonySharedState.GetOriginal

The problem was that PlatformTriple.Current.GetIdentifiable was only called from Harmony.GetOriginalMethod, so Harmony.GetMethodFromStackframe never used it and often couldn't find the replacement. So I moved it from Harmony.GetOriginalMethod to HarmonySharedState.GetOriginal.

Also, and I only noticed this now, HarmonySharedState.FindReplacement actually returns the original and not the replacement, is this intentional? Because then Harmony.GetMethodFromStackframe actually returns the original, not the replacement as in docs, and Harmony.GetOriginalMethodFromStackframe will always fail, basically. As far as I understand it, this is a bug, and then original test makes sense, not my version

// Basically, my question is what is the point of Harmony.GetMethodFromStackframe(StackFrame) and HarmonySharedState.FindReplacement(StackFrame) if, by docs

For normal frames, <c>frame.GetMethod()</c> is returned. For frames containing patched methods, the replacement method is returned or <c>null</c> if no method can be found

it should be exactly StackFrame.GetMethod() in any case

pardeike commented 5 months ago

I started reviewing your PR and created this branch. I think I cleaned up what is confusing. You can have a look at my changes - they explain it better than I could with words: https://github.com/pardeike/Harmony/tree/fix/get-method-from-stackframe

Note the complexity that supporting Mono infers. A stacktrace there does not contain any MethodInfo if the frame is a dynamic method (a replacement).

pardeike commented 5 months ago

Sorry, I am going to reject this PR mainly because my own branch has a more clean design and fixes things that this does not address.