Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add IL verification with ILVerify in addition to PEVerify #37994

Merged
merged 54 commits into from
Feb 11, 2022
Merged

Add IL verification with ILVerify in addition to PEVerify #37994

merged 54 commits into from
Feb 11, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Aug 14, 2019 •
edited
Loading

Relates to #22872

@jcouv jcouv self-assigned this Aug 14, 2019
@jcouv jcouv force-pushed the il-verify2 branch 4 times, most recently from 387a56d to f60966d Compare September 13, 2019 00:51
@jcouv jcouv closed this Dec 19, 2019
@jcouv jcouv reopened this Dec 19, 2019
@jcouv 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
@jcouv jcouv mentioned this pull request Feb 8, 2022
Closed
cston
cston reviewed Feb 8, 2022
View reviewed changes
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)
{
Copy link
Member

@cston cston Feb 8, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the catch block is unnecessary. #Resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jcouv
Revert "remove catch"
f5387f8 
This reverts commit 7e0ec94.
This was referenced Feb 9, 2022
Closed
Closed
cston
cston reviewed Feb 9, 2022
View reviewed changes
src/Compilers/Test/Core/CompilationVerifier.cs Outdated
string message = printVerificationResult(result);
throw new Exception("IL Verify failed unexpectedly: \r\n" + message);
}
catch (Exception)
Copy link
Member

@cston cston Feb 9, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
cston reviewed Feb 9, 2022
View reviewed changes
src/Compilers/Test/Core/CompilationVerifier.cs Outdated
return;
}

throw new Exception("IL Verify succeeded unexpectedly");
Copy link
Member

@cston cston Feb 9, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new Exception

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cston
cston reviewed Feb 9, 2022
View reviewed changes
src/Compilers/Test/Core/CompilationVerifier.cs Outdated
var resolver = new Resolver(_allModuleData);
var verifier = new ILVerify.Verifier(resolver);

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

@cston cston Feb 9, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
cston reviewed Feb 9, 2022
View reviewed changes
src/Compilers/Test/Core/CompilationVerifier.cs Outdated
string name = module.SimpleName;
if (_imagesByName.ContainsKey(name))
{
throw new Exception($"Multiple modules named '{name}' were found");
Copy link
Member

@cston cston Feb 9, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jcouv jcouv marked this pull request as draft February 9, 2022 20:07
@cston cston self-requested a review February 10, 2022 04:19
cston
cston reviewed Feb 10, 2022
View reviewed changes
src/Compilers/Test/Core/CompilationVerifier.cs Outdated
@@ -228,6 +233,127 @@ public void VerifyTypeIL(string typeName, string expected)
}
}

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

@cston cston Feb 10, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

The initial value is overwritten in the constructor. #Resolved

cston
cston reviewed Feb 10, 2022
View reviewed changes
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);
Copy link
Member

@cston cston Feb 10, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mscorlibModule.SimpleName

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

cston
cston reviewed Feb 10, 2022
View reviewed changes
src/Compilers/Test/Core/CompilationVerifier.cs Outdated
if (result.Count() != 0)
{
return (false, printVerificationResult(result));
}
Copy link
Member

@cston cston Feb 10, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Count() must be inside.

@jcouv jcouv marked this pull request as ready for review February 11, 2022 02:35
@RikkiGibson RikkiGibson merged commit 262a650 into dotnet:main Feb 11, 2022
@ghost ghost added this to the Next milestone Feb 11, 2022
@jcouv jcouv deleted the il-verify2 branch February 11, 2022 05:46
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkotas jkotas jkotas left review comments

@jaredpar jaredpar jaredpar approved these changes

@cston cston cston approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers
Projects
None yet
Milestone
17.2.P2
Development

Successfully merging this pull request may close these issues.
5 participants
@jcouv @jaredpar @jkotas @cston @RikkiGibson