Add explicit null checks for zero-offset struct fields #77641
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFixes #77640.
|
| // This flag is set to enable the "conservative" style of explicit null-check insertion. | ||
| // This means that we insert an explicit null check whenever we create byref by adding a | ||
| // constant offset to a ref, in a MACK_Addr context (meaning that the byref is not immediately | ||
| // dereferenced). The alternative is "aggressive", which would not insert such checks (for | ||
| // small offsets); in this plan, we would transfer some null-checking responsibility to | ||
| // callee's of methods taking byref parameters. They would have to add explicit null checks | ||
| // when creating derived byrefs from argument byrefs by adding constants to argument byrefs, in | ||
| // contexts where the resulting derived byref is not immediately dereferenced (or if the offset is too | ||
| // large). To make the "aggressive" scheme work, however, we'd also have to add explicit derived-from-null | ||
| // checks for byref parameters to "external" methods implemented in C++, and in P/Invoke stubs. | ||
| // This is left here to point out how to implement it. |
There was a problem hiding this comment.
This comment is not correct; ldflda is required by the specification to throw on null addresses, immediately.
SingleAccretion
force-pushed
the
Zero-Offset-Fields
branch
from
bffbc42
to
324ab75
Compare
|
The diffs are relatively minor, the only large regressions are in MinOpts context from This fix will significantly simplify the @dotnet/jit-contrib |
|
Reminds me that we used to tolerate this sort of issues and treat pointers to value types as never null. |
|
Although the diffs don't look too bad in FullOpts so presumably it's fine? |
|
Some of the diffs do look to be in some scary functions: 3 (20.00 % of base) : 3668.dasm - ConfiguredValueTaskAwaiter[System.__Canon]:get_IsCompleted():bool:this
3 (20.00 % of base) : 3851.dasm - ConfiguredValueTaskAwaiter[System.__Canon]:GetResult():System.__Canon:this
3 (20.00 % of base) : 26284.dasm - System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder:SetException(System.Exception):this
3 (20.00 % of base) : 16608.dasm - System.Runtime.CompilerServices.ValueTaskAwaiter`1[System.__Canon]:get_IsCompleted():bool:this
3 (20.00 % of base) : 16609.dasm - System.Runtime.CompilerServices.ValueTaskAwaiter`1[System.__Canon]:GetResult():System.__Canon:this
3 (10.00 % of base) : 16777.dasm - System.Runtime.CompilerServices.AsyncTaskMethodBuilder:AwaitUnsafeOnCompleted[YieldAwaiter,<MultipleSerial>d__23[System.__Canon]](byref,byref):this
3 (0.53 % of base) : 14294.dasm - System.Collections.Generic.Dictionary`2[uint,System.__Canon]:FindValue(uint):byref:thisWe should keep an eye on related benchmarks and/or double check that the diffs in these are not impactful. |
I assume these all are small functions judging by the relative diffs and are inlined in the real world where null-checks might be folded |
That would be my assumption as well. For reference, the large relative diffs mainly come from cases where have a method that immediately tail-calls another (a member function). |
ghost
locked as resolved and limited conversation to collaborators
Fixes #77640.
We expect some unavoidable regressions.