Fix for bug #53520, Entry point not found #53772
Conversation
|
Do we know why this isnt hit for later versions of newtonsoft? The affected method doesnt seem to have changed. |
There was a problem hiding this comment.
Is NotFoundBehavior.ReturnNull correct here?
When we run into missing depedencies during compilation, we should leave holes in the image. We should not generate broken code. I suspect that NotFoundBehavior.ReturnNull will generate broken code.
Notice that there are not existing uses of NotFoundBehavior.ReturnNull in this file.
There was a problem hiding this comment.
Good point, thanks, fixed in 2nd commit.
|
@mangod9 - I think it can easily be an ordering issue - if we first harvest MemberInfo with the right typeref token (01000076), everything is fine. This can be easily affected by subtle changes in the IL code, it could even be non-deterministic in parallel mode. |
Manish investigated this issue and he found out that the problem is caused by the fact that in the method Newtonsoft.Json.Utilities.ReflectionUtils.GetFields Crossgen2 produces a NewObject fixup for the incorrect type System.Collections.Generic.List`1<System.Type> instead of the right type System.Collections.Generic.List`1<System.Reflection.MemberInfo> This was caused by a bug in the token harvesting logic; at some point resolveToken was asked to resolve the token 0x0A02BA, string [System.Runtime/*23000001*/]System.Type/*01000019*/::get_Name() /* 0A0002BA */ As part of the token harvesting logic we looked at the MethodReference.Parent (01000019) and stored it in the harvesting table along with the OwningType of the method. The problem is that the translation of the MemberReference to MethodDesc resolves the method on a base class, string [System.Reflection/*23000009*/]System.Reflection.MemberInfo/*01000076*/::get_Name() /* 0A0002B5 */ As a result we were storing the incorrect [token - type] pair [01000019 - System.Reflection.MemberInfo] into the type token translation table and that confused the signature encoder because 01000019 is System.Type. The trivial solution is to separately translate the memberref parent entity handle to the owning type entity instead than extracting it from the MethodDesc. I have verified using R2RDump that I now see the correct NewObject fixup getting generated in the method. Thanks Tomas
trylek
force-pushed
the
OwningTypeTokenFix
branch
from
27e6426 to
f767389
Compare
|
Sigh, a new CG2 failure has appeared, it's hitting the test I swear my outerloop run for the 46239 fix was green and so were several runs after mine, this is the last successful run: https://dev.azure.com/dnceng/public/_build/results?buildId=1172801&view=results This is the first run that hits the regression: I'm going to call it a day shortly; I'll take a look tomorrow what exactly is failing in the test unless someone beats me to it. |
|
Yeah I am trying to look what might have caused the issue. My CI run for the stack overflow fix was green, and I also see it in this batched run before my merge: https://dev.azure.com/dnceng/public/_build/results?buildId=1172774&view=results |
|
Also doesnt look like this is cg2 related since it fails on non-r2r runs too. |
|
According to my initial local testing on Windows-x64 the regression test indeed fails when compiled with both Crossgen1 and Crossgen2 but for some reason it passes when not AOT-compiled. |
|
I cant repro without R2R as well, but wonder how is it failing in the non-r2r CI runs though? Adding @kunalspathak in case his change affected the test since it started failing as part of Batched CI run here: https://dev.azure.com/dnceng/public/_build/results?buildId=1172774&view=results. The run after this was possibly green since it just missed picking up the change? |
|
Yeah looks to be related to that change. I have created an issue for that: #53781. So you can merge if other tests are looking good. |
|
Thanks Manish for root-causing the issue. Indeed all the outerloop failures in my run are hitting just this one test so I'm merging this in. |
ghost
locked as resolved and limited conversation to collaborators
3 participants
Manish investigated this issue and he found out that the problem
is caused by the fact that in the method
Newtonsoft.Json.Utilities.ReflectionUtils.GetFields
Crossgen2 produces a NewObject fixup for the incorrect type
System.Collections.Generic.List`1<System.Type>
instead of the right type
System.Collections.Generic.List`1<System.Reflection.MemberInfo>
This was caused by a bug in the token harvesting logic; at some
point resolveToken was asked to resolve the token 0x0A02BA,
string [System.Runtime/23000001/]System.Type/01000019/::get_Name() /* 0A0002BA */
As part of the token harvesting logic we looked at the
MethodReference.Parent (01000019) and stored it in the harvesting
table along with the OwningType of the method. The problem is
that the translation of the MemberReference to MethodDesc
resolves the method on a base class,
string [System.Reflection/23000009/]System.Reflection.MemberInfo/01000076/::get_Name() /* 0A0002B5 */
As a result we were storing the incorrect [token - type] pair
[01000019 - System.Reflection.MemberInfo] into the type token
translation table and that confused the signature encoder because
01000019 is System.Type.
The trivial solution is to separately translate the memberref
parent entity handle to the owning type entity instead than
extracting it from the MethodDesc. I have verified using R2RDump
that I now see the correct NewObject fixup getting generated
in the method.
Thanks
Tomas
Fixes: #53520
/cc @dotnet/crossgen-contrib