Mark forwarders for all TypeReferences #2276
Conversation
sbomer
force-pushed
the
markForwardersForBuiltinTypes
branch
from
4275362
to
3a60602
Compare
Leave the logic as before and mark them from MarkStep.
| if (typeDefinition.HasProperties) { | ||
| foreach (var p in typeDefinition.Properties) { | ||
| WalkCustomAttributesTypesScopes (p); | ||
| // p.PropertyType is not saved |
There was a problem hiding this comment.
For future reference - this feels dangerous - not because of saving, but because of custom steps. We update the type refs in sweep step - but we don't update this one (intentionally). Again - nothing to do with this change, just that I noticed this...
test/Mono.Linker.Tests/TestCasesRunner/TestCaseMetadaProvider.cs
Outdated
Show resolved
Hide resolved
- Use ref assemblies for ReferenceAttribute - Avoid adding to common references unless needed by test dependencies
* Build testcases against reference assemblies * Reproduce issue in testcase * Add TypeReferenceMarker * Update one more testcase * Don't mark forwarders in TypeReferenceMarker Leave the logic as before and mark them from MarkStep. * PR feedback - Use ref assemblies for ReferenceAttribute - Avoid adding to common references unless needed by test dependencies
|
@sbomer - can you please prepare a backport to 6.0 RC2 for this? My understanding is that Xamarin will need this - @rolfbjarne can you please confirm? |
Correct, we need this. |
|
This went into the RC2 SDK with this change: dotnet/sdk#21118. |
|
Sorry - I didn't notice this PR was for the 6.0 branch. |
* Build testcases against reference assemblies * Reproduce issue in testcase * Add TypeReferenceMarker * Update one more testcase * Don't mark forwarders in TypeReferenceMarker Leave the logic as before and mark them from MarkStep. * PR feedback - Use ref assemblies for ReferenceAttribute - Avoid adding to common references unless needed by test dependencies
* Build testcases against reference assemblies * Reproduce issue in testcase * Add TypeReferenceMarker * Update one more testcase * Don't mark forwarders in TypeReferenceMarker Leave the logic as before and mark them from MarkStep. * PR feedback - Use ref assemblies for ReferenceAttribute - Avoid adding to common references unless needed by test dependencies
|
I can confirm this is in RC2, but it's not in the 6.0.1xx branch: https://github.com/dotnet/sdk/blob/7156781d4c687ddf6e0f24d244c0c239a92dd02e/eng/Version.Details.xml#L123-L131 |
|
@agocke - do you know what branch flows to the 6.0.1xx SDK? It might be that the answer is "none yet" and we would definitely need to fix that. |
|
Ah I thought there was automerge in sdk like there is in runtime. I'll insert the same build into both for now. |
…inker#2280) * Build testcases against reference assemblies * Reproduce issue in testcase * Add TypeReferenceMarker * Update one more testcase * Don't mark forwarders in TypeReferenceMarker Leave the logic as before and mark them from MarkStep. * PR feedback - Use ref assemblies for ReferenceAttribute - Avoid adding to common references unless needed by test dependencies Commit migrated from dotnet/linker@ff06442
Fixes #2267.
This fixes the issue by taking the first approach mentioned in #2267 (comment). We now mark type forwarders referenced from any
TypeReferencewe discover for a copy assembly, includingTypeReferences to built-in types which only exist as bytes in metadata rather than typeref rows.The walking logic in
TypeReferenceMarkeris copied fromAssemblyReferencesCorrectorinSweepStepand adjusted so that instead of updating the scopes of typereferences, it just marks any type forwarders they point to. The walking logic was duplicated to avoid touchingSweepStepto reduce the risk of this change.This includes a change to the test infrastructure to build tests against
System.Runtime, which is necessary to reproduce the issue (since it has to do with references to builtin types forwarded fromSystem.RuntimetoSystem.Private.CoreLib). That change caused the compiler to produce different code for event methods - before the change, there were unnecessary locals in the generated code. Either way the unreachablebodies optimization removes all locals for unreachable event methods, which is why I removed theExpectLocalsModifiedfrom a few testcases.I plan to factor out the duplicated logic in a follow-up that we won't consider taking into .NET6.