Bugfix attempt for mismatched jump labels by IlProcessor #77
Conversation
Keep original leave jump labels as given by original IL. Clean-up unnecessary double leave op-codes (from IlProcessor). Addresses GitHub Issue BepInEx#65
mgreter
changed the title
|
Is there anything else I could provide to get this/a fix over the merge line? |
|
I did a quick IL compare of this and the release version, and it looks like this version changes where the last leave call of a catch jumps after running a transpiler. In some cases it's I think harmless (this PR on left, latest release on right): In other cases it's not so harmless (this PR on left, latest release on right, transpiler used): Looks like I can't merge this since it would result in a potentially quite bad regression (the above issue happened in multiple patches). |
|
Thanks for taking a look, although a bit hard to reproduce your "regression" on my own here, so I had to go by the screenshots. And if I'm not wrong it doesn't really look like it would regress (maybe it's even doing the proper thing now). Edit. also see that the try/catch now actually jumps to different parts in the code, which may or may not be more reasonable to expect!? Your second screen will jump right after the "end handler (catch)". There it will load the "stfld lastLoadErrorCode" again, doesn't it? IMO it seems it was doing the exact same code thing in the catch clause too and put the result into "stloc.s V_8". My patch on the other hand will jump to the label where it will load the value it has put before into "ldloc.s V_8" again. After that both parts seem to jump then to the same label again "br IL_033f". Did you actually try/test the resulting executable if any regression actually show when the patched code is run? I couldn't get it to produce any regressions on my tests, but as I said, I can only test a limited amount, although at some point I did just blindly add empty transpiler patches to all kind of methods, and with my patch applied it never failed AFAIR. Anyway, thx again for looking into this issue! Can you maybe compare it to IL code before any patch was applied? Another argument for that it is now more correct is that nothing else seems to use the jump label at "0338", but there was one to begin with. So original code probably had some jumps to that position, but where are they after patching with your latest release? FWIW I see one possible regression with your test patches and my fix here. If you insert code at a position that has jump labels attached, you will need to make sure to move the labels too, if you want to insert new opcodes to run before the original code. I kinda can see how the "fix" I replaced in this PR could maybe been there to "address" this potential issue!? var labels = codes[i].labels;
codes[i].labels = null;
codes.InsertRange(i, new[]{ ... });
codes[i].labels = labels; |
|
You are right, it appears like the original IL jumps like the fixed version rather than the release version. This is the original IL for the second example I posted: I do remember some issues with labels in the past, but I think it was a different issue where a new label object was instantiated incorrectly. I'll have to check for differences in different Unity versions of the game code and patch I posted. |
|
I did find a regression, one of very old patches now throws. This is under Unity 5.6.2 with dynamicmethod backend: with cecil backend: The same patch runs fine on Unity 2019.4.9 with cecil backend, but original IL appears to be slightly different. Source of the patch in question (PH == false) |
|
That will be quite hard to debug without the original function to reproduce it, but I can try :) |
|
I can help you repro this if you DM me on discord. |
|
Bump, does anyone want to try fixing this? |
|
Do we have any updates on this / if it's production ready? |
The issue I've hit was not fixed, I didn't hear from @mgreter after his last message here. |
|
Did a test, but unfortunately that patch seems to regress even cases that didn't before here :-/ |
|
I think I at least solved the current regression also against my own previous fix. @@ -162,10 +162,12 @@ public static class OpaqueTextures
IEnumerable<CodeInstruction> instructions)
{
var codes = new List<CodeInstruction>(instructions);
- // Execute our static function to do our thing
- codes.Insert(codes.Count - 2, CodeInstruction.Call(
- typeof(OpaqueTextures), "InitOpaqueConfig"));
- // Return new IL codes
+ var lbls = codes[codes.Count - 2].labels;
+ codes[codes.Count - 2].labels = null;
+ var instr = CodeInstruction.Call(
+ typeof(OpaqueTextures), "InitOpaqueConfig");
+ codes.Insert(codes.Count - 2, instr);
+ codes[codes.Count - 3].labels = lbls;
return codes;
}
}Moving the existing labels seems to fix my new regression. |
|
Possibly, let me know whenever you have something to test. I can also send you the setup to reproduce my issue if you want. |
Do we happen to have any updates on this? |
Not from me. It does change the original behavior AFAICT, as I now need to make sure to move the label if needed. But that seems to be ok/correct. E.g. there is a semantic difference when injecting some code at a certain point, as in: should original code jump to the beginning of the new code or after it. |
|
It's not mismatched jump by IlProcessor, it's actually correct from it point of view, we removed correct What I meant was why would we even remove second jump if first one is going to prevent second one from happening anyway? Let it be by completely deleting `Case 1' // This would be a breaking change anyway, regardless of the implementation of the fix. Though I doubt if anyone has ever intentionally used it. I don't see any reliable way to determine if the jump was originally from block or not, given the transpiler's capabilities |
|
Did anyone else test this PR for regressions in other games? I've only found a single patch that started failing, so it's not a deal breaker if it does fix other issues. Especially since this harmony update would not be used in bep5 so would not immediately affect the patch in question. |
|
I'd still recommend keeping it simple: @@ -346,11 +346,7 @@ internal class ILManipulator
cur.blocks.ForEach(b => il.MarkBlockBefore(b));
// We need to handle exception handler opcodes specially because ILProcessor emits them automatically
- // Case 1: leave + start or end of exception block => ILProcessor generates leave automatically
- if ((cur.opcode == SRE.OpCodes.Leave || cur.opcode == SRE.OpCodes.Leave_S) &&
- (cur.blocks.Count > 0 || next?.blocks.Count > 0))
- goto mark_block;
- // Case 2: endfilter/endfinally and end of exception marker => ILProcessor will generate the correct end
+ // endfilter/endfinally and end of exception marker => ILProcessor will generate the correct end
if ((cur.opcode == SRE.OpCodes.Endfilter || cur.opcode == SRE.OpCodes.Endfinally) && cur.blocks.Count > 0)
goto mark_block;
// Other cases are either intentional leave or invalid IL => let them be processed and let JIT generate correct exceptionYes, there will be two |
|
Related #96 |
This is a potential BugFix for #65, although I don't really know the implications.
Please check and give feedback and I can adjust it accordingly if needed.
I certainly only tested it with my specific use case!
Addresses GitHub Issue #65
Hope it's okay to add the dll if others want to test if this fix also works for them: 0Harmony.dll.zip
Note: I've had to up the net version to 48 as I don't have 45 on this PC ...