Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetMethodFromStackframe fix (and .gitignore update) #598

Closed

Conversation

kohanis
Copy link
Contributor

@kohanis kohanis commented Mar 7, 2024

No description provided.

@pardeike
Copy link
Owner

Why are the rather large changes to .gitignore necessary?

@kohanis
Copy link
Contributor Author

kohanis commented Mar 16, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

kohanis commented Mar 22, 2024

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
Copy link
Owner

pardeike commented Mar 22, 2024

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
Copy link
Owner

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.

@pardeike pardeike closed this Mar 25, 2024
@kohanis kohanis deleted the fix/get-method-from-stackframe branch March 25, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants