Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix A/V in ApplyUpdate API #50452

Merged
merged 2 commits into from
Apr 1, 2021
Merged

Fix A/V in ApplyUpdate API #50452

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
894ce16
Fix A/V in ApplyUpdate API
mikem8361 Mar 30, 2021
7178568
Remove thread suspension; switch to ResetCodeEntryPoint()
mikem8361 Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/coreclr/vm/encee.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,13 @@ HRESULT EditAndContinueModule::UpdateMethod(MethodDesc *pMethod)
// to the Method's code must be to the call/jmp blob immediately in front of the
// MethodDesc itself. See MethodDesc::IsEnCMethod()
//
pMethod->Reset();
pMethod->ResetCodeEntryPoint();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return S_OK;
}
Expand Down