Redo IL Verification #90418

mrvoorhe · 2023-08-11T18:10:52Z

Now checks all assemblies in the output directory rather than just test.exe
Once again respects the the ability to skip verifying a single assembly via [SkipIlVerify("foo.dll")]
Now loads core libraries from the output directory (if they exist) instead of from the runtime install dir.
ALC logic was removed. I do not understand what value this provided.
The class libraries lead to a lot of errors. Rather than having to filter out a large number of errors, I added diff'ing. ILVerify will check the input assembly and remove any errors that existed in the input assembly. This makes verifying class libraries viable. Without it, you'd have to use [SkipIlverify] on every core link test or filter out a lot of VerifierError values.
Moved IL verification back to InitialChecking where PeVerifier was
Add a test to verify that il verification is mostly working. It doesn't give complete coverage over every behavior, but it's better than nothing.
SkipPeVerify renamed to SkipIlVerify
SkipPeVerifyForToolchian was removed. There is only 1 tool now.
Removed PeVerifier.
Remove many [SkipIlVerify] attributes. Diffing means they are no longer needed
ValidateTypeRefsHaveValidAssemblyRefs now runs regardless of whether or not the IL is verified. It wasn't clear to me why this logic would only be useful when il was verified.
Change UninitializedLocals to use ExpectIlFailure. This test seems to expect invalid il. Might as well assert that rather than skip ilverify entirely.
Remove the logic that disables ilverify when a test uses the unsafe argument. Diffing makes this obsolete.
Cut down on the number of VerifierError values that are filtered out. Diffing removes the need to filter them out.
IL Verification errors have been greatly improved.
will now output all IL errors in the failure message rather than just the first invalid IL.
Type and Method names are now displayed in the error message. I didn't do an exhaustive implementation, SRM is so tedious to use, but it's better than not having it
Tokens and Offsets are formatted nicely
Extension points opened up for Unity.
We need to supply different search directories.
We need to search for .winmd files since we support windows runtime.
In general I opened some things up in case we need to call them

ghost · 2023-08-11T18:11:17Z

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

Now checks all assemblies in the output directory rather than just test.exe
Once again respects the the ability to skip verifying a single assembly via [SkipIlVerify("foo.dll")]
Now loads core libraries from the output directory (if they exist) instead of from the runtime install dir.
ALC logic was removed. I do not understand what value this provided.
The class libraries lead to a lot of errors. Rather than having to filter out a large number of errors, I added diff'ing. ILVerify will check the input assembly and remove any errors that existed in the input assembly. This makes verifying class libraries viable. Without it, you'd have to use [SkipIlverify] on every core link test or filter out a lot of VerifierError values.
Moved IL verification back to InitialChecking where PeVerifier was
Add a test to verify that il verification is mostly working. It doesn't give complete coverage over every behavior, but it's better than nothing.
SkipPeVerify renamed to SkipIlVerify
SkipPeVerifyForToolchian was removed. There is only 1 tool now.
Removed PeVerifier.
Remove many [SkipIlVerify] attributes. Diffing means they are no longer needed
ValidateTypeRefsHaveValidAssemblyRefs now runs regardless of whether or not the IL is verified. It wasn't clear to me why this logic would only be useful when il was verified.
Change UninitializedLocals to use ExpectIlFailure. This test seems to expect invalid il. Might as well assert that rather than skip ilverify entirely.
Remove the logic that disables ilverify when a test uses the unsafe argument. Diffing makes this obsolete.
IL Verification errors have been greatly improved. ** will now output all IL errors in the failure message rather than just the first invalid IL. ** Type and Method names are now displayed in the error message. I didn't do an exhaustive implementation, SRM is so tedious to use, but it's better than not having it ** Tokens and Offsets are formatted nicely
Extension points opened up for Unity. ** We need to supply different search directories. ** We need to search for .winmd files since we support windows runtime. ** In general I opened some things up in case we need to call them

Author:	mrvoorhe
Assignees:	-
Labels:	`linkable-framework`, `needs-area-label`
Milestone:	-

ghost · 2023-08-11T18:49:16Z

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

