Add sources from iltrim hackathon#126276
Add sources from iltrim hackathon#126276MichalStrehovsky wants to merge 119 commits intodotnet:mainfrom
Conversation
* intial FieldDefinition support * fb
Add support for TypeReferences and Assembly References
* Keep repro project in the SLN * Move ILTrim to the top of the SLN (first entry is what VS defaults to as a startup project even if the project is not an EXE...) * Remove unnecessary line.
Scan method body instructions for all opcodes which take a token operand. Create static dependency nodes for all tokens found. Rewrite the method body when writing output to update all tokens in it. I improved the tests to validate member removal as well. Implementation notes: - Brings in ILReader and ILOpcode form the type system - For now implemented ThrowHelper stub since the type system code uses this. What's missing: Scanning exception regions and reporting dependencies for tokens in them Rewriting exception regions and updating tokens in them I added MichalStrehovsky/iltrim#17 to track that work.
This removes the need for manually aligning the instructions in the IL stream, the encoder takes care of this when necessary.
This ports all of the test infrastructure from the linker repo. I intentionally removed any reflection pattern validation (since we don't want it in the linker either and it would not be possible anyway). Obviously all of the various command line arguments for the linker don't work either (the attributes are there, but they do nothing). Otherwise it works exactly like in the linker repo. As we implement features we should validate that the test infra can actually test them (it's very possible there are holes in the linker test infra). As for the tests: I ported the existing test over as the "First". I also copied one of the linker tests "UnusedMethodGetsRemoved" as-is... and it passes 😉 Note: This is obviously a HUGE change - so I don't expect people review it really. What I did: - Copied over the necessary files - Adapted everything to XUnit (this is actually most of the work) - I had to use FluentAssertions as well, since XUnit is very opinionated (and stubborn) in some cases. - This has some effect on how tests are viewed in the Test Explorer - XUnit doesn't have the same ability as NUnit to programaticaly generate test cases - it can do it, but there's no tests name customization. I tried to keep the names reasonable. - Fixed lot of null annotations - I tried to keep everything with nullable on, but wasn't able to do all of it - but most of it is nullable enabled. - Rewrite the argument construction - currently mostly empty - Removed any logging and reflection pattern validation - as the "product" doesn't have these capabilities yet
* Draft PR to get FB * Still not refactored and in draft but incorporate Sven's FB * Partial support for Local var signature * FB
* Add ParameterNode * Add Parameter to the list of supported HandleKinds * PR feedback
This replaces the stub EcmaModule with the "real" EcmaModule from the type system. The type system will provide services for us around resolving things to definitions and having a way to represent types/methods/fields in an interned universe. For now, this doesn't do much, but notice that the EcmaModule we're now using is more powerful. It e.g. provides a `GetObject` API that allows resolving EntityHandles to their type system representation.
* Add ILVerify checking * Fix formatting
Introduces common helpers to rewrite and analyze signatures. Local variable signatures are analyzed & rewritten in the first commit. The second commit shows how are we going to hook up the other kinds of signatures into the common code since all signatures somehow deal with encoding types and we don't want to duplicate that code. I hooked it up into MethodDefinition and MemberReference. The third commit enables @tlakollo's disabled test since we now support it. Fixes #4.
The ILReader and related classes didn't see ThrowHelpers. So I moved those to the TypeSystem project. But now ILReader is internal, so I added IVT from TypeSystem to Core. Not super clean, but probably best we can do right now. I cleaned up the VS-view of the TypeSystem project to match the TypeSystem directory on disk (mostly). Removed some unnecessary properties from projects. Added the ILVerification project into the solution, otherwise VS-based build fails.
The ILVerifier expects the resolver to resolve the same simple name to the same reader instance. Fixed the resolver implementation to do this. This makes all the tests pass now.
Fully compare ldstr and also compare any tokens in an instruction.
* Add runtime assembly references * Remove test code
* Stop fighting the coreclr build system, it enforces platform specific output. Instead adapt the projects in such a way that VS will always build it as x64. For now hardcoded the fact that it runs on x64 - everywhere. * Add missing project ref
Configure Logger from LinkContext properties instead of hardcoded values: - SingleWarn/SingleWarnEnabled/DisabledModules from context.SingleWarn - TreatWarningsAsErrors from context.GeneralWarnAsError - WarningsAsErrors from context.WarnAsError - SuppressedCategories includes 'Trim analysis' when context.NoTrimWarn - DisableGeneratedCodeHeuristics from context Remove CompilerTypeSystemContext stub, use ILTrimTypeSystemContext directly. Fix GetClosestDefType, AsyncMaskingILProvider, and Logger.GetModuleFileName to work with ILTrimTypeSystemContext under ILTRIM. Include exception message in test Assert.Ignore for better diagnostics. Update ILTrimExpectedFailures.txt: 7 warning tests now pass (SingleWarn, WarnAsError, etc.), 1 added (WarnVersion not supported in shared Logger). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/TypeDefinitionNode.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/ResultChecker.cs
Show resolved
Hide resolved
Hook --dump-dependencies and --dependencies-file into ILTrim's Driver: - Remove #if !ILTRIM around the dumpDependencies block in SetupContext - Override AddXmlDependencyRecorder/AddDgmlDependencyRecorder in ILTrim Driver to store the file name on LinkContext (DGML-only for now) - Use DependencyTrackingLevel from shared infrastructure to choose between NoLog/EventSource (when no dump) and FullGraph (when dumping) - Write DGML output after trimming when DependenciesFileName is set - Source-include DependencyTrackingLevel.cs in ILTrim.Core Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add --parallelism in the #else branch of the --custom-step ifdef block, setting context.MaxDegreeOfParallelism. Change the property from init to set to allow assignment from SetupContext. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use CodeOptimizations.RemoveDescriptors and IgnoreDescriptors from LinkContext to control ILLink.Descriptors.xml handling, matching ILLink: - RemoveDescriptors (default on): strip the resource from output - IgnoreDescriptors: skip processing descriptor roots - strip-descriptors false: keep the resource in output Update ILTrimExpectedFailures.txt: 2 descriptor tests now pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use BlobEncoder.Field() instead of FieldSignature() to properly handle the full ECMA-335 field signature format (FIELD CustomMod* Type), including ByReference fields and required/optional custom modifiers. This fixes BadImageFormatException when trimming assemblies that contain ref fields (e.g. Span<T>) or fields with unmanaged constraints. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ling Remove CompilerTypeSystemContext stub — use ILTrimTypeSystemContext directly via ifdefs in GetClosestDefType, AsyncMaskingILProvider, and Logger. Rework ConstructedTypeNode to properly track constructed types and their virtual method slots, matching ILLink's behavior more closely. Remove ObjectGetTypeFlowDependenciesNode from ILTrim (not needed for IL-level trimming). Ifdef out related code in shared dataflow files. Skip writing modules with assembly action Skip, preventing corrupted output for non-trimmed assemblies like System.Private.CoreLib. Rename DataflowStubs.cs to RootingHelpers.cs (only RootingHelpers remains). Update ILTrimExpectedFailures.txt: 20 tests now pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace hardcoded security attribute stripping with a check against context.StripSecurity, which is configurable via --strip-security (defaults to true, matching ILLink behavior). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per ECMA-335 §II.22.9, constant value blobs only contain primitive literals (bool, char, int, float, string, null) — never metadata tokens. The Parent token is already mapped correctly during writing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Match upstream commit 7e2ee62 which migrated Mono.Linker.Tests to MSTest. Update ILTrim.Tests to use the same framework: - TestRunnerName NUnit -> MSTest in csproj - [TestFixture] -> [TestClass], [TestCaseSource] -> [TestMethod][DynamicData] - Assert.Ignore -> Assert.Inconclusive, remove IgnoreException handling - NUnit.Framework -> Microsoft.VisualStudio.TestTools.UnitTesting - Fix Assert.False -> Assert.IsFalse in shared ResultChecker.cs (ifdef) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- FieldDefinitionNode: align RVA field data to MappedFieldDataAlignment - Driver: use Directory.Exists for output dir, File.Create to truncate, determine .exe/.dll from entry point presence instead of assembly name - LinkContext: fix AddSearchDirectory path concatenation, fix ToReferenceFilePaths to properly merge and return dictionary - ModuleWriter: only map entry point token when non-zero, add HasEntryPoint property - TypeDefinitionNode: use mapped handle for AddPropertyMap/AddEventMap - ResultChecker: add missing public modifier on partial class Co-Authored-By: Copilot <223556219+Copilot@users.noreply.github.com>
src/coreclr/tools/ILTrim.Core/DependencyAnalysis/TokenBased/ManifestResourceNode.cs
Outdated
Show resolved
Hide resolved
ManifestResource.Implementation can be an AssemblyReferenceHandle which TokenMap doesn't map. Mirror the pattern from TypeReferenceNode by resolving through AssemblyReferenceNode.TargetToken for AssemblyRef handles, falling back to TokenMap for other handle kinds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 77 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/tools/illink/src/linker/Linker/LinkContext.cs:60
- The type name
UnintializedContextFactoryappears to be misspelled ("Unintialized" vs "Uninitialized"). Since this is a public type, consider renaming it (and updating references) to avoid baking the typo into API surface and to improve searchability.
| case "--parallelism": | ||
| if (!GetStringParam(token, out string? parallelismValue)) | ||
| return -1; | ||
|
|
||
| context.MaxDegreeOfParallelism = int.Parse(parallelismValue); | ||
| continue; |
There was a problem hiding this comment.
--parallelism parses the value with int.Parse, which will throw and crash the tool on invalid input (e.g., non-numeric or out-of-range). Prefer int.TryParse (and validate > 0) and report a user-friendly LogError/missing-argument style error consistent with other options.
| // All AssemblyReferenceNodes should have the same table index. | ||
| Debug.Assert(base.CompareToHelper(other) == 0); | ||
| // Sort by simple assembly name. | ||
| int result = _reference.GetName().Name.CompareTo(otherAssemblyReferenceNode._reference.GetName().Name); |
There was a problem hiding this comment.
CompareTo uses string.CompareTo, which is culture-sensitive. Since this ordering affects metadata emission, the output can become non-deterministic across locales. Use an ordinal comparison (e.g., StringComparer.Ordinal/OrdinalIgnoreCase) for stable, deterministic ordering.
| int result = _reference.GetName().Name.CompareTo(otherAssemblyReferenceNode._reference.GetName().Name); | |
| int result = StringComparer.Ordinal.Compare(_reference.GetName().Name, otherAssemblyReferenceNode._reference.GetName().Name); |
| { | ||
| // Grabbing the mapped token to use as a ParameterList is awakward because | ||
| // S.R.Metadata abstracts away the raw value of the ParameterList field of the record. | ||
| // |
There was a problem hiding this comment.
Typo in comment: "awakward" should be "awkward".
5 years ago we did a hackathon exploring illink written on top of the ILCompiler.DependencyAnalysisFramework/managed type system and System.Reflection.Metadata.
I let Claude:
I experimentally started prompting Claude to fix failing tests and have a couple good result, but this is not part of that.
Submitting as a draft, I haven't really reviewed this myself either.
Cc @dotnet/illink