Repository navigation
ILVerify: continue checking methods that use calli, mkrefany, refanyval and refanytype instead of aborting them - #136133
Merged
jkotas merged 3 commits intoOct 21, 2026
Conversation
ILVerify stopped verifying a method at calli, mkrefany, refanyval or refanytype, so errors after the instruction were not reported. calli is now always reported as Unverifiable, and its call site is still checked: the signature token, the method pointer, the arguments, tail. and the return value. When the pointer comes straight from ldftn or ldvirtftn, the call-site signature is also compared with that method's signature. The tail. return checks move from ImportCall into CheckTailCallReturn, which call and calli both use. mkrefany, refanyval and refanytype follow ECMA-335 III.4.19, III.4.23 and III.4.22. mkrefany also rejects readonly pointers. It accepts a byref-like class, because the ECMA-335 augments allow a managed pointer to a byref-like type. arglist is unchanged and is still not supported. New VerifierError codes: ExpectedSignatureToken, CalliSignatureMismatch and ReadOnlyInMkrefany. Contributes to dotnet#136111
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @agocke |
jkotas
reviewed
Oct 20, 2026
The comment saying that mkrefany accepts a byref-like class sat above the type access check, which it does not describe. It now follows the ECMA-335 III.4.19 comment on the managed-pointer check, which is the rule it qualifies. There is no change in behaviour.
When the function pointer came directly from ldftn or ldvirtftn, the calli call-site signature was also compared with the target method's signature. Following review feedback, that check is removed, because real code does not pass the pointer to calli this way. CalliSignatureMismatch and the tests for the check go with it; the other calli call-site checks are unchanged.
This was referenced Oct 20, 2026
Member
|
/ba-g opened #136204 on the test failure |
Contributor
Author
|
Thank you for the review and merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This follows the reply in #136111:
callicannot be verifiable, because the runtime does not track the lifetime of function pointers, and it belongs under [ILVerify] Add option for IL validation without IL verification #37391. So everycalliis reported asUnverifiable, and verification continues. Call-site errors such as a wrong number of arguments are still reported. This PR does not add the validation-only mode that [ILVerify] Add option for IL validation without IL verification #37391 asks for.arglistunchanged:ImportArgListstill throws "RuntimeArgumentHandle not supported in .NET Core".#136111 still asks for
arglist, so this PR contributes to it rather than fixing it. If #136111 is narrowed to the four instructions this PR covers, this PR completes it.Rules
Section numbers are from ECMA-335, 6th edition.
calli(III.3.20):ExpectedSignatureToken).StackMethod).explicit, the signature does not give the type ofthis, so it is only popped. An uninitializedthiscannot be passed.tail.is checked as forcall. The return checks that were inImportCallare now inCheckTailCallReturn, whichcallandcallishare.mkrefany(III.4.19):StackByRef) that is not readonly (ReadOnlyInMkrefany).StackUnexpected).TypeAccess). It may be byref-like (see review point 1).TypedReferenceis pushed.refanyval(III.4.23): the operand must be aTypedReference. A managed pointer to the class is pushed. Because it may point to a caller's local, returning it is reported (ReturnPtrToStack).refanytype(III.4.22): the operand must be aTypedReference.RuntimeTypeHandleis pushed.The two new codes are appended at the end of
VerifierError, each with aStrings.resxentry.Two limits remain:
Unverifiablecounts as an error. An error after acalliin the same block is reported, but a later block may not be checked.calliandcall. This PR does not change that.Tests
CalliTests.ilhas 17 tests andTypedReferenceTests.ilhas 16, with one rule per test where possible.ILVerification.Testsgoes from 513 tests onmainto 546, all passing.Measured impact
I compared builds of
main(677ab53) with and without this change, running on linux-x64 under .NET 11 RC 1, with the .NET 10.0.12 shared framework as the reference set.The figures below were measured before commit 8463836 removed the check comparing a
callisignature with the method loaded byldftnorldvirtftn. In a separate comparison, builds from before and after that commit gave identical results for every method in the .NET 10.0.13 shared framework and the package sets below.Shared framework (172 assemblies, 130,146 method bodies). 55 methods contain
calliand 3 containrefanytype. None containsmkrefanyorrefanyval.mainUnverifiableAll 37 new findings come after the
calli:InitLocals33,UnmanagedPointer2 andStackUnexpected2. The same methods already report these kinds of errors before thecalli. Neither of the two new codes fired.Published packages (388,443 method bodies from hand-written IL packages, packages built with InlineIL.Fody, the 300 most-downloaded packages on nuget.org, and saved dynamic proxies). 149 methods were aborted at one of the four instructions, and ILVerify now continues checking them. They gained six findings, all at or after the instruction. One is a real bug: ImpromptuNinjas.ZStd 1.4.5.5's
ZStdDCtx::Copypushes two arguments for acalliwhose signature takes three, and ILVerify now reports a stack underflow. No other method changed, and neither of the two new codes fired.C# compiler output (SDK 10.0.401). Function-pointer calls now report only
Unverifiableat thecalli. Methods that use__makeref,__refvalueand__reftypeverify with no errors. A method that uses__argliststill fails as before.Roslyn. Four Roslyn tests quote the old messages:
ExtensionTests.AddressOf_TypeReceiver,LocalStateTracingTests.RefArguments_FunctionPointerInvocation,FunctionPointerTests.FunctionPointerInference_ExactInferenceThroughArray_RefKindMatch(in a comment) andCodeGenTests.Issue51228. They would need updating when Roslyn takes a new ILVerify.Points for review
These questions from #136111 are still open, and I chose a default for each:
mkrefanyand byref-like classes. This PR follows the augmented spec and accepts them. The 2012 edition of ECMA-335 said a managed pointer cannot point to a typed reference (I.8.6.1.3). The augments replace that with "a managed pointer can point to a byref-like type" (I.8.6.1.2). The old native verifier rejectedTypedReference,ArgIteratorandRuntimeArgumentHandle(VER_E_RA_PTR_TO_STACK), and the old JIT verifier rejected every byref-like class. I can add a rejection of all byref-like classes if you prefer it.mkrefanytype match. The type the pointer points to must exactly match the type named by the class token, soint32&withuint32is reported. Comparing verification types instead would accept it.TokenResolveorReadOnlyIllegalWriteinstead, or rename these.Contributes to #136111
Note
This PR was prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed it before posting.