Now checks all assemblies in the output directory rather than just test.exe
Once again respects the the ability to skip verifying a single assembly via [SkipIlVerify("foo.dll")]
Now loads core libraries from the output directory (if they exist) instead of from the runtime install dir.
ALC logic was removed. I do not understand what value this provided.
The class libraries lead to a lot of errors. Rather than having to filter out a large number of errors, I added diff'ing. ILVerify will check the input assembly and remove any errors that existed in the input assembly. This makes verifying class libraries viable. Without it, you'd have to use [SkipIlverify] on every core link test or filter out a lot of VerifierError values.
Moved IL verification back to InitialChecking where PeVerifier was
Add a test to verify that il verification is mostly working. It doesn't give complete coverage over every behavior, but it's better than nothing.
SkipPeVerify renamed to SkipIlVerify
SkipPeVerifyForToolchian was removed. There is only 1 tool now.
Removed PeVerifier.
Remove many [SkipIlVerify] attributes. Diffing means they are no longer needed
ValidateTypeRefsHaveValidAssemblyRefs now runs regardless of whether or not the IL is verified. It wasn't clear to me why this logic would only be useful when il was verified.
Change UninitializedLocals to use ExpectIlFailure. This test seems to expect invalid il. Might as well assert that rather than skip ilverify entirely.
Remove the logic that disables ilverify when a test uses the unsafe argument. Diffing makes this obsolete.
IL Verification errors have been greatly improved.
will now output all IL errors in the failure message rather than just the first invalid IL.
Type and Method names are now displayed in the error message. I didn't do an exhaustive implementation, SRM is so tedious to use, but it's better than not having it
Tokens and Offsets are formatted nicely
Extension points opened up for Unity.
We need to supply different search directories.
We need to search for .winmd files since we support windows runtime.
In general I opened some things up in case we need to call them

Author:	mrvoorhe
Assignees:	-
Labels:	`linkable-framework`, `community-contribution`, `area-Tools-ILLink`, `needs-area-label`
Milestone:	-

ghost · 2023-08-11T23:43:29Z

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

Now checks all assemblies in the output directory rather than just test.exe
Once again respects the the ability to skip verifying a single assembly via [SkipIlVerify("foo.dll")]
Now loads core libraries from the output directory (if they exist) instead of from the runtime install dir.
ALC logic was removed. I do not understand what value this provided.
The class libraries lead to a lot of errors. Rather than having to filter out a large number of errors, I added diff'ing. ILVerify will check the input assembly and remove any errors that existed in the input assembly. This makes verifying class libraries viable. Without it, you'd have to use [SkipIlverify] on every core link test or filter out a lot of VerifierError values.
Moved IL verification back to InitialChecking where PeVerifier was
Add a test to verify that il verification is mostly working. It doesn't give complete coverage over every behavior, but it's better than nothing.
SkipPeVerify renamed to SkipIlVerify
SkipPeVerifyForToolchian was removed. There is only 1 tool now.
Removed PeVerifier.
Remove many [SkipIlVerify] attributes. Diffing means they are no longer needed
ValidateTypeRefsHaveValidAssemblyRefs now runs regardless of whether or not the IL is verified. It wasn't clear to me why this logic would only be useful when il was verified.
Change UninitializedLocals to use ExpectIlFailure. This test seems to expect invalid il. Might as well assert that rather than skip ilverify entirely.
Remove the logic that disables ilverify when a test uses the unsafe argument. Diffing makes this obsolete.
Cut down on the number of VerifierError values that are filtered out. Diffing removes the need to filter them out.
IL Verification errors have been greatly improved.
will now output all IL errors in the failure message rather than just the first invalid IL.
Type and Method names are now displayed in the error message. I didn't do an exhaustive implementation, SRM is so tedious to use, but it's better than not having it
Tokens and Offsets are formatted nicely
Extension points opened up for Unity.
We need to supply different search directories.
We need to search for .winmd files since we support windows runtime.
In general I opened some things up in case we need to call them

Author:	mrvoorhe
Assignees:	-
Labels:	`linkable-framework`, `community-contribution`, `area-Tools-ILLink`
Milestone:	-

mrvoorhe · 2023-08-14T20:32:14Z

Locally I can reproduce the test failures on main. I also don't see how these failures could be related to my changes. So I suspect the linker tests are broken on main.

I believe this PR is ready to go @marek-safar @vitek-karas

vitek-karas · 2023-08-16T18:56:40Z

The failures are real now - I checked out this branch and rebased to main and it fails locally with:

Attributes.OnlyKeepUsed.NullableOnConstraints9s 17ms
Error:
Internal.TypeSystem.TypeSystemException+TypeLoadException : Failed to load type 'System.String' from assembly 'System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'

