Enable processing of dynamically referenced assemblies #1666
Conversation
This file contains 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
marek-safar
reviewed
Dec 7, 2020
marek-safar
reviewed
Dec 7, 2020
Merged
sbomer
force-pushed
the
lazyLoad3
branch
7 times, most recently
from
5d5851b to
8c83813
Compare
sbomer
force-pushed
the
lazyLoad3
branch
3 times, most recently
from
c79c59d to
23a1757
Compare
sbomer
changed the title
sbomer
requested review from
vitek-karas,
MichalStrehovsky,
mateoatr and
tlakollo
vitek-karas
reviewed
Dec 16, 2020
sbomer
force-pushed
the
lazyLoad3
branch
2 times, most recently
from
5224272 to
f61d38c
Compare
We used to remove references to any resolved assembly that was unused. Now, an unused assembly may not have been resolved at all, so we need to iterate over references that might resolve to an unused assembly to ensure that they are removed. Iterating over all references from loaded assemblies accomplishes this, but we might miss a resolved unused assembly that was never referenced, and never set its action to Delete. (This can happen for type forwarders, for example.) A simple pre-pass over loaded assemblies ensures we handle this case.
To avoid loading mscorlib in most cases. Otherwise we load mscorlib and do a bunch of unnecessary work to resolve all of its exported types. Fixes CanEnableReducedTracing, which was getting a lot of spew from the exported type marking for mscorlib.
- Run RemoveSecurityStep for lazy assemblies - Resolve mscorlib in LoadI18nAssemblies
This class mediates access to the method actions which can be set by substitution xml. Also fix and enable the constant prop testcases which depend on substitutions.
This preserves the behavior which searches all referenced assemblies for interfaces with DynamicInterfaceCastableImplementationAttribute, and keeps them if they implement any marked interfaces.
- Clean up dead code - Clean up use of non-generic IDictionary - Add file headers - Use static classes for per-assembly logic - Link to open github issues - Avoid "step" terminology for per-assembly logic
vitek-karas
approved these changes
Feb 2, 2021
There was a problem hiding this comment.
Looks good. One last thing (can be done as a follow-up probably):
In order to avoid breaking Xamarin in a bad way, we could add a simple logic where if any custom step is added, insert a step which preloads the reference closure (just like we did before this change). The whole thing should still work just fine, it would just make it easier for the Xamarin custom steps to migrate over time.
Instead of calling EnsureProcessed at key points, the attribute XML processing is now done as part of cache initialization when it is first accessed. GetCustomAttributes, GetAction, and similar will no longer trigger processing that adds attributes to the global store. Instead, we maintain separate global and per-assembly stores, where the precedence (global > per-assembly) is explicit in the getters. In the case of attributes, the precedence is not important because they are additive, but it does matter for the method actions which can be mutated. As part of the change, the attribute/substitution steps have been separated into steps (which run on command-line XML) and parsers (which are run on cache initialization for the embedded XML). The steps are simple wrappers that call the parsers. The parsers can either return a new object representing the parsed information, or they can modify a passed-in object. The latter is used for the command-line XML steps which modify the global store of attributes or substitutions.
marek-safar
approved these changes
Feb 3, 2021
| EnsureProcessedSubstitutionXml (method.Module.Assembly); | ||
|
|
||
| if (_methodActions.TryGetValue (method, out MethodAction action)) | ||
| if (GlobalSubstitutionInfo.MethodActions.TryGetValue (method, out MethodAction action)) |
There was a problem hiding this comment.
It's odd that global substitution wins over assembly local substitution but it's probably just a naming issue.
There was a problem hiding this comment.
It's to keep the behavior where SetAction from a custom step wins over the action from a substitution XML. I changed it to "PrimarySubstitutionInfo" - not sure if that's much better, but at least it doesn't mislead about the precedence.
CustomAttributeSource.GlobalXmlInfo -> GlobalAttributeInfo LinkAttributesParser._xmlInfo -> _attributeInfo
- Rename "Global" to "Primary" - Use TryGetValue - Use properties instead of fields
vitek-karas
approved these changes
Feb 3, 2021
| FieldValues[field] = value; | ||
| } | ||
|
|
||
| public void SetSubstitutedInit (FieldDefinition field) |
There was a problem hiding this comment.
This should read SetFieldInit right?
There was a problem hiding this comment.
This was the existing name, but I like your suggestion better - changed
| FieldInit = new HashSet<FieldDefinition> (); | ||
| } | ||
|
|
||
| public void SetAction (MethodDefinition method, MethodAction action) |
There was a problem hiding this comment.
Nit: I would call this SetMethodAction.
There was a problem hiding this comment.
Same as above - changed (note that the Annotations helper is still called SetAction)
|
|
||
| namespace Mono.Linker.Steps | ||
| { | ||
| public class BodySubstitutionParser : ProcessLinkerXmlStepBase |
There was a problem hiding this comment.
Nit: rename ProcessLinkerXmlStepBase - it's not a "Step base" anymore.
There was a problem hiding this comment.
It actually was still a base step - in the case of descriptor xml, it was called from the pipeline. And even for the embedded XML, parsing relied on the Process (LinkContext context) method from IStep.
I factored this out some more so that the shared XML processing logic is in a new class that's no longer an IStep (named ProcessLinkerXmlBase). There's a new derived class for the descriptor XML, and ResolveFromXmlStep becomes a wrapper for it like the other XML steps.
- SubstitutionInfo.SetAction -> SetMethodAction - SubstitutionInfo.SetSubstitutedInit -> SetFieldInit - ProcessLinkerXmlStepBase -> ProcessLinkerXmlBase
- Make ProcessLinkerXmlBase not an IStep - Separate descriptor marking logic from IStep implementation in ResolveFromXmlStep - Introduce DescriptorMarker helper similar to the XML parsers - Remove parse overload that returns XML info - Provide context to ctor of XML processing logic
vitek-karas
approved these changes
Feb 4, 2021
|
The previous linker update in runtime is in, this is good to go. |
|
Thanks for all the feedback @vitek-karas and @marek-safar! |
This was referenced Mar 12, 2021
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
Track pending marked members This enables tracking of pending marked items which need to be fully marked by MarkStep. Contributes to dotnet/linker#1735 - this change is in preparation for running custom steps during MarkStep, where they could change the Annotations state in ways that interact with other marking logic. The idea is that custom steps can call Annotations methods without triggering a lot of other processing, because the full logic is deferred. This way custom steps don't need to be re-entrant. This change causes Annotations.Mark to place members into a set of "pending" items which will get fully marked ("processed") later. "pending" items are already considered marked. AddPreservedMethod conceptually adds a conditional dependency from source -> destination - so if source gets marked, destination should get marked. This change will immediately call Annotations.Mark on the destination if the source is already marked, and otherwise track the condition to be applied if the source gets marked later. SetPreserve can change the TypePreserve of a type after it has been marked. This change will track any changes to TypePreserve and apply them later even for types which have already been marked. This works a little differently from AddPreservedMethod, to avoid traversing type members in Annotations. Note that once we allow custom steps to run during during MarkStep, Process will call ProcessMarkedPending - but this isn't required in this change. The intention here is just to add adding extra tracking (that will actually be used by dotnet/linker#1666), while mostly preserving existing behavior. Commit migrated from dotnet/linker@24ba73a
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
…#1666) * Remove LoadReferencesStep Introduce TryResolve helper, and avoid some calls to GetLoadedAssembly * Load assemblies lazily - Run TypeMap logic on-demand - Register every resolved assembly so that it gets an action - Process embedded XML files lazily - Run body substitutions lazily - Introduce abstraction to allow XML processing to run before or during MarkStep - Iterate over reference closure when needed instead of only looking at loaded assemblies - Track applied preserve info so that it may be changed by new XML - Only mark assemblies when used - Introduce test attributes to check instructions in other assemblies - Add a base class for per-assembly logic Squashed commit with updates, cleanup, PR feedback Cleanup Clean up copy/save mark logic Call TryResolve where we used to look for loaded assemblies without throwing Process XML from type forwarder assemblies PR feedback - Prefix "assembly" field with underscore - Change "context" field to a property Separate per-assembly step processing Embedded XML is now read only when needed, with separate caches for descriptors, substitutions, attributes, removing unreachable blocks, and remaining per-assembly steps. The attribute cache is kept on CustomAttributeSource. BaseAssemblyStep -> BasePerAssemblyStep And don't implement IStep. Instead, take an assembly and context in the ctor. Some PR feedback - Don't change GetType use in attribute XML - Add clarifying comments - Avoid an extra cache for marking entire assembly PR feedback - remove Delete case (this command-line option will be removed) - change EmbeddedXmlStep -> EmbeddedXmlInfo static class PR feedback - Don't support xml in pure facades - Update constant prop test after rebase - Slightly clean up RemoveUnreachableBlocksStep * Add tests for swept references And fix up some existing tests * Add lazy loading testcases - RemoveAttributeInstances from lazy XML - Changing TypePreserve from lazy XML - Substitutions from lazy XML - Constant propagation in lazy assemblies * Keep copy/save behavior for lazy assemblies * Avoid loading all references while sweeping * Ensure that unused assemblies have references removed We used to remove references to any resolved assembly that was unused. Now, an unused assembly may not have been resolved at all, so we need to iterate over references that might resolve to an unused assembly to ensure that they are removed. Iterating over all references from loaded assemblies accomplishes this, but we might miss a resolved unused assembly that was never referenced, and never set its action to Delete. (This can happen for type forwarders, for example.) A simple pre-pass over loaded assemblies ensures we handle this case. * Remove DynamicDependencyLookupStep * Change corlib search order To avoid loading mscorlib in most cases. Otherwise we load mscorlib and do a bunch of unnecessary work to resolve all of its exported types. Fixes CanEnableReducedTracing, which was getting a lot of spew from the exported type marking for mscorlib. * Fix behavior of mono steps - Run RemoveSecurityStep for lazy assemblies - Resolve mscorlib in LoadI18nAssemblies * Turn on tracking of lazy pending members * Move MarkExportedTypesTargetStep * Hide substitution processing inside helper class This class mediates access to the method actions which can be set by substitution xml. Also fix and enable the constant prop testcases which depend on substitutions. * Load all references for IDynamicInterfaceCastable This preserves the behavior which searches all referenced assemblies for interfaces with DynamicInterfaceCastableImplementationAttribute, and keeps them if they implement any marked interfaces. * PR feedback - Clean up dead code - Clean up use of non-generic IDictionary - Add file headers - Use static classes for per-assembly logic - Link to open github issues - Avoid "step" terminology for per-assembly logic * Move attribute/substitution processing to cache initialization Instead of calling EnsureProcessed at key points, the attribute XML processing is now done as part of cache initialization when it is first accessed. GetCustomAttributes, GetAction, and similar will no longer trigger processing that adds attributes to the global store. Instead, we maintain separate global and per-assembly stores, where the precedence (global > per-assembly) is explicit in the getters. In the case of attributes, the precedence is not important because they are additive, but it does matter for the method actions which can be mutated. As part of the change, the attribute/substitution steps have been separated into steps (which run on command-line XML) and parsers (which are run on cache initialization for the embedded XML). The steps are simple wrappers that call the parsers. The parsers can either return a new object representing the parsed information, or they can modify a passed-in object. The latter is used for the command-line XML steps which modify the global store of attributes or substitutions. * Pick better names CustomAttributeSource.GlobalXmlInfo -> GlobalAttributeInfo LinkAttributesParser._xmlInfo -> _attributeInfo * PR feedback - Rename "Global" to "Primary" - Use TryGetValue - Use properties instead of fields * Rename more things - SubstitutionInfo.SetAction -> SetMethodAction - SubstitutionInfo.SetSubstitutedInit -> SetFieldInit - ProcessLinkerXmlStepBase -> ProcessLinkerXmlBase * Separate shared XML logic from IStep interface - Make ProcessLinkerXmlBase not an IStep - Separate descriptor marking logic from IStep implementation in ResolveFromXmlStep - Introduce DescriptorMarker helper similar to the XML parsers - Remove parse overload that returns XML info - Provide context to ctor of XML processing logic Commit migrated from dotnet/linker@ed3d691
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.
Replaces #1611, now using the design from #1626.