Add IL verification with ILVerify in addition to PEVerify by jcouv · Pull Request #37994 · dotnet/roslyn

jcouv · 2019-08-14T21:08:23Z

Relates to #22872

cston · 2022-02-08T20:52:19Z

src/Compilers/Test/Core/CompilationVerifier.cs

+                // Main module is the first one
+                var result = verifier.Verify(resolver.Resolve(_allModuleData[0].SimpleName));
+                if (result.Count() == 0)
+                {


It looks like the catch block is unnecessary. #Resolved

Great catch 😄

Can't remove the catch. Tried again just to be sure:

Sorry, I'd missed the fact that Verify() can throw exceptions. In that case, consider having the try { } catch { } wrap the Verify() call only, and catch specific exceptions rather than all.

This reverts commit 7e0ec94.

cston · 2022-02-09T16:38:24Z

src/Compilers/Test/Core/CompilationVerifier.cs

+                string message = printVerificationResult(result);
+                throw new Exception("IL Verify failed unexpectedly: \r\n" + message);
+            }
+            catch (Exception)


Exception

By catching all exceptions, aren't we potentially hiding bugs? For instance, Microsoft.CodeAnalysis.VisualBasic.UnitTests.CodeGenTests.TestGetObjectValueCalls() seems to fail ILVerify() because of a NullReferenceException. #Resolved

From previous discussion with Jared, we don't distinguish between the different failure modes of ILVerify. Either it succeeds or it fails. That's why I added a comment to the test:

' ILVerify null ref ' Tracked by https//github.com/dotnet/roslyn/issues/58652

cston · 2022-02-09T16:45:17Z

src/Compilers/Test/Core/CompilationVerifier.cs

+                        return;
+                    }
+
+                    throw new Exception("IL Verify succeeded unexpectedly");


throw new Exception

Won't the outer catch clause simply return without failing with if ((verification & Verification.FailsILVerify) != 0) { return ; }?
#Resolved

There was indeed a bug here. Thanks. Fixed by reducing usage of exceptions for failure scenarios.

cston · 2022-02-09T16:55:21Z

src/Compilers/Test/Core/CompilationVerifier.cs

+                var resolver = new Resolver(_allModuleData);
+                var verifier = new ILVerify.Verifier(resolver);
+
+                var mscorlibModule = _allModuleData.Single(m => m.IsCorLib);


var mscorlibModule = _allModuleData.Single(m => m.IsCorLib);

Consider moving outside of try { } catch { }. (It looks like this throws an InvalidOperationException from Microsoft.CodeAnalysis.CSharp.UnitTests.UseSiteErrorTests.MissingTypeKindBasisTypes() for instance.) #Resolved

cston · 2022-02-09T17:01:31Z

src/Compilers/Test/Core/CompilationVerifier.cs

