Closed saki7 closed 6 years ago
pardeike
commented
6 years ago Very interesting. As you said, this is a new feature and I’ve not tested it on all cases. It’s also a good case for me to try different approaches when generating IL code for it.
I’ll take a look and see if I can make ref work as one would expect it to work like that. One thing: you said that the error disappeared when you removed the ref but does it work as expected?
saki7
commented
6 years ago Yes, it does work as expected.I visually see the difference ingame.
pardeike
commented
6 years ago I get back to my dev machine soon. I suspect that the IL code can be designed I two different ways. The second one requiring ref and thus giving you the ability to write to the field even though it is a value type. I guess I change the IL code generator in Harmony so that value types always require ref - or even better, adapt the IL code so you can use ref or not.
saki7
commented
6 years ago I think I found a clue to this issue. I have managed to get the error message from Harmony:
System.FormatException: Method CameraController.HandleKeyEvents(System.Single) cannot be patched.
Reason: Invalid IL code in (wrapper dynamic-method) CameraController:HandleKeyEvents_Patch1
(object,single): IL_0305: stfld 0x000000bbHere, the original method:
private void HandleKeyEvents(float multiplier) { /* ... */ }Patcher:
static void Postfix(
CameraController __instance,
float multiplier,
ref Vector3 ___m_velocity, // <-- ref here
// ...The log's method is different from the one I posted initially, but I'm doing the same thing: private field injection parameter with ref + triple underscores.
This time the modification (i.e. re-assignment of new value) is apparently not working, because the param is a value-type (float).
Previous example was working because the param type is UnityStandardAssets.ImageEffects.DepthOfField, which is a class type.
I've grepped Harmony repo and found few locations for the stfld op:
I also found an active issue with similar description: #79. However I think the issue is not relevant because I have my ref patches working (as you pointed out in #79) for non-private-field-injected params.
@pardeike Does this ring a bell?
p.s. I have also confirmed that the patch won't work (throws invalid IL error) for both Prefix and Postfix patches
saki7
commented
6 years ago @pardeike I have submitted a pr. I'm seeing my patches working ingame, finally. Can you confirm the fix?
BTW: the Unity GC thingy was irrelevant. I think we should change the issue title to something like: Patch failure on ref-qualified private field
pardeike
commented
6 years ago I’ll have a look.
pardeike
commented
6 years ago So, I fired up LINQPad and played around with different scenarios. And clearly, I must have been out of my mind when I wrote the code dealing with private field injection. There is no reason to do it with local variables. Instead, this syntax is much more appealing and simpler:
public class SomeClass
{
private float someFloat;
private Class someInstance;
private Struct someStruct;
public void DoSomething(int n)
{
someInstance = new Class();
someStruct = new Struct();
PatchClass.Patch1(someFloat);
PatchClass.Patch2(ref someFloat);
PatchClass.Patch3(someInstance);
PatchClass.Patch4(ref someInstance);
PatchClass.Patch5(someStruct);
PatchClass.Patch6(ref someStruct);
}
}which generates the following IL code:
SomeClass.DoSomething:
IL_0000: ldarg.0
IL_0001: newobj UserQuery+Class..ctor
IL_0006: stfld UserQuery+SomeClass.someInstance
IL_000B: ldarg.0
IL_000C: ldflda UserQuery+SomeClass.someStruct
IL_0011: initobj UserQuery.Struct
IL_0017: ldarg.0
IL_0018: ldfld UserQuery+SomeClass.someFloat
IL_001D: call UserQuery+PatchClass.Patch1
IL_0022: ldarg.0
IL_0023: ldflda UserQuery+SomeClass.someFloat
IL_0028: call UserQuery+PatchClass.Patch2
IL_002D: ldarg.0
IL_002E: ldfld UserQuery+SomeClass.someInstance
IL_0033: call UserQuery+PatchClass.Patch3
IL_0038: ldarg.0
IL_0039: ldflda UserQuery+SomeClass.someInstance
IL_003E: call UserQuery+PatchClass.Patch4
IL_0043: ldarg.0
IL_0044: ldfld UserQuery+SomeClass.someStruct
IL_0049: call UserQuery+PatchClass.Patch5
IL_004E: ldarg.0
IL_004F: ldflda UserQuery+SomeClass.someStruct
IL_0054: call UserQuery+PatchClass.Patch6
IL_0059: retSo my proposal is to not accept your pull request and instead simply make Harmony emit:
| You write | Harmony emits |
|---|---|
aType ___someFloat |
LDARG.0 LDFLD <field> |
ref aType ___someFloat |
LDARG.0, LDFLDA <field> |
In my tests, I tested that it is the same IL for all types: float, class, struct. There might be an issue for specific .NET version if the struct is larger than 8 bytes it swaps the stack order but I deal with that later (one issue already filed).
saki7
commented
6 years ago They both work. I think there's a misunderstanding.
ldfldldarg.0 <-- load <this>
ldfld #N <field_src> <-- load Nth field
stfld <field_dst> <-- store value to <this>.<field_dst>| Context | <this> |
<field_src> |
operation means... |
|---|---|---|---|
| Target class | Target instance | Target field | instance.field = value |
| Harmony patch | Target instance | Harmony's Nth field (?) | instance.<invalid field> = value |
IL log
... [snip] ...
L_0007: Local var 10: UnityEngine.Vector3
... [snip] ...
L_005a: call Boolean Prefix(CameraController, Single, Vector3 ByRef, Vector2 ByRef, Single ByRef, ColossalFramework.SavedInputKey, ColossalFramework.SavedBool, ColossalFramework.SavedBool, ColossalFramework.SavedFloat, ColossalFramework.SavedFloat)
L_005f: ldarg.0
L_0060: ldfld 10 (UnityEngine.Vector3)
L_0062: stfld UnityEngine.Vector3 m_velocityldlocldarg.0 <-- load <this>
ldloc #N <local> <-- load Nth local variable
stfld <field> <-- store value to <this>.<field>| Context | <this> |
<local> |
stfld results in... |
|---|---|---|---|
| Inside target | Target instance | Target's Nth local var | N/A |
| After Harmony call | Target instance | Harmony's Nth local var | instance.field = value |
IL log
... [snip] ...
L_0007: Local var 10: UnityEngine.Vector3
... [snip] ...
L_005a: call Boolean Prefix(CameraController, Single, Vector3 ByRef, Vector2 ByRef, Single ByRef, ColossalFramework.SavedInputKey, ColossalFramework.SavedBool, ColossalFramework.SavedBool, ColossalFramework.SavedFloat, ColossalFramework.SavedFloat)
L_005f: ldarg.0
L_0060: ldloc 10 (UnityEngine.Vector3)
L_0062: stfld UnityEngine.Vector3 m_velocityI believe the op code should be ldloc. Harmony is creating the private field injection temporary privateFieldVar as a local variable (LocalBuilder). So we should treat it as locals. I think ldfld is wrong here but the interpreter is falling back somehow. Those two op codes both work in my game application.
The choice between <op> and <op>a should depend on how you build your temporaries. The current implementation is creating a local value type so I think the <op>a will result in dangling reference.
If you want to directly detour the assignment within the patcher, I think you have to peek into the patcher IL and redirect all assignment target to the original class field.
I think the order matters. I've never heard of any VM swapping stack order for optimization.
pardeike
commented
6 years ago Latest commit uses no local variables anymore and thus is not only more efficient but also more elegant.
saki7
commented
6 years ago It's working. Thanks for the fix. I think the untouched ref parameters are not throwing exceptions anymore. They used to produce invalid memory access.
I wrote a Harmony patcher for my Cities: Skylines mod, with the new private field injection feature (which is introduced in d66ae5710a35e8fd7177f5da65ff0cc34227fc01.
I got errors when accessing a Unity object which might be destroyed at some moment of the injection. First of all, here's the stacktrace from the game:
*
無効なアドレスにアクセスしようとしています。= "attempted to access the invalid address" in my system localeNote the function signature:
The param is passed by reference. It is a private field instance of the Unity class
UnityStandardAssets.ImageEffects.DepthOfField, which could be destroyed by the Unity GC when it gets disabled.At first I was retreiving this parameter as a
ref+ triple-underscored param in the patch method and got the access violation error. When I changed the same param to non-reftype, the error disappeared. The type is a class, and is not a value type (such as struct). My question is:refqualifier? Is it something to do with the reference-counting system in Mono GC?p.s. the private field injection is so cool. I love it so much.