search

MonoMod / MonoMod.Common

Common code used by MonoMod and other .NET modding libraries. Not to be confused with MonoMod.Utils (for mods).
MIT License
74 stars 32 forks source link

Implement deep CustomDebugInformations copy #32

Closed Popax21 closed 1 year ago

Popax21 commented 1 year ago

Previously, AsyncMethodBodyDebugInformation and StateMachineScopeDebugInformation instances of cloned method bodys would still refer to the old method bodies, which could cause the PDB instruction offsets to desync. This change fixes this by explicitly cloning them along with the method body.

nike4613 commented 1 year ago

MonoMod.Common is being deprecated, of sorts. All of its functionality is being moved into the main MonoMod repo, where active development is, on the reorganize branch. This would likely be better targeted against that in the long term.

That aside, is this only used in the AOT patcher? AFAIK none of the DMD backends support debug information at all.

Also, it should be noted that there are (fairly abstract) plans to rewrite the AOT patcher from scratch. No actual work has been done, though I'm sure if you hop on the MonoMod Discord and ask about it, 0x0ade (and others) would be happy to talk about what should be/needs to be done around that.

Popax21 commented 1 year ago

MonoMod.Common is being deprecated, of sorts. All of its functionality is being moved into the main MonoMod repo, where active development is, on the reorganize branch. This would likely be better targeted against that in the long term.

That aside, is this only used in the AOT patcher? AFAIK none of the DMD backends support debug information at all.

Also, it should be noted that there are (fairly abstract) plans to rewrite the AOT patcher from scratch. No actual work has been done, though I'm sure if you hop on the MonoMod Discord and ask about it, 0x0ade (and others) would be happy to talk about what should be/needs to be done around that.

Actually, I've already discussed these PRs with jade - while she said that there won't be any new nuget builds, she agreed to merging this (+ a future accompanying PR on the main MonoMod repo) to get this + other related issues sorted out. I am aware that everything is moving over to the reorganize branch / rework, however Everest (the Celeste mod loader) still depends on the old legacy code, and it's unlikely that it will/can migrate over until the new code is stable on all platforms (biggest deal breaker so far is the incomplete Mac compatibility jade mentioned). By still merging this into the main repo Everest can just depend on MonoMod using a git submodule instead of having to maintain its own fork.

nike4613 commented 1 year ago

Sounds good. I'll let jade merge this then; it seems entirely fine.