Fix A/V in ApplyUpdate API

[mikem8361]

Add thread suspend around the update API.

Issue: #50445

[dotnet-issue-labeler]

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

[jkotas]

I do not think that's the proper fix. Instead, I think we should patch the precode automically same was as tiered JIT. Tiered JIT is able to do that without thread suspend, so this one should be able to do it without thread suspend as well.

[jkotas]

LGTM. Thanks! We may want to open an issue to track switching this to the code version manager.

[stephentoub]

Thanks for tracking this down, Mike.

[noahfalk]

There is probably still theoretically a race here that would result in the code not being updated. In practice I could imagine it is non-existent. The flow would look like:

Thread A: calls into unjitted method Foo for the first time and begins jitting using the original IL
Thread B: starts applying an Update, sets new dynamic IL for the method, resets code entrypoint, sets native code slot to NULL
Thread A: finishes jitting, sets the native code slot to the jitted method body for the original IL, backpatches the precode.
Thread B: exits the update, sometime later calls Foo(). The original IL runs because that is what A backpatched it to.

[jkotas]

Agree. There may be also potential crashes where the code out there is not expecting native code to go from non-NULL to NULL.

[jkotas]

@noahfalk Do you believe that it is a good idea to switch EnC to use the code version manager to address this type of issues?

[noahfalk]

EnC using code version manager is a nice long term direction - it is what I always hoped we get to. In terms of doing it now vs. do it later... its probably at least worth investigating now. The cheap way to implement it would be for the debugger to share the same API that the profiler uses, SetActiveILCodeVersions. Currently the profiler creates new versions with no IL and then uses a callback later to populate the IL just-in-time, but I'd expect it is fairly simple to add a new overload that accepts the IL up-front and stores it skipping the callback.

[mikem8361]

I'm not sure changing ENC to use the code version manager can fit into our current 6.0 schedule. I'm already overbooked.

/cc: @tommcdon

[noahfalk]

This certainly looks better than the AV. There is probably still a theoretical race but potentially so rare its not clear it is worth fixing.

[jkotas]

Failures are known issue #50520

[mikem8361]

We might have jumped the gun a little in merging this PR because under a checked build we hit this assert:

runtime/src/coreclr/vm/method.cpp

Line 5012 in 5e79b4f

_ASSERTE(IsVersionable());

The reason I didn't hit before is I was only running on release builds because the repro runs everything (dotnet-watch, building the diffs, and the test app) on the same runtime. The "debug" builds were way too slow (took 5 minutes or longer to even get the test app started). I finally tried it on a checked build after Noah's comment that ENC edited methods will return IsVersionable() == false.

[jkotas]

I think you can avoid that assert by calling GetPrecode()->ResetTargetInterlocked(); directly from encee.cpp