Add support for async thunk methods in GetNativeCodeInfo#124354
Add support for async thunk methods in GetNativeCodeInfo#124354tommcdon wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix an issue where DacDbiInterfaceImpl::GetNativeCodeInfo incorrectly returns data for async thunk methods instead of the actual async implementation method. The runtime caches async thunks in the methoddef map (see clsload.cpp:2804-2810), causing FindLoadedMethodRefOrDef to return the thunk. This PR adds logic to detect async thunk methods and attempts to find the corresponding async implementation method by iterating through all methods in the type.
Changes:
- Added logic to detect async thunk methods using
IsAsyncThunkMethod() - Implemented a loop to iterate through all introduced methods in the method table
- Filters methods using
IsDiagnosticsHidden()to find the actual async implementation method
| MethodTable * pMT = pMethodDesc->GetMethodTable(); | ||
| MethodTable::IntroducedMethodIterator it(pMT); | ||
| for (; it.IsValid(); it.Next()) | ||
| { | ||
| MethodDesc * pMD = it.GetMethodDesc(); | ||
| CONSISTENCY_CHECK(pMD != NULL && pMD->GetMethodTable() == pMT); | ||
| if (!pMD->IsDiagnosticsHidden()) | ||
| { | ||
| pMethodDesc = pMD; | ||
| break; | ||
| } |
There was a problem hiding this comment.
The loop that searches for a non-hidden method does not handle the case where no non-hidden method is found. If all methods in the type are diagnostics-hidden, the loop will exit without finding a match, and pMethodDesc will remain pointing to the async thunk method. This defeats the purpose of the fix.
Consider using GetAsyncOtherVariant() or GetAsyncVariantNoCreate() methods (defined in method.hpp) which directly return the async variant method associated with a thunk, rather than iterating through all methods. These methods are specifically designed for this purpose and are used elsewhere in the codebase (e.g., stubmgr.cpp:2293, method.cpp:3305, reflectioninvocation.cpp:1298).
| MethodTable * pMT = pMethodDesc->GetMethodTable(); | |
| MethodTable::IntroducedMethodIterator it(pMT); | |
| for (; it.IsValid(); it.Next()) | |
| { | |
| MethodDesc * pMD = it.GetMethodDesc(); | |
| CONSISTENCY_CHECK(pMD != NULL && pMD->GetMethodTable() == pMT); | |
| if (!pMD->IsDiagnosticsHidden()) | |
| { | |
| pMethodDesc = pMD; | |
| break; | |
| } | |
| MethodDesc * pAsyncVariant = pMethodDesc->GetAsyncOtherVariant(); | |
| if (pAsyncVariant != NULL && !pAsyncVariant->IsDiagnosticsHidden()) | |
| { | |
| pMethodDesc = pAsyncVariant; |
| { | ||
| MethodDesc * pMD = it.GetMethodDesc(); | ||
| CONSISTENCY_CHECK(pMD != NULL && pMD->GetMethodTable() == pMT); | ||
| if (!pMD->IsDiagnosticsHidden()) |
There was a problem hiding this comment.
Do we also want to make sure the mdToken is the same as functionToken?
As I understand this, it would get the first !IsDiagnosticsHidden() method on the methodtable, not necessarily the non thunk variant.
| MethodDesc* pMethodDesc = FindLoadedMethodRefOrDef(pModule, functionToken); | ||
| if (pMethodDesc->IsAsyncThunkMethod()) | ||
| { | ||
| MethodTable * pMT = pMethodDesc->GetMethodTable(); |
There was a problem hiding this comment.
Would this work? All the code below seems like it is trying to emulate the behavior of GetAsyncVariant.
if (PMethodDesc != NULL && pMethodDesc->IsAsyncThunkMethod())
{
pMethodDesc = pMethodDesc->GetAsyncVariantNoCreate()
}
There was a problem hiding this comment.
The MethodDesc::GetAsyncOtherVariant calls down to the static MethodDesc::FindOrCreateAssociatedMethodDesc which is defined in genmeth.cpp which is not included in DAC builds.
Not sure if this would be safe to DAC-ize.
There was a problem hiding this comment.
I think we should try to DAC-ize the portion of the code path that is needed for GetAsyncVariantNoCreate(). If that is problematic for some reason we could do a separate standalone implementation but for that impl to be correct it has to replicate the logic in FindOrCreateAssociatedMethodDesc.
The runtime by default caches the async thunk when looking up runtime async methods in the methoddef map:
runtime/src/coreclr/vm/clsload.cpp
Lines 2804 to 2810 in 0c7ef81
This causes
DacDbiInterfaceImpl::GetNativeCodeInfoto incorrectly return data for the async thunk and not the actual method.