| Age | Commit message (Collapse) | Author |
|---|
|
* Warn on usage of attributes with annotated ctors
* PR feedback
Add ProducedBy from Test Restructure
Add support for SetupCompileBefore in the analyzer
Run analyzer tests on all members (not only methods)
Move tests to RequiresCapability
* Enable analyzer tests for attributes which use RUC annotated properties
* Lint
* Rename CheckAttributeCtor
Check for instantiations that set annotated properties
* Apply suggestions from code review
Co-authored-by: Andy Gocke <angocke@microsoft.com>
* Update comment
Rename Linker => Trimmer in ProducedBy
* Fix applied suggestions
* Lint
Co-authored-by: Andy Gocke <angocke@microsoft.com>
|
|
|
|
|
|
|
|
* Nullable annotations part 1
Enable nullable annotations for LinkContext and the XML processing classes.
|
|
* Normalize warning messages
* Apply suggestions from code review
Co-authored-by: Sven Boemer <sbomer@gmail.com>
* Update expected logged message
Co-authored-by: Sven Boemer <sbomer@gmail.com>
|
|
This confuses IDE tests runners in different ways.
* In Rider, if two folders contain a test with the same name, only 1 will ever run. Rider silently drops the other and never runs it.
* In Visual Studio 2022 Preview the tests a grouped oddly in the UI. For example, tests under `Attributes.Csc.*` will appear as having their own top level grouping rather than nested under `Attributes`. I did not test if VS suffered from the above. It may also.
|
|
|
|
We are migrating our unit test projects to run on net5. However, we will continue to run our linker tests against a mono like profile (since that is what Unity is based on).
Eliminating PeVerifier entirely causes us problems. We want to continue running it on the linker test executables.
This PR refactors how PeVerifier is made to do nothing when compiling with NETCOREAPP defined. This approach lets the illink tests continue to operate as they do now. While also giving us the ability to run our `Unity.Linker.Tests` project on net5, our test case assemblies compiled against a .NET Framework profile, and have our test framework continue to run PeVerifier
|
|
This causes us problems with our test framework since our assembly containing additional test cases is called Unity.Linker.Tests.Cases.
|
|
* Validate unrecognized patterns against logged warnings
This changes the linker validation to check for unrecognized
reflection access patterns against logged messages. The plan is to do
the same for the analyzer tests, allowing us to make progress as we
replace UnrecognizedReflectionAccessPatternAttribute with
ExpectedWarningAttribute.
The UnrecognizedReflectionAccessPatternAttribute is now checked
against both the logged messages and recorded unrecognized reflection
patterns. We check that it matches the same number of patterns as
logged warnings for consistency. This required some changes to the
signature formats used for validation, since the signatures used in
this attribute now need to use the same format as the one we use for
logged warnings.
Changing the signature formats here uncovered some places where we
were still using the cecil signature format even for user-visible
warnings. These have been changed to use the format from
GetDisplayName.
Generic parameter dataflow validation treats the
UnrecognizedReflectionAccessPatternAttribute differently, using the
member name to refer to a generic type parameter. This inconsistency
has been lifted into a separate custom attribute argument that
specifies when the attribute refers to a generic
parameter. Non-generic method parameters had a similar problem, but
this was less common, so those usages were just replaced by
ExpectedWarningAttribute.
* PR feedback
- Continue returning cecil signature as fallback
(replace exception with a debug assert)
- Remove left-over logic from debugging
* Fix more tests
- Use display name signature format for MethodReference
- Revert changes that expect attributes to match only one warning
- Remove debug output
- Also add missing using
|
|
Add new error code for cases in which the static constructor is
annotated with RUC
Add IL2116 to error codes
Move static constructor verification in MarkField before pushing a new
stack to print warnings in the method caller instead of fields
Don't treat static constructors annotated with RUC as dangerous
Add test for several static constructor calls
Warn on field instead of warning on .cctor
* Field should warn no matter the DependencyKind except
DynamicallyAccessedMemberOnType
* Add test for Access to field via reflection, dynamic dependency and
using fields on attributes
Co-authored-by: vitek-karas <vitek.karas@microsoft.com>
|
|
* Support assembly as message origin
* Add tests
* Fix test failures
* Add missing file
* Update src/linker/Linker.Steps/MarkStep.cs
Co-authored-by: Mateo Torres-Ruiz <mateoatr@users.noreply.github.com>
Co-authored-by: Mateo Torres-Ruiz <mateoatr@users.noreply.github.com>
|
|
Cecil doesn't handle IL editing well. It doesn't patch scopes and branches. Added a linker-private IL processor which does this.
Added tests for the IL corruption when removing branches around code which has exception handlers.
I also removed now unnecessary test attribute, since in my previous change the exception handlers are now validated within the instruction stream, so no need for a second attribute to validate these.
Co-authored-by: Sven Boemer <sbomer@gmail.com>
|
|
The isinst optimization replaces that instruction with a pop, ldnull when the type is not instantiated. This changes the instruction and also changes the length of the instruction in that position. Cecil unfortunately doesn't update try/catch/filter references and they keep pointing to the old isinst instruction which not part of the method body anymore. When saving the assembly the offsets stores in the try/catch/filter records end up effective random and corrupted.
This is a short-term fix to unblock failures in runtime due to this problem.
Medium term fix would be to carefully handle all IL replacements in the linker with regard to try/catch/filter records.
Ideally the long term fix would be to do this in Cecil in such a way that IL replacements would be correctly handled on their own.
This fixes the Http3RequestStream failures mentioned in #2181, but I was not able to confirm if this fixes the CoreLib ArrayPool issues as well (I think it will not).
|
|
|
|
* Fix warning origin for DAM on types
* Update tests with link to suppression issue
Messages for these warnings can not be suppressed at the member level
due to issues described in https://github.com/mono/linker/issues/2163.
Also fix expected warnings for properties, revert ResultChecker changes,
and remove redundant test.
* PR feedback
- Include "Attribute" in attribute type names
- Add comments about why we want to report warnings
- Add comment about the interaction with RUC on types
* Add testcase for nested type with default ctor
* PR feedback
- Clean up some test attributes
- Add test with DAM field on annotated PublicMethods type
- Add link to issue about duplicate warnings
Also use replace DAMT with DAM in tests.
* Use member-level suppressions in tests
* Unify with behavior for DAM warnings
- Don't warn on non-virtual methods that don't have return annotations
- Use the helpers introduced in https://github.com/mono/linker/pull/2145
* PR feedback
Clarify that getter without annotated return value doesn't warn because
it's non-virtual
|
|
* Bump cecil package version
The submodule update in https://github.com/mono/linker/pull/2156
incremented the cecil version, but we were still using 0.11.3 as the version
in our package graph. See https://github.com/mono/linker/pull/1515 for more
context on how this is set up.
Fixes https://github.com/mono/linker/issues/2173
* Remove unnecessary cast
|
|
The core already recognized empty array as no args, but null means the same thing.
This change also:
* Fixes a bug when PublicConstructors was required but only PublicParameterlessConstructors were provided the warning message would not say what the requirement was.
* Simplifies some of the code
* Better matching for UnrecognizedReflectionAccessPattern and warnings in tests
* Added tests
|
|
* Fix writing of updated copyused scopes
Fixes https://github.com/mono/linker/issues/2138
* Fix using order
|
|
* Preserve custom operators
This will keep custom operators on marked types whenever System.Linq.Expressions
is used, and the operator input types are marked.
The behavior is enabled by default, and can be disabled by passing
--disable-operator-discovery.
Addresses https://github.com/mono/linker/issues/1821
* Fix behavior for operators on nullable types
* Cleanup and PR feedback
- Avoid processing pending operators Dictionary if Linq.Expressions is unused
- Allocate this possibly-unused Dictionary lazily
- Use readonly field for always-used HashSet
- Rename markOperators -> seenLinqExpressions
- Clean up ProcessCustomOperators call to make intent more clear
- Add comments
- Check MetadataType.Int32 instead of searching BCL for Int32
* Remove unnecessary parens
* PR feedback
- seenLinqExpressions -> _seenLinqExpressions
- use List for pending operators instead of HashSet
|
|
This change implements the following high-level functionality:
* `UnconditionalSuppressMessage` applies to the entire method body even for iterators, async methods
* `RequiresUnreferencedCode` automatically suppresses trim analysis warnings from entire method body for iterators, async methods
Solution approach:
* Detect compiler generated code by using the `IteratorStateMachineAttribute`, `AsyncStateMachineAttribute` and `AyncIteratorStateMachineAttribute`.
* When a method which is compiler generated (detected by looing for `<` in its name) is processed the original "user method" is determined by looking for a method with the "state machine" attribute pointing to the compiler generated type.
* This information is cached to avoid duplication of relatively expensive detection logic.
* Looks for `UnconditionalSuppressMessage` and `RequriesUnreferencedCode` in:
* If the warning origin is not a compiler generated code - simply use the warning origin (method) - existing behavior
* If the warning origin is compiler generated use both the origin as well as the user defined method (non-compiler-generated) which is the source of the code for the compiler generated origin
This is done by storing additional `SuppressionContextMember` on `MessageOrigin` which should always point to non-compiler-generated item.
Added lot of new tests for these scenarios.
This implements warning suppression part of https://github.com/mono/linker/issues/2001 for state machines.
|
|
|
|
Recent chaneg introduced a scope stack to marking which carries `MessageOrigin` around so that warnings can be reported with precise origin. The one exception to this was the `ReflectionMethodBodyScanner`.
This change integrates the scope stack with `ReflectionMethodBodyScanner`. The visible result is more precise reporting of some warnings. This change will be needed for the work around compiler generated code (since then MessageOrigin will carry even more information than it does currently).
The main API change is addition of the `MessageOrigin` parameter to the `IReflectionPatternRecorder.UnrecognizedReflectionAccessPattern`. There are plans to remove this interface completely, so there' not much need to clean it up right now (so I left the potentially duplicated `origin` and `source` arguments as is).
The rest of the change is just correctly flowing the information around.
I also improved the test infra to be able to correctly handle `ExpectedWarning` with source/line info on all warnings.
There are visible changes in the `SourceLines` test where some warnings changed location (more precise now).
|
|
Also added a small fix in the test infra
|
|
Probably due to copy/paste error we've validated the attribute by looking at all warning messages at once. This means that potentially any warning can be matched to the attribute even if its location doesn't match.
This change fixes that but uncovers some small test issues:
* The expected warnings happen in different places - fixed that
* Some warnings can't be reported by the Roslyn Analyzer as they require global view - added a mechanism to mark these
Refactored a bit of the Roslyn analyzer validation code to support named properties in attributes.
|
|
* Flow cppcli assemblies into the linker
* Add tests
* Add C++/CLI assembly
* Update docs/error-codes.md
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
* PR feedback
* Change to copy action.
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
|
|
|
|
Implement marking logic for Interfaces
- Transitively mark all interface implementations on a type
- Mark all interface implementations on all base types as well
Implement annotation propagation over Type.GetInterface
Add tests for marking behavior as well as annotation behavior
Workaround: Currently the Interfaces enum value can't be used by the linker itself
since it can't yet rely on high enough version of framework. Worked around this
by adding an overlay class and a const value. Once linker is upgraded to high enough
framework version, this workaround should be removed.
|
|
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
|
|
|
|
This reverts commit bf37b2909bbfb2942173e7b383642c9b32ae0e79.
|
|
|
|
* Pick up the DynamicallyAccessedMemberTypes.Interfaces enum from runtime
* Update branding of the linker assembly
* Fix nullref in tests - corelib now has an assembly level attribute with annotations (so the reported "source" is null - since we can't represent assembly as a source just yet).
|
|
* Check that namespace is not empty
* Add test
|
|
* Preserve types for XML serializer
* Move TypePreserve.All
* Add doc about serialization handling
* Adjust comments
* Remove dataflow type discovery
* Add more tests, update doc
* Add logic for DataContractSerializer
* Remove IsActiveFor check
* Fix comments
* Update docs
* PR feedback
- Remove redundant usings
- Make MarkSubStepsDispatcher non-abstract, make ctor public
- PreserveSerialization -> PreserveSerializationSubStep
- SerializationHelper -> SerializationMarker
* Update heuristics to be less conservative
Instead of trying to conservatively match the xamarin-android behavior,
this now does something more "correct" without building in too much
serializer-specific logic. Specifically:
- Don't preserve methods
- Don't preserve static members
- Don't preserve private members
* Only scan types that are already marked
* Fix test
* Activate serialization logic only for ctor calls
With this change the serialization heuristics will mark types only if
there was a call to a relevant serialization constructor.
* PR feedback
Update docs
* PR feedback
Change flag to --disable-serialization-discovery, and add a test
to check that disabling works as expected.
* Add missing changes
* Update docs/serialization.md
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
* Update docs/serialization.md
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
* Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
* Update src/linker/Linker.Steps/DiscoverSerializationHandler.cs
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
* PR feedback
To ensure private attributed members get marked, this changes the "root"
tracking to track members rather than types. Marking a root member for
serialization will now ensure that the member and its type
(for fields/properties) also get marked recursively.
Similar behavior applies for static members, methods, and events.
Also address other feedback:
- Update docs to describe .ctor-based activation
- Clarify kept vs considered root
- RecursiveType -> SerializedRecursiveType
- Add comment about array type handling
* Handle DataContractJsonSerializer
* Mention DataContractJsonSerializer in docs
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
|
|
* Stop rewriting scopes for copy assemblies with removed references
* Keep exported types in copy assemblies
Mark dynamically accessed forwarders
Add tests
* Transitive forwarders are linked
* Keep copyused behavior
* Tests
* Mark type if it's exported
* PR feedback
* Transitively mark forwarders when a facade is dynamically accessed and has copy action
* Transitively mark forwarders
* Fix formatting
* Update MarkExportedType
* Keep mscorlib assert
* Mark forwarders when there's a type ref to an exported type in a copy assembly
* Update DependencyKind
* Keep forwarders when member ref is resolved from a copy assembly
* Add more tests, mark forwarded CAs
* Mark forwarded interfaces
* Simplify logic for typerefs
* Clean
* Keep ProcessInternalsVisibleAttributes in process while-loop
* Fix whitespace formatting
* Remove unused param
* Feedback
* Whitespace formatting
* Remove unnecessary assembly
* Add IL2104 warning
* Remove unnecessary marking
* Update comment
* Remove ExportedTypeExtensions
* Remove formatDiagnostics and revert cecil subm change
* Remove warning
* Comment out check for removed warning
* Update more of existing calls
* Reproduce attribute issue
* Update scopes before sweeping forwarders
* Remove my enum from compilation
* Fix formatting
* Keep same behavior for scopes in copyused assemblies
Co-authored-by: Marek Safar <marek.safar@gmail.com>
Co-authored-by: Sven Boemer <sbomer@gmail.com>
|
|
This reverts commit 1d96189abb1b2cfe00dff817c63b84b25534746a.
|
|
Co-authored-by: Marek Safar <marek.safar@gmail.com>
|
|
|
|
|
|
|
|
* Support attribute opt-in
* Update docs
* Rename test attributes to match
* Don't pass native SDK assemblies to tests
on Windows
* Update LinkTask
* PR feedback
- Don't warn on duplicate attributes
- Remove comment
- Fix typos and wording
- Use static array
- Rename CheckIsTrimmable -> IsTrimmable
* PR feedback: rename options
- --trim-action -> --trim-mode
-- --default-action -> --action
* Fix ILLink.Tasks test
* PR feedback
- Add plenty of comments
- RootNonTrimmableAssemblies -> ProcessReferencesStep
- Make -reference order stable using List instead of HashSet
* PR feedback
Make GetInputAssemblyPaths private
|
|
|
|
(#1830)
|
|
settings exist"
This reverts commit 4a74557e98bcf472d9bac9d05a934bc8216a4fca.
|
|
|
|
* 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
|
|
Linker has intrinsic handling for some methods which are annotated with `RequiresUnreferencedCodeAttribute`. The intrinsic handling will sometimes recognize the pattern and avoid generating a warning, at other times it will generate a better more specific warning. In cases where this happens the generic handler for `RequiresUnreferencedCodeAttribute` should not kick in, as it would still produce a warning.
The change:
* For direct calls, virtual calls and `.ctor` calls rely on `ReflectionMethodBodyScanner` to handle all warnings caused by `RequiresUnreferencedCodeAttribute`. It has knowledge about intrinsic handling and can handle all non-intrisic calls as well.
* Limit the generic handler in `MarkMethod` to all other cases (typically reflection or other indirect references to the method)
Test changes:
* We already have test cases which trigger these conditions, but we don't have a way to validate that extra warnings were not produced
* Added `ExpectedNoWarnings` test attribute which validates no warnings other than those explicitly stated by the test are produced.
* Used it on two test cases which trigger the interesting condition - and fixed the tests to correctly expect all warnings
|
|
* Only process direct calls and reflection method references.
* Clean test
* Add tests
* Check for null origin
* Warn on DynamicallyAccessedMember kind.
* Update warnings message for Requires Unreferenced Code
* Mark interfaces of types accessed via reflection
* Fix whitespace
* Add includeInterfaceTypes parameter
* Keep set action for methods in MarkEntireType
* Add test for dynamic dependency with RUC.
* Fix DependencyKind in MarkTypeForDynamicallyAccessedMembers
Pass null to origin's parameter when processing optimized overrides
Add DynamicDependency to list of valid dependency kinds in ProcessRequiresUnreferencedCode
* Fix formatting
|
