Sporadic AV while using ApplyUpdate

[stephentoub]

Repro:
RayTracerRepro.zip

1. Ensure you have a recent nightly build installed (https:/dotnet/installer/blob/main/README.md#installers-and-binaries)... I have 6.0.100-preview.4.21179.9.
2. Unzip this project (an old parallel loops demo app).
3. dotnet watch
4. Click the parallel check box (it repros without it, but not as fast or consistently)
5. Click the Start button. The ball should start bouncing.
6. notepad Raytracer.cs
7. Edit L139 (return new Color(0, 0, 0);) to change one or more of the arguments to be a double value between 0 and 1 (e.g. return new Color(0, 0, .5);. Ctrl-S to save.
8. Repeat step 7, changing the values and saving, until this is printed out by dotnet watch:

watch : Hot reload of changes succeeded.
watch : Exited with error code -1073741819
watch : Waiting for a file to change before restarting dotnet...

The AV ends up coming from the patched method:

OS Thread Id: 0x5e54
Child SP IP Call Site
000000306157D1F8 00007FFE81620D04 Microsoft.ParallelComputingPlatform.ParallelExtensions.Samples.RayTracer.TraceRay(Microsoft.ParallelComputingPlatform.ParallelExtensions.Samples.Ray, Microsoft.ParallelComputingPlatform.ParallelExtensions.Samples.Scene, Int32)
000000306157FC00 00007ffe814a2d7d [DebuggerU2MCatchHandlerFrame: 000000306157fc00]

cc: @mikem8361, @tommcdon

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Repro:
RayTracerRepro.zip

1. Ensure you have a recent nightly build installed (https:/dotnet/installer/blob/main/README.md#installers-and-binaries)... I have 6.0.100-preview.4.21179.9.
2. Unzip this project (an old parallel loops demo app).
3. dotnet watch
4. Click the parallel check box (it repros without it, but not as fast or consistently)
5. Click the Start button. The ball should start bouncing.
6. notepad Raytracer.cs
7. Edit L139 (return new Color(0, 0, 0);) to change one or more of the arguments to be a double value between 0 and 1 (e.g. return new Color(0, 0, .5);. Ctrl-S to save.
8. Repeat step 7, changing the values and saving, until this is printed out by dotnet watch:

watch : Hot reload of changes succeeded.
watch : Exited with error code -1073741819
watch : Waiting for a file to change before restarting dotnet...

The AV ends up coming from the patched method:

OS Thread Id: 0x5e54
Child SP IP Call Site
000000306157D1F8 00007FFE81620D04 Microsoft.ParallelComputingPlatform.ParallelExtensions.Samples.RayTracer.TraceRay(Microsoft.ParallelComputingPlatform.ParallelExtensions.Samples.Ray, Microsoft.ParallelComputingPlatform.ParallelExtensions.Samples.Scene, Int32)
000000306157FC00 00007ffe814a2d7d [DebuggerU2MCatchHandlerFrame: 000000306157fc00]

cc: @mikem8361, @tommcdon

Author:	stephentoub
Assignees:	-
Labels:	area-Diagnostics-coreclr, untriaged
Milestone:	6.0.0

[jkotas]

I guess pMethod->Reset() in encee.cpp needs to be pMethod->ResetCodeEntryPoint() so that the reset is done atomically.

Also, delete MethodDesc::Reset if it is not used anywhere else.

[mikem8361]

There is one other use: dynamicmethod.cpp line 267:

    // Reset the method desc into pristine state

    // Note: Reset has THROWS contract since it may allocate jump stub. It will never throw here
    // since it will always reuse the existing jump stub.
    pNewMD->Reset();

[mikem8361]

With ResetCodeEntryPoint(), the new method with the changes doesn't seem to be used. In Stephen's demo, the background colors don't change.

[mikem8361]

Adding ThreadSuspend::SuspendEE around the ApplyEditAndContinue call in ApplyUpdate seems to fix the issue or at least makes it rare. After 35 changes, it hasn't crashed with this change. Before the change, it crashed with 3 or 4 updates.

[jkotas]

ResetCodeEntryPoint

Have you tried to step through this method to see why it does not work?

ThreadSuspend::SuspendEE around the ApplyEditAndContinue

Yes, that will make it harder to reproduce, but I do not think it would be the proper fix.

[mikem8361]

I'm working on stepping through ResetCodeEntryPoint but my repro using dotnet-watch make that difficult (it keeps kill off the target process I'm trying to debug). I have a very rough sketch of the calls being made in this method:

ResetCodeEntryPoint
     GetPrecode()->ResetTargetInterlocked();
           case PRECODE_FIXUP:
                 AsFixupPrecode()->ResetTargetInterlocked();

I don't know this part of the runtime (obviously) very well so I not sure what is suppose to happen. The other uses of MethodDesc::ResetCodeEntryPoint seem to do more than just call this method.

[jkotas]

ResetTargetInterlocked should do the same thing as Reset, except that it should do it atomically. Reset updates the precode via multiple writes. ResetTargetInterlocked updates the precode using a single InterlockedCompareExchange. The final outcome should be identical in both cases.

[mikem8361]

I stepped though both pMethod->Reset (works but crashes) and pMethod->ResetCodeEntryPoint (doesn't work) one of the differences is that in Reset this code executed:

    if (pMethod->HasNativeCodeSlot())
    {
        RelativePointer<TADDR> *pRelPtr = (RelativePointer<TADDR> *)pMethod->GetAddrOfNativeCodeSlot();
        pRelPtr->SetValueMaybeNull(NULL);
    }

Adding that after the call to ResetCodeEntryPoint causes the updated methods to be executed. It probably isn't correct either because it isn't interlocked.

UpdateMethod()
...
    pMethod->ResetCodeEntryPoint();
    if (pMethod->HasNativeCodeSlot())
    {
        RelativePointer<TADDR> *pRelPtr = (RelativePointer<TADDR> *)pMethod->GetAddrOfNativeCodeSlot();
        pRelPtr->SetValueMaybeNull(NULL);
    }

I don't understand all the details here, but could it be that ApplyUpdate is changing the method via ENC (which I think doesn't use the code version manager) and tier compilation (which does)? And yes stepping through FixupPrecode::ResetTargetInterlocked() and FixupPrecode::Init() (called from Reset()) confirm they do the same thing but it doesn't seem to be enough.

/cc @noahfalk because of his work on tier compilation.

[jkotas]

Adding that after the call to ResetCodeEntryPoint causes the updated methods to be executed.

I would be ok with it as interim fix.

changing the method via ENC (which I think doesn't use the code version manager)

I guess that it is something we need to fix to make this robust and avoid intermittent crashes similar to this one.

[noahfalk]

I don't understand all the details here, but could it be that ApplyUpdate is changing the method via ENC (which I think doesn't use the code version manager) and tier compilation (which does)?

Yes, I expect that is exactly what you are seeing. Calling ResetEntryPoint() is directing the next call through the entrypoint to go into the PreStub. Once in there if the method has IsVersionable() == true then the PreStub would identify which version is now active and backpatch the precode with a pointer to that code. EnC won't take that path. I can't recall if there is a shortcut it might take by observing the NativeCodeSlot already has a valid code pointer in it, but even if it does go through all the work of jitting the updated IL, that new code will never be used because SetNativeCodeInterlocked isn't going to replace a non-null pre-existing code pointer.

[mikem8361]

I'm going to leave this issue open to address the checked build assert at

runtime/src/coreclr/vm/method.cpp

Line 5012 in 5e79b4f

_ASSERTE(IsVersionable());

and maybe the theoretical race condition Noah mentioned.