Stack trace:
   at Internal.TypeSystem.ThrowHelper.ThrowTypeLoadException(ExceptionStringID id, String typeName, String assemblyName) in F:\dotnet\runtime\src\coreclr\tools\Common\TypeSystem\Common\ThrowHelper.cs:line 17
   at Internal.TypeSystem.ThrowHelper.ThrowTypeLoadException(String namespace, String name, ModuleDesc module) in F:\dotnet\runtime\src\coreclr\tools\Common\TypeSystem\Common\ThrowHelper.Common.cs:line 21
   at Internal.TypeSystem.ResolutionFailure.Throw() in F:\dotnet\runtime\src\coreclr\tools\Common\TypeSystem\Common\ResolutionFailure.cs:line 93
   at Internal.TypeSystem.Ecma.EcmaModule.GetType(String nameSpace, String name, NotFoundBehavior notFoundBehavior) in F:\dotnet\runtime\src\coreclr\tools\Common\TypeSystem\Ecma\EcmaModule.cs:line 369
   at Internal.TypeSystem.ModuleDesc.GetType(String nameSpace, String name, Boolean throwIfNotFound) in F:\dotnet\runtime\src\coreclr\tools\Common\TypeSystem\Common\ModuleDesc.cs:line 38
   at Internal.TypeSystem.MetadataTypeSystemContext.SetSystemModule(ModuleDesc systemModule) in F:\dotnet\runtime\src\coreclr\tools\Common\TypeSystem\Common\MetadataTypeSystemContext.cs:line 74
   at ILVerify.Verifier.SetSystemModuleName(AssemblyName name) in F:\dotnet\runtime\src\coreclr\tools\ILVerification\Verifier.cs:line 44
   at Mono.Linker.Tests.TestCasesRunner.ILVerification.ILVerifier..ctor(NPath[] searchDirectories, String systemModuleName) in F:\dotnet\runtime\src\tools\illink\test\Mono.Linker.Tests\TestCasesRunner\ILVerification\ILVerifier.cs:line 36
   at Mono.Linker.Tests.TestCasesRunner.ILVerification.ILChecker.CreateILVerifier(NPath directory) in F:\dotnet\runtime\src\tools\illink\test\Mono.Linker.Tests\TestCasesRunner\ILVerification\ILChecker.cs:line 159
   at Mono.Linker.Tests.TestCasesRunner.ILVerification.ILChecker.Check(LinkedTestCaseResult linkResult, AssemblyDefinition original) in F:\dotnet\runtime\src\tools\illink\test\Mono.Linker.Tests\TestCasesRunner\ILVerification\ILChecker.cs:line 25
   at Mono.Linker.Tests.TestCasesRunner.ResultChecker.InitialChecking(LinkedTestCaseResult linkResult, AssemblyDefinition original, AssemblyDefinition linked) in F:\dotnet\runtime\src\tools\illink\test\Mono.Linker.Tests\TestCasesRunner\ResultChecker.cs:line 282
   at Mono.Linker.Tests.TestCasesRunner.ResultChecker.Check(LinkedTestCaseResult linkResult) in F:\dotnet\runtime\src\tools\illink\test\Mono.Linker.Tests\TestCasesRunner\ResultChecker.cs:line 96
   at Mono.Linker.Tests.TestCases.All.Run(TestCase testCase) in F:\dotnet\runtime\src\tools\illink\test\Mono.Linker.Tests\TestCases\TestSuites.cs:line 299
   at Mono.Linker.Tests.TestCases.All.AttributesTests(TestCase testCase) in F:\dotnet\runtime\src\tools\illink\test\Mono.Linker.Tests\TestCases\TestSuites.cs:line 34
   at InvokeStub_All.AttributesTests(Object, Span`1)
   at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

vitek-karas · 2023-08-16T18:57:05Z

Note that without rebase to main everything passes - so it has to be some interaction with very recent changes.

mrvoorhe · 2023-08-16T20:39:13Z

Note that without rebase to main everything passes - so it has to be some interaction with very recent changes.

Thanks for checking on that. I did not think to recheck the errors after the rebase. I will take a look

mrvoorhe · 2023-08-16T21:05:59Z

@vitek-karas NullableOnConstraints should be fixed now. It has [SetupLinkerTrimMode ("link")] which means the ILVerifier is going to load the trimmed core lib. System.String is in fact gone which IlVerifier is not happy about. Adding [IgnoreDescriptors (false)] ensures the basis survive.

mrvoorhe · 2023-08-17T13:41:17Z

@vitek-karas I'm not sure what to make of CI. Are the failures related to my changes?

vitek-karas · 2023-08-17T14:27:47Z

The non-wasm failures were already fixed by #90699 - I don't know why rerunning doesn't bet that. We would have to restart the whole CI probably - not worth it.

The wasm failures are definitely unrelated - they occur in other PRs as well.

mrvoorhe · 2023-08-17T14:49:08Z

@vitek-karas Great. This PR is ready to go then.

vitek-karas

I think this looks good - it's a lot of new code... but if it works and effectively enabled IL Verification everywhere (or almost), that's a big win.

@sbomer, could you please also take a quick look?

The one thing we could improve (eventually) is to avoid having to implement the TypeSystem->String conversions - we have that in the ILC type system.

...link/test/Mono.Linker.Tests.Cases.Expectations/Assertions/DisableIlVerifyDiffingAttribute.cs

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/SkipIlVerifyAttribute.cs

...ools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/ExpectIlFailureAttribute.cs

vitek-karas · 2023-08-17T14:50:39Z

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ILVerification/Extensions.cs

+
+public static class Extensions
+{
+	public static string GetMethodSignature (this MethodDefinitionHandle handler, MetadataReader metadataReader)


Thanks for writing this - eventually we might migrate this to use the internal type system from the ILCompiler (it's in the same repo, so taking a project reference is very cheap) which has full implementation of this. But that's for some other time.

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ILVerification/ILChecker.cs

vitek-karas · 2023-08-17T14:56:20Z

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ILVerification/ILChecker.cs

+			if (!DisableILDiffing (linkResult, original)) {
+				var inputResults = inputVerifier.VerifyByName (file.FileNameWithoutExtension);
+
+				outputResults = Diff (outputResults, inputResults);


This is a nice idea and I like it. My only hesitation is perf - if it makes tests a lot slower that would be something to think about... Did you try to get at least some rough understanding how much it costs to run the verifying twice effectively?

I did a high level check of the performance impact. Running the same command CI does to run tests I got
Before

Time Elapsed 00:06:15

And with this PR

Time Elapsed 00:06:31

So it's essentially insignificant.

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ILVerification/ILChecker.cs

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs

* Now checks all assemblies in the output directory rather than just `test.exe` * Once again respects the the ability to skip verifying a single assembly via `[SkipIlVerify("foo.dll")]` * Now loads core libraries from the output directory (if they exist) instead of from the runtime install dir. * ALC logic was removed. I do not understand what value this provided. * The class libraries lead to a lot of errors. Rather than having to filter out a large number of errors, I added diff'ing. ILVerify will check the input assembly and remove any errors that existed in the input assembly. This makes verifying class libraries viable. Without it, you'd have to use `[SkipIlverify]` on every core link test or filter out a lot of `VerifierError` values. * Moved IL verification back to `InitialChecking` where `PeVerifier` was * Add a test to verify that il verification is mostly working. It doesn't give complete coverage over every behavior, but it's better than nothing. * `SkipPeVerify` renamed to `SkipIlVerify` * `SkipPeVerifyForToolchian` was removed. There is only 1 tool now. * Removed `PeVerifier`. * Remove many [SkipIlVerify] attributes. Diffing means they are no longer needed * `ValidateTypeRefsHaveValidAssemblyRefs` now runs regardless of whether or not the IL is verified. It wasn't clear to me why this logic would only be useful when il was verified. * Change `UninitializedLocals` to use `ExpectIlFailure`. This test seems to expect invalid il. Might as well assert that rather than skip ilverify entirely. * Remove the logic that disables ilverify when a test uses the unsafe argument. Diffing makes this obsolete. * IL Verification errors have been greatly improved. ** will now output all IL errors in the failure message rather than just the first invalid IL. ** Type and Method names are now displayed in the error message. I didn't do an exhaustive implementation, SRM is so tedious to use, but it's better than not having it ** Tokens and Offsets are formatted nicely * Extension points opened up for Unity. ** We need to supply different search directories. ** We need to search for `.winmd` files since we support windows runtime. ** In general I opened some things up in case we need to call them

mrvoorhe · 2023-08-17T18:07:39Z

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ILVerification/ILVerifier.cs

+		return true;
+	}
+
+	protected string[] PossibleAssemblyExtensions => new[] { ".dll", ".exe", ".winmd" };


I added .winmd here as well.

vitek-karas · 2023-08-18T14:38:14Z

The test failures are either known or unrelated.

mrvoorhe requested a review from marek-safar as a code owner August 11, 2023 18:10

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 11, 2023

mrvoorhe changed the title ~~Redo ILVerfificaiton~~ Redo IL Verification Aug 11, 2023

ghost added linkable-framework Issues associated with delivering a linker friendly framework community-contribution Indicates that the PR has been added by a community member labels Aug 11, 2023

jkotas added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Aug 11, 2023

jkotas removed the linkable-framework Issues associated with delivering a linker friendly framework label Aug 11, 2023

ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Aug 11, 2023

mrvoorhe force-pushed the linker-redo-ilverifier branch from 39e8430 to e52903d Compare August 11, 2023 19:58

build-analysis bot mentioned this pull request Aug 11, 2023

Insufficient memory of docker containers on CI dotnet/dnceng#450

Open

3 tasks

jkoritzinsky removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners linkable-framework Issues associated with delivering a linker friendly framework labels Aug 11, 2023

ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Aug 11, 2023

mrvoorhe force-pushed the linker-redo-ilverifier branch from e52903d to 2fb5f82 Compare August 14, 2023 18:07

mrvoorhe force-pushed the linker-redo-ilverifier branch from 2fb5f82 to d2f1edb Compare August 16, 2023 21:04

vitek-karas approved these changes Aug 17, 2023

View reviewed changes

vitek-karas requested a review from sbomer August 17, 2023 15:51

sbomer approved these changes Aug 17, 2023

View reviewed changes

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ILVerification/ILChecker.cs Outdated Show resolved Hide resolved

src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs Show resolved Hide resolved

mrvoorhe force-pushed the linker-redo-ilverifier branch from d2f1edb to 07ccd69 Compare August 17, 2023 17:35

mrvoorhe force-pushed the linker-redo-ilverifier branch from 07ccd69 to 9a0bae5 Compare August 17, 2023 18:07

mrvoorhe commented Aug 17, 2023

View reviewed changes

build-analysis bot mentioned this pull request Aug 17, 2023

BinderTracingTest_ResolutionFlow test fails #90593

Closed

vitek-karas merged commit 6229f5f into dotnet:main Aug 18, 2023
127 of 132 checks passed

ghost locked as resolved and limited conversation to collaborators Sep 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redo IL Verification #90418

Redo IL Verification #90418

mrvoorhe commented Aug 11, 2023 •

edited

Loading

ghost commented Aug 11, 2023

ghost commented Aug 11, 2023

ghost commented Aug 11, 2023

mrvoorhe commented Aug 14, 2023 •

edited

Loading

vitek-karas commented Aug 16, 2023

vitek-karas commented Aug 16, 2023

mrvoorhe commented Aug 16, 2023

mrvoorhe commented Aug 16, 2023 •

edited

Loading

mrvoorhe commented Aug 17, 2023

vitek-karas commented Aug 17, 2023

mrvoorhe commented Aug 17, 2023

vitek-karas left a comment

vitek-karas Aug 17, 2023

vitek-karas Aug 17, 2023

mrvoorhe Aug 17, 2023

mrvoorhe Aug 17, 2023

vitek-karas commented Aug 18, 2023

Redo IL Verification #90418

Redo IL Verification #90418

Conversation

mrvoorhe commented Aug 11, 2023 • edited Loading

ghost commented Aug 11, 2023

ghost commented Aug 11, 2023

ghost commented Aug 11, 2023

mrvoorhe commented Aug 14, 2023 • edited Loading

vitek-karas commented Aug 16, 2023

vitek-karas commented Aug 16, 2023

mrvoorhe commented Aug 16, 2023

mrvoorhe commented Aug 16, 2023 • edited Loading

mrvoorhe commented Aug 17, 2023

vitek-karas commented Aug 17, 2023

mrvoorhe commented Aug 17, 2023

vitek-karas left a comment

Choose a reason for hiding this comment

vitek-karas Aug 17, 2023

Choose a reason for hiding this comment

vitek-karas Aug 17, 2023

Choose a reason for hiding this comment

mrvoorhe Aug 17, 2023

Choose a reason for hiding this comment

mrvoorhe Aug 17, 2023

Choose a reason for hiding this comment

vitek-karas commented Aug 18, 2023

mrvoorhe commented Aug 11, 2023 •

edited

Loading

mrvoorhe commented Aug 14, 2023 •

edited

Loading

mrvoorhe commented Aug 16, 2023 •

edited

Loading