Add support to add new FieldRVAs to EnC - #114463
Conversation
|
/cc @dotnet/dotnet-diag |
|
Tagging subscribers to this area: @tommcdon |
|
/cc @tmat |
noahfalk
left a comment
There was a problem hiding this comment.
I wasn't clear from #112446, is this experimental or this fully supported going forward? Jan seemed to have concerns. Is the plan that all diagnostic testing for this is going to come from Roslyn?
In terms of the code it seemed reasonable to me but I'm not a type system expert.
This would be fully supported.
Nope. I'll be submitting additional testing in other places in the coming days. |
I assume that it will be new test under https://github.com/dotnet/runtime/tree/main/src/libraries/System.Runtime.Loader/tests/ApplyUpdate |
|
This is ready for official review. I'll add disabled tests on Monday. |
|
LGTM |
| ApplyUpdateUtil.ApplyUpdate(assm); | ||
|
|
||
| { | ||
| var lrosLen = ApplyUpdate.Test.AddFieldRVA.LocalReadOnlySpan(); |
There was a problem hiding this comment.
These tests should verify that the content of the RVA field is expected. None of these tests seem to be actually verifying that the RVAs of the added fields are right (length is not part of the RVA blob).
(It is fine to do this as part of the tests enablement.)
| // Assert.Equal(5, mfaLen); | ||
| var utf8ros = ApplyUpdate.Test.AddFieldRVA.Utf8LiteralReadOnlySpan(); | ||
| Assert.Equal(6, utf8ros.Length); | ||
| var strlit = ApplyUpdate.Test.AddFieldRVA.StringLiteral(); |
There was a problem hiding this comment.
I expect that this will create RVA field only when the RVA strings Roslyn feature is turned on, with artificially low threshold. This will need change in the .csproj file to turn the feature on with artificially low threshold. It can use comment as well.
Fixes #112446