+                    string name = module.SimpleName;
+                    if (_imagesByName.ContainsKey(name))
+                    {
+                        throw new Exception($"Multiple modules named '{name}' were found");


Consider constructing the Dictionary<,> in the caller, without throwing an exception. #Resolved

cston · 2022-02-10T16:16:36Z

src/Compilers/Test/Core/CompilationVerifier.cs


+        private sealed class Resolver : ILVerify.ResolverBase
+        {
+            private readonly Dictionary<string, ImmutableArray<byte>> _imagesByName = new Dictionary<string, ImmutableArray<byte>>(StringComparer.OrdinalIgnoreCase);


= new Dictionary<string, ImmutableArray>(StringComparer.OrdinalIgnoreCase)

The initial value is overwritten in the constructor. #Resolved

cston · 2022-02-10T16:19:18Z

src/Compilers/Test/Core/CompilationVerifier.cs

+            // Main module is the first one
+            var mainModuleReader = resolver.Resolve(_allModuleData[0].SimpleName);
+
+            var (succeeded, errorMessage) = verify(verifier, mscorlibModule.SimpleName, mainModuleReader);


mscorlibModule.SimpleName

If (verification & Verification.FailsILVerify) == 0, mscorlibModule may be null. #Resolved

cston · 2022-02-10T16:21:08Z

src/Compilers/Test/Core/CompilationVerifier.cs

+                    if (result.Count() != 0)
+                    {
+                        return (false, printVerificationResult(result));
+                    }


Consider moving this outside the try { } catch { }. #ByDesign

The Count() must be inside.

jcouv self-assigned this Aug 14, 2019

jcouv added the Area-Compilers label Aug 14, 2019

jcouv force-pushed the il-verify2 branch 4 times, most recently from 387a56d to f60966d Compare September 13, 2019 00:51

jcouv closed this Dec 19, 2019

jcouv reopened this Dec 19, 2019

jcouv and others added 3 commits December 18, 2019 20:57

Add IL verification with ILVerify in addition to PEVerify

d1e402c

Annotating more tests

70d3ab8

Use new package with strong name

3f8b70f

jcouv force-pushed the il-verify2 branch from ad0be0b to 3f8b70f Compare December 19, 2019 04:59

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 8, 2020

Base automatically changed from master to main March 3, 2021 23:51

Julien Couvreur added 16 commits December 30, 2021 10:09

Merge remote-tracking branch 'dotnet/main' into il-verify2

6e2efce

Fix a couple more tests

bf116d9

Address some PROTOTYPE markers

a993d0e

Tighten checks

7af8ecc

Fix IL verification plumbing

0ee5f6a

Transition ILVerify to Core test runner

1701dbb

Fix Resolver

260a40e

public

7b0263a

Start tightening the IL verification

400d315

Remove some unused error entries

e7a7c3f

Annotate failures

9cafb53

InitOnly and NotVisible

656d328

MissingStringType

eea70d3

MissingAssembly

d328e27

BadReturnType

ebbc74a

More

66aa35b

indentation

cb27c93

jcouv mentioned this pull request Feb 8, 2022

ILVerification fails unexpectedly on delegate construction dotnet/runtime#65010

Closed

Julien Couvreur added 2 commits February 8, 2022 12:17

Merge remote-tracking branch 'dotnet/main' into il-verify2

a11ad37

Resolve conflicts

e32bf0a

cston reviewed Feb 8, 2022

View reviewed changes

remove catch

7e0ec94

cston approved these changes Feb 9, 2022

View reviewed changes

Revert "remove catch"

f5387f8

This reverts commit 7e0ec94.

This was referenced Feb 9, 2022

Flaky test: Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.FastDoubleInvoke #57551

Closed

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

cston reviewed Feb 9, 2022

View reviewed changes

Shrink usage of exceptions

f9cf399

jcouv marked this pull request as draft February 9, 2022 20:07

Julien Couvreur added 2 commits February 9, 2022 16:44

Adjust to new logic

c55a61d

one more test

fcc2dc2

cston self-requested a review February 10, 2022 04:19

cston reviewed Feb 10, 2022

View reviewed changes

Address feedback

3db3d54

cston approved these changes Feb 10, 2022

View reviewed changes

jcouv marked this pull request as ready for review February 11, 2022 02:35

RikkiGibson merged commit 262a650 into dotnet:main Feb 11, 2022

ghost added this to the Next milestone Feb 11, 2022

jcouv deleted the il-verify2 branch February 11, 2022 05:46

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022

Conversation

jcouv commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Feb 8, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

cston Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

cston Feb 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

cston Feb 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

cston Feb 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Feb 9, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Feb 10, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Feb 10, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Feb 10, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcouv commented Aug 14, 2019 •

edited

Loading

cston Feb 8, 2022 •

edited by jcouv

Loading

cston Feb 9, 2022 •

edited by jcouv

Loading

cston Feb 9, 2022 •

edited by jcouv

Loading

cston Feb 9, 2022 •

edited by jcouv

Loading

cston Feb 9, 2022 •

edited by jcouv

Loading

cston Feb 10, 2022 •

edited by jcouv

Loading

cston Feb 10, 2022 •

edited by jcouv

Loading

cston Feb 10, 2022 •

edited by jcouv

Loading