Open tmat opened 1 month ago
@lambdageek ptal. Would be nice to get fixed for .NET 9, if not too difficult.
@tmat what's allowed? changing toward more visibility? or is public
->private
also legal? mono pretty intentionally assumes that there are no changes to any MONO_TABLE_FIELD
entries (only additions to a class that is being defined in the current update are allowed):
I can't really tell without trying it, but this seems fairly straightforward (although I can't tell how risky it might be for .NET 9):
replace the assertion by an if
; allow the table update if it's a modification; then trigger mono_field_resolve_type
for the modified field (assuming the parent class has already been inited) and allow it to overwrite the field's MonoType with an updated one with updated attributes.
@tmat what happens if an SRE method was JITed with a public
version of the field but now it's become private. What is supposed to tell the JIT to make a new version with an updated accessibility check?
attn @tommcdon
changing toward more visibility? or is public->private also legal?
Any visibility changes would be allowed.
@tmat what happens if an SRE method was JITed with a public version of the field but now it's become private. What is supposed to tell the JIT to make a new version with an updated accessibility check?
Not sure I understand the scenario. What does SRE mean? System.Reflection.Emit? There shouldn't be a need for any extra accessibility checks or notifications. The JITed code should be able to access the member until the code is recompiled (the referring method is updated) regardless of whether or not the accessibility of the referred member changes after the JIT checks the visibility.
@tmat what happens if an SRE method was JITed with a public version of the field but now it's become private. What is supposed to tell the JIT to make a new version with an updated accessibility check?
Not sure I understand the scenario. What does SRE mean? System.Reflection.Emit? There shouldn't be a need for any extra accessibility checks or notifications. The JITed code should be able to access the member until the code is recompiled (the referring method is updated) regardless of whether or not the accessibility of the referred member changes after the JIT checks the visibility.
I'm thinking of a scenario like this:
using System.Reflection;
using System.Reflection.Emit;
public class C {
/*private*/ public int X; /// EnC: change this
public void Increment()
{
X++;
}
}
public class Program
{
public static void Main()
{
var c = new C();
var f1 = GenerateGetter();
while (true)
{
c.Increment();
Foo(f1, c); // Q: this should always succeed?
var f2 = GenerateGetter();
Foo(f2, c); // Q: after edit, this should throw FieldAccessException?
System.Threading.Thread.Sleep(1000);
}
}
public static void Foo(Func<C,int> func, C c)
{
int i = func(c);
Console.WriteLine ($"i = {i}");
}
public static Func<C,int> GenerateGetter()
{
var fi = typeof(C).GetField("X", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public);
var an = new AssemblyName("x1");
var ab = AssemblyBuilder.DefineDynamicAssembly(an, AssemblyBuilderAccess.RunAndCollect);
var modb = ab.DefineDynamicModule(an.FullName);
var tb = modb.DefineType("GetterHolder", TypeAttributes.Public);
var methodName = "GetX";
var mb = tb.DefineMethod(methodName, MethodAttributes.Public | MethodAttributes.Static, typeof(int), [ typeof(C) ]);
var ilg = mb.GetILGenerator();
ilg.Emit(OpCodes.Ldarg_0);
ilg.Emit(OpCodes.Ldfld, fi);
ilg.Emit(OpCodes.Ret);
var ti = tb.CreateType();
var mi = ti.GetMethod (methodName, BindingFlags.Public | BindingFlags.Static);
return (Func<C,int>)Delegate.CreateDelegate(typeof(Func<C,int>), mi);
}
}
Suppose you run this app and then change C.X
from public
to private
. Is the expected behavior that f1
will still be able to access X
? but f2
will now begin throwing FieldAccessException
?
I think so, as long as f1
is jitted before the change is applied. The CLR behaves that way.
Thanks. In that case, I think the change I summarized in https://github.com/dotnet/runtime/issues/108097#issuecomment-2368344901 should be sufficient
Just so I'm clear, are any of the 3 things that might happen what is supposed to happen when EnC can't continue?
If Roslyn sends invalid update to the runtime the behavior is undefined. It's Roslyn's responsibility to block rude edits. That said, as much as possible a better failure than crashing VS or debugger would be certainly desirable.
Ideally, the runtime would send message to the debugger that EnC/Hot Reload failed and an error code.
However, I can see that it might be hard to do so if an issue is found in the middle of updating various data structures (i.e. rolling back partial updates).
@tmat Do we also need to support changing method/property/event/nested class accessibility? or is this just about fields?
@lambdageek All members and types.
Description
Mono throws when member accessibility is changed during debugging
Reproduction Steps
Use Roslyn build from PR: https://github.com/dotnet/roslyn/pull/75191 for repro.
1) Create Blazor WASM app 2) Replace
@code
block inCounter.razor
with:3) Place breakpoint to
IncrementCount
and F5. 4) Click "Increment" button and uncomment commented code after hitting the breakpoint. 5) StepExpected behavior
Assignment to
x
is successfully executed.Actual behavior
One of the following: 1) The Blazor app crashes and disconnects:
2) VS crashes, 3) VS debugger shows exception in JavaScript:
Regression?
No
Known Workarounds
No response
Configuration
No response
Other information
No response