[ILVerify] Define and Implement a public API surface

[gregkalapos]

Currently basically everything is internal in ILVerify.

Having a public API surface is needed for at least two things:

• Supporting a .dll flavor, so it can be embedded into any .NET application
• Creating tests without exposing every internal data structure to the test code. (see [ILVerify] Added first tests #3725 (comment))

Step1: would be: let's discuss what we want to expose as public API
Step2: implement it. I'm happy to do that.

[leppie]

Isn't ILVerify a COM thing? Or has it been ported to managed code (which would be super awesome!)?

Edit: I see that work is being done! https://github.com/dotnet/corert/tree/master/src/ILVerify (not sure it is living in the correct place though)

[gregkalapos]

@leppie I guess what you mean by "COM thing" is PEVerify. And yes, your guess is basically correct: ILVerify will be a managed port of that (...well not 100% port... ). Look here: https://github.com/dotnet/corert/tree/master/src/ILVerify

not sure it is living in the correct place though

Well, I'm not the one making these kinds of decisions, and honestly I don't have a strong opinion on where this should be, but one thing I can say: we need an infrastructure to parse and "understand" .NET assemblies and that infrastructure is definitely given in this repo.

[jcouv]

As part of working on integrating ILVerify into the test framework for Roslyn, I prototyped a simple API (note: this is not a PR for review, I left a number of temporary hacks in there).

Basically, a new public type (Verifier) which can be constructed or created, and has two methods:

• bool AddModule(ImmutableArray<byte> image, StringBuilder output)
• bool VerifyModule(string moduleToVerify, StringBuilder output) // moduleToVerify is the simple name for one of the modules previously added with AddModule

I think there is no need for a SetSystemModule, as the core library can be detected (it has no assembly references).
APIs for loading images from the filesystem could be added.

If this API sounds alright as a starting point, I'll make a PR. Thanks

Tagging @ArztSamuel @VSadov

[jkotas]

We should build the .exe tool and tests on top of the same API. It will ensure that the API is reasonably flexible, and that we don't have to redo it from the ground up multiple times.

I do not think that the API should take the modules as ImmutableArray<byte> upfront. It implies that the caller of the API has to open all potential files that the verification may need and read them into memory. It would be bad for performance for larger assembly graphs. I think that the API should allow lazy resolution of the assemblies and/or allow passing in more efficient representations of the metadata, like System.Reflection.PEReader.

[VSadov]

I agree with not requiring the caller to read/resolve.
But there is also a case when we have the set if all possible/expected dependencies already in a form of byte arrays. I think this case should be allowed.

Peehaps verifier ahould take abstract resolver as an input and a trivial implementation of resolver would be a bag pre-filled with binaries.

[jcouv]

Here's a second proposal for API. Let me know what you think.

interface IResolver
{
    PEReader Resolve(string name);
}
class Verifier
{
    Verifier(IResolver resolver);
    VerificationResult Verify(string[] names);
}
class VerificationResult
{
    bool Success;
    string Message;
}

Note: I'd suggest that name mean full name or identity, but internally ILVerify currently looks up references by simple name (for instance, just "mscorlib" without a version number or public token).

[jkotas]

I'd suggest that name mean full name or identity

So should it be AssemblyName then? The lookup in ILVerify does actually start with AssemblyName - it is just using simple name out of the AssemblyName for the actual lookup.

string[] names

What are these names? I would think that we should have Verify(PEReader, MethodDefinitionHandle) to verify single method.

For convenience, we can also have Verify(PEReader) to verify single assembly and maybe Verify(PEReader, TypeDefinitionHandle) to verify single type.

[jkotas]

Also, we need a way to specify system assembly.

[ArztSamuel]

I think it would make sense to change it to:

PEReader Resolve(AssemblyName name);

I would think that we should have Verify(PEReader, MethodDefinitionHandle) to verify single method.

I also think that it makes sense to have mutiple overloads of Verify, however resolving the AssemblyName to a PEReader should be done by the Verifier internally using IResolver, right? Therefore we wouldn't have a PEReader or MethodDefinition/TypeDefinitionHandle object from outside.

Also, we need a way to specify system assembly.

As @jcouv mentioned earlier, the system assembly could be detected automatically, since it does not reference any other assemblies. However, that would require resolving all assemblies first, which defeats the whole purpose of the IResolver, right?
Would it make sense to simply define an additional GetSystemAssembly() for the IResolver? Depending on the actual implementation, this could then automatically determine the system assembly (for when all assemblies are actually already in memory) or define a setter for an implementation using lazy resolution (e.g. SetSystemAssembly(AssemblyName name)).

As far as the verification result is concerned:
We currently use a callback for when a VerificationError is reported, in order to be able to progressively print the errors to console (see: https://github.com/dotnet/corert/blob/master/src/ILVerify/src/Program.cs#L124). Would it make sense to also have the ability to subscribe to a callback for this purpose?
I also think that it would make sense to have a more detailed VerificationResult, rather than just the message as a string. Would it make sense to make the existing VerificationErrorArgs struct and the VerifierError enum public for this?

[jkotas]

however resolving the AssemblyName to a PEReader should be done by the Verifier internally using IResolver

If the API takes PEReader and caller has AssemblyName, the caller can call resolver like: verifier.Verify(resolver.Resolve(assemblyName)). It would not be as easy the other way if the API takes AssemblyName and caller has PEReader.

The API that takes PEReader is the more core one. I think we should have it. If we actually see callers that have AssemblyName, we can make it a bit easier for them by introducing convenience overload that just does Verify(_resolver(assemblyName)).

[jcouv]

So should it be AssemblyName then?

I don't see how ILVerify.exe can get those assembly names without cracking the files open. I suspect that is the main reason why short names are used now: the file name is assumed to match the simple name of the assembly, so it is used as an identifier.
When time comes to actually load the file, that assumption is checked.

[ArztSamuel]

I don't see how ILVerify.exe can get those assembly names without cracking the files open.

That is true. But I guess that is the responsibility of the IResolver then, not ILVerify.
I think it makes sense to make the interface use AssemblyName. Whether the implementation of IResolver then simply assumes filename = simple name or whether it actually compares the full information of the AssemblyName is irrelevant for ILVerify, right?

[jcouv]

PR for review at #5186