dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.04k stars 4.68k forks source link

[API Proposal]: Add PersistedAssemblyBuilder type #97015

Closed buyaa-n closed 6 months ago

buyaa-n commented 8 months ago

Background and motivation

We added a new persisted AssemblyBuilder implementation and related new APIs in .NET 9 preview 1. While discussing adding the SetEntryPoint method questions raised for adding SetEntryPoint(MethodInfo, PEFileKinds) the overload that sets PEFileKinds option.

Further we would need to add more options that used in .NET framework, like PortableExecutableKinds or ImageFileMachine that used to set with Save(String, PortableExecutableKinds, ImageFileMachine) overload, plus would need to add APIs for setting AddResourceFile, DefineResource, DefineUnmanagedResource.

Instead of adding all old .NET framework APIs we prefer to let the customers handle their assembly building process by themselves using the PEHeaderBuilder and System.Reflection.PortableExecutable.PEBuilder implementation ManagedPEBuilder-system-reflection-metadata-blobcontentid)))) options. https://github.com/dotnet/runtime/blob/27214a0787f9a0ee95c799639431d9d16c25d1ac/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEHeaderBuilder.cs#L46-L65 and/or https://github.com/dotnet/runtime/blob/27214a0787f9a0ee95c799639431d9d16c25d1ac/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/ManagedPEBuilder.cs#L37-L48

These options are used to produce the assembly and allow users to configure it any way they want. These covers all options that existed in .NET framework, (but not exactly same way), plus provide many other options that not available in .NET framework that customers may want to set on the final binary.

In order to achieve this, we would need to provide all metadata information produced with Reflection.Emit APIs (MetadataBuilder and ILStreams) so that user could embed them into the corresponding section of PEBuidler. But because the MetadataBuilder and BlobBuilder types are not accessible from within CoreLib we decided to make the PersistedAssemblyBuilder type public and reintroduce the related new APIs there, remove the new APIs from the base AssemblyBuilder type.

API Proposal

namespace System.Reflection.Emit;

public abstract partial class AssemblyBuilder
{
    // Previously approved new APIs
-   public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

-   public void Save(Stream stream);
-   public void Save(string assemblyFileName);
-   protected abstract void SaveCore(Stream stream);
}

+public sealed class PersistedAssemblyBuilder : AssemblyBuilder
{
+   public PersistedAssemblyBuilder(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

+   public void Save(Stream stream);
+   public void Save(string assemblyFileName);
+   public MetadataBuilder GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder mappedFieldData);
}

API Usage

PersistedAssemblyBuilder ab = new PersistedAssemblyBuilder(new AssemblyName("MyAssembly"), typeof(object).Assembly);
TypeBuilder tb = ab.DefineDynamicModule("MyModule").DefineType("MyType", TypeAttributes.Public | TypeAttributes.Class);
// ...
MethodBuilder entryPoint = tb.DefineMethod("Main", MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static);
ILGenerator il2 = entryPoint.GetILGenerator();
// ...
il2.Emit(OpCodes.Ret);
tb.CreateType();

MetadataBuilder metadataBuilder = ab.GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder fieldData);
PEHeaderBuilder peHeaderBuilder = new PEHeaderBuilder(
                // Header for PEFileKinds.WindowApplication imageCharacteristics and subsystem need to be set
                imageBase: 0x00400000, // this is the default value, was not necessarily needed to set.
                imageCharacteristics: Characteristics.ExecutableImage,
                subsystem: Subsystem.WindowsGui);

ManagedPEBuilder peBuilder = new ManagedPEBuilder(
                header: peHeaderBuilder,
                metadataRootBuilder: new MetadataRootBuilder(metadataBuilder),
                ilStream: ilStream,
                mappedFieldData: fieldData,
                entryPoint: MetadataTokens.MethodDefinitionHandle(entryPoint.MetadataToken));

BlobBuilder peBlob = new BlobBuilder();
peBuilder.Serialize(peBlob);

// in case saving to a file:
using var fileStream = new FileStream("MyAssembly.exe", FileMode.Create, FileAccess.Write);
peBlob.WriteContentTo(fileStream); 

Old AssemblyBuilder.SetEntryPoint(MethodInfo) Proposal folded below:

Add AssemblyBuilder.SetEntryPoint(MethodInfo) method ### Background and motivation As a new [persist able AssemblyBuilder](https://github.com/dotnet/runtime/issues/83988#issuecomment-1487355324) implementation is being added in .NET 9 it also needs an [API for setting the Entry point](https://github.com/dotnet/runtime/issues/92975). We could add similar but virtual [API we have in .NET framework](https://learn.microsoft.com/dotnet/api/system.reflection.emit.assemblybuilder.setentrypoint?view=netframework-4.8.1). ### API Proposal ```diff namespace System.Reflection.Emit; public abstract partial class AssemblyBuilder { // Previously approved new APIs public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable? assemblyAttributes = null); public void Save(Stream stream); public void Save(string assemblyFileName); protected abstract void SaveCore(Stream stream); + public virtual void SetEntryPoint(MethodInfo entryMethod); } ``` ### API Usage ```csharp AssemblyBuilder ab = AssemblyBuilder.DefinePersistedAssembly(new AssemblyName("MyAssembly"), typeof(object).Assembly, null); TypeBuilder tb = ab.DefineDynamicModule("MyModule").DefineType("MyType", TypeAttributes.Public | TypeAttributes.Class); MethodBuilder mb1 = tb.DefineMethod("SumMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(int), [typeof(int), typeof(int)]); ILGenerator il = mb1.GetILGenerator(); il.Emit(OpCodes.Ldarg_0); il.Emit(OpCodes.Ldarg_1); il.Emit(OpCodes.Add); il.Emit(OpCodes.Ret); MethodBuilder main = tb.DefineMethod("Main", MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static); ILGenerator il2 = main.GetILGenerator(); il2.Emit(OpCodes.Ldc_I4_S, 10); il2.Emit(OpCodes.Ldc_I4_1); il2.Emit(OpCodes.Call, mb1); il2.Emit(OpCodes.Call, typeof(Console).GetMethod("WriteLine", [typeof(int)])!); il2.Emit(OpCodes.Ret); tb.CreateType(); ab.SetEntryPoint(main); ab.Save("MyAssembly.exe"); ``` ### Alternative Designs Or keep the same pattern (have protected virtual `*Core` method). ```diff namespace System.Reflection.Emit; public abstract partial class AssemblyBuilder { // Previously approved new APIs public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable? assemblyAttributes = null); public void Save(Stream stream); public void Save(string assemblyFileName); protected abstract void SaveCore(Stream stream); + public void SetEntryPoint(MethodInfo entryMethod); + protected virtual void SetEntryPointCore(MethodInfo entryMethod); } ```
ghost commented 8 months ago

Tagging subscribers to this area: @dotnet/area-system-reflection-emit See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation As a new [persist able AssemblyBuilder](https://github.com/dotnet/runtime/issues/83988#issuecomment-1487355324) implementation is being added in .NET 9 it also needs an [API for setting the Entry point](https://github.com/dotnet/runtime/issues/92975). We could add the same API we have in .NET framework. ### API Proposal ```diff namespace System.Reflection.Emit; public partial class AssemblyBuilder { // Previously approved new APIs public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable? assemblyAttributes = null); public void Save(Stream stream); public void Save(string assemblyFileName); protected abstract void SaveCore(Stream stream); + public void SetEntryPoint(MethodInfo entryMethod); } ``` ### API Usage ```csharp AssemblyBuilder ab = AssemblyBuilder.DefinePersistedAssembly(new AssemblyName("MyAssembly"), typeof(object).Assembly, null); TypeBuilder tb = ab.DefineDynamicModule("MyModule").DefineType("MyType", TypeAttributes.Public | TypeAttributes.Class); MethodBuilder mb1 = tb.DefineMethod("SumMethod", MethodAttributes.Public | MethodAttributes.Static, typeof(int), [typeof(int), typeof(int)]); ILGenerator il = mb1.GetILGenerator(); il.Emit(OpCodes.Ldarg_0); il.Emit(OpCodes.Ldarg_1); il.Emit(OpCodes.Add); il.Emit(OpCodes.Ret); MethodBuilder main = tb.DefineMethod("Main", MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static); ILGenerator il2 = main.GetILGenerator(); il2.Emit(OpCodes.Ldc_I4_S, 10); il2.Emit(OpCodes.Ldc_I4_1); il2.Emit(OpCodes.Call, mb1); il2.Emit(OpCodes.Call, typeof(Console).GetMethod("WriteLine", [typeof(int)])!); il2.Emit(OpCodes.Ret); tb.CreateType(); ab.SetEntryPoint(main); ab.Save("MyAssembly.exe"); ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: buyaa-n
Assignees: -
Labels: `api-suggestion`, `area-System.Reflection.Emit`, `untriaged`
Milestone: -
jkotas commented 8 months ago

public void SetEntryPoint(MethodInfo entryMethod)

Does it need to be virtual?

buyaa-n commented 8 months ago

Does it need to be virtual?

Yes, it needs to be virtual, thank you. Updated the proposal to public virtual void SetEntryPoint(...), or can be protected virtual SetEntryPointCore(...) that will be called from the public void SetEntryPoint(...).

reflectronic commented 8 months ago

Is there a reason why SetEntryPoint(MethodInfo, PEFileKinds) from .NET Framework is not also being added? I notice that Save(string, PortableExecutableKinds, ImageFileMachine) is missing from the AssemblyBuilder proposal too. These overloads are needed for building certain kinds of .NET Framework assemblies. Is it expected that AssemblyBuilder.Save in .NET 9 will not support this?

buyaa-n commented 8 months ago

PEFileKinds doesn't exist in .NET core and doesn't really have anything useful.

For now, the assembly is DLL only if no entry point set, if user needs to set more than this we can consider adding API for setting PEHeaderBuilder in the future, but that option doesn't have to be associated with SetEntryPoint or Save method

These overloads are needed for building certain kinds of .NET Framework assemblies. Is it expected that AssemblyBuilder.Save in .NET 9 will not support this?

Please try out the default implementation and let us know what exact scenarios is not covered, and also check PEHeaderBuilder if there is an option that could cover your scenarios and let us know if there is anything not covered.

bartonjs commented 7 months ago

Video

namespace System.Reflection.Emit;

+ public enum PEFileKinds
+ {
+     Dll                = 0x0001,
+     ConsoleApplication = 0x0002,
+     WindowApplication = 0x0003,
+ }

public abstract partial class AssemblyBuilder
{
    // Previously approved new APIs
    public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

    public void Save(Stream stream);
    public void Save(string assemblyFileName);
    protected abstract void SaveCore(Stream stream);
+   public void SetEntryPoint(MethodInfo entryMethod);
+   public void SetEntryPoint(MethodInfo entryMethod, PEFileKinds fileKind);
+   protected virtual void SetEntryPointCore(MethodInfo entryMethod, PEFileKinds fileKind);
}
jkotas commented 7 months ago

PEFileKinds is not very future proof. There are all sorts of other bits one may want to set on the final binary. The future proof solution would be to give the caller access to System.Reflection.PortableExecutable.PEBuilder that is used to produce the binary and allow them to edit it any way they want.

buyaa-n commented 7 months ago

PEFileKinds is not very future proof. There are all sorts of other bits one may want to set on the final binary.

Completely agree, PEFileKinds is not enough to cover all options can be set by the user. We should not add a new API for each specific use case

The future proof solution would be to give the caller access to System.Reflection.PortableExecutable.PEBuilder that is used to produce the binary and allow them to edit it any way they want.

The PEBuilder doesn't seem to provide much options to edit the PR file. PeBuilder.Header returns readonly PEHeaderBuilder, I guess people can use PEDirectoriesBuilder from PEBuilder.GetDirectories to set some options, not sure it could cover PEHeader options

We can't add option to set PEBuilder instance, as there is no API that gives access to the internal MetadataBuilder and BlobBuilders for writing method IL, might could use PEDirectoriesBuilder looks very low level API difficult to use.

I am thinking to propose options for setting PEHeaderBuilder and CoreFlags, their combination cover all options used to set with PEFileKinds and PortableExecutableKinds, ImageFileMachine options used to set with Save(string, PortableExecutableKinds, ImageFileMachine) overload. Plus of course cover more options that can be set in PEHeader and PEBuilder what you think?

jkotas commented 7 months ago

I am thinking to propose options for setting PEHeaderBuilder and CoreFlags,

What would the API look like? Would one be able to also define resources through this API (discussed in https://github.com/dotnet/runtime/issues/15704#issuecomment-1950300514)?

I am wondering whether it would be better to introduce a variant of the Save API that returns metadataBuilder, ilStream and mappedFieldData, and allows the user to wrap it in a fully custom PE envelope..

buyaa-n commented 7 months ago

What would the API look like? Would one be able to also define resources through this API (discussed in https://github.com/dotnet/runtime/issues/15704#issuecomment-1950300514)?

I was thinking to add an overload for Save with defaulted options, instead of adding separate API for each.

I am wondering whether it would be better to introduce a variant of the Save API that returns metadataBuilder, ilStream and mappedFieldData, and allows the user to wrap it in a fully custom PE envelope.

That is a great idea, as including the resources it's getting too many options to set.

buyaa-n commented 7 months ago

API Proposal

As per above discussion the new API could look like this:

public abstract partial class AssemblyBuilder
{
    // Existing APIs
    public void Save(Stream stream) { }
    protected virtual void SaveCore(Stream stream) { }
+   public virtual MetadataBuilder GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder mappedFieldData) { }
}

API Usage

AssemblyBuilder ab = AssemblyBuilder.DefinePersistedAssembly(new AssemblyName("MyAssembly"), typeof(object).Assembly, null);
TypeBuilder tb = ab.DefineDynamicModule("MyModule").DefineType("MyType", TypeAttributes.Public | TypeAttributes.Class);
// ...
MethodBuilder entryPoint = tb.DefineMethod("Main", MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static);
ILGenerator il2 = main.GetILGenerator();
// ...
il2.Emit(OpCodes.Ret);
tb.CreateType();

MetadataBuilder metadataBulder = ab.GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder fieldData);
PEHeaderBuilder peHeaderBuilder = PEHeaderBuilder.CreateExecutableHeader();

ManagedPEBuilder peBuilder = new ManagedPEBuilder(
    header: peHeaderBuilder,
    metadataRootBuilder: new MetadataRootBuilder(metadataBuilder),
    ilStream: ilStream,
    mappedFieldData: fieldData,
    entryPoint: MetadataTokens.EntityHandle(entryPoint.MetadataToken));

BlobBuilder peBlob = new BlobBuilder();
peBuilder.Serialize(peBlob);

// in case saving to a file:
using var fileStream = new FileStream("MyAssembly.exe", FileMode.Create, FileAccess.Write);
peBlob.WriteContentTo(fileStream); 

I assume we don't want to add this SetEntryPoint(MethodInfo) method overloads anymore, so did not create a new issue, probably better to use same issue for the context. @jkotas PTAL

jkotas commented 7 months ago

The general idea looks good. I do not think it can be a virtual method on AssemblyBuilder. The problem is that AssemblyBuilder lives in CoreLib and MetadataBuilder lives in System.Reflection.Metadata. It is not possible to take a dependency on System.Reflection.Metadata in CoreLib.

There are a few options that I can think of:

Option 1 (I think that this is the cleanest option):

class PersistedAssemblyBuilder : AssemblyBuilder
{
     public MetadataBuilder GenerateMetadata(...);
     public void Save(...);
}

Option 2:

Option 3:

For the purpose of API discussion, you can enhance the sample to show how one would set a custom properties like StackSize, BaseSize, or the equivalent of PEFileKinds.WindowApplication.

I assume we don't want to add this SetEntryPoint(MethodInfo) method overloads anymore,

Yes, I agree, SetEntryPoint would be just an extra convenience on top of GenerateMetadata to save a few extra lines. It is likely that users who want to generate executable with entrypoint will want to use GenerateMetadata to customize other properties anyway.

stephentoub commented 7 months ago

I like Option 1 even without the cited issues, as it moves Save from somewhere it will typically throw to somewhere it will typically work. Presumably the members on PersistedAssemblyBuilder would still need to be virtual if we want to support extensibility of it just as we do with AssemblyBuilder?

buyaa-n commented 7 months ago

I do not think it can be a virtual method on AssemblyBuilder. The problem is that AssemblyBuilder lives in CoreLib and MetadataBuilder lives in System.Reflection.Metadata. It is not possible to take a dependency on System.Reflection.Metadata in CoreLib.

Good point, forgot that, thank you! I will add another proposal incorporating all suggestions.

jkotas commented 7 months ago

Presumably the members on PersistedAssemblyBuilder would still need to be virtual if we want to support extensibility

I think PersistedAssemblyBuilder should be a sealed type with no extensibility.

buyaa-n commented 7 months ago

API Proposal

namespace System.Reflection.Emit;

public abstract partial class AssemblyBuilder
{
    // Previously approved new APIs
-   public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

-   public void Save(Stream stream);
-   public void Save(string assemblyFileName);
-   protected abstract void SaveCore(Stream stream);
}

+public sealed class PersistedAssemblyBuilder : AssemblyBuilder
{
+   public PersistedAssemblyBuilder(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

+   public void Save(Stream stream);
+   public void Save(string assemblyFileName);
+   public MetadataBuilder GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder mappedFieldData);
}

API Usage

PersistedAssemblyBuilder ab = new PersistedAssemblyBuilder(new AssemblyName("MyAssembly"), typeof(object).Assembly);
TypeBuilder tb = ab.DefineDynamicModule("MyModule").DefineType("MyType", TypeAttributes.Public | TypeAttributes.Class);
// ...
MethodBuilder entryPoint = tb.DefineMethod("Main", MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static);
ILGenerator il2 = entryPoint .GetILGenerator();
// ...
il2.Emit(OpCodes.Ret);
tb.CreateType();

MetadataBuilder metadataBulder = ab.GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder fieldData);
PEHeaderBuilder peHeaderBuilder = new PEHeaderBuilder(
                // Header for PEFileKinds.WindowApplication imageCharacteristics and subsystem need to be set
                imageBase: 0x00400000, // this is the default value, was not necessarily needed to set.
                imageCharacteristics: Characteristics.ExecutableImage,
                subsystem: Subsystem.WindowsGui);

ManagedPEBuilder peBuilder = new ManagedPEBuilder(
                header: peHeaderBuilder,
                metadataRootBuilder: new MetadataRootBuilder(metadataBuilder),
                ilStream: ilStream,
                mappedFieldData: fieldData,
                entryPoint: MetadataTokens.MethodDefinitionHandle(entryPoint.MetadataToken));

BlobBuilder peBlob = new BlobBuilder();
peBuilder.Serialize(peBlob);

// in case saving to a file:
using var fileStream = new FileStream("MyAssembly.exe", FileMode.Create, FileAccess.Write);
peBlob.WriteContentTo(fileStream); 
fileStream.Close();
jkotas commented 7 months ago

public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable? assemblyAttributes = null);

This can be a regular constructor. I do not think that a factory method serves any purpose here.

buyaa-n commented 7 months ago

I have moved the API proposal to the description as it became completely different proposal than the original one, folded the old proposal in the description. Added blocking to get it reviewed ASAP.

masonwheeler commented 7 months ago

@buyaa-n

PersistedAssemblyBuilder ab = PersistedAssemblyBuilder.DefinePersistedAssembly(new AssemblyName("MyAssembly"), typeof(object).Assembly);
TypeBuilder tb = ab.DefineDynamicModule("MyModule").DefineType("MyType", TypeAttributes.Public | TypeAttributes.Class);
// ...
MethodBuilder entryPoint = tb.DefineMethod("Main", MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static);
ILGenerator il2 = main.GetILGenerator();

main has not been defined in this sample code. Is it supposed to be the same thing as entryPoint?

buyaa-n commented 7 months ago

main has not been defined in this sample code. Is it supposed to be the same thing as entryPoint?

Right, in the new example I changed the main -> entryPoint, forgot to update in ILGenerator il2 = main.GetILGenerator();, thanks!

bartonjs commented 6 months ago

Video

Breaking API compatibility from .NET Framework (by having Save somewhere other than on AssemblyBuilder) is unfortunate; but seems tolerable based on expected usage.

Given that, looks good as proposed.

namespace System.Reflection.Emit;

public abstract partial class AssemblyBuilder
{
    // Previously approved new APIs
-   public static AssemblyBuilder DefinePersistedAssembly(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

-   public void Save(Stream stream);
-   public void Save(string assemblyFileName);
-   protected abstract void SaveCore(Stream stream);
}

+public sealed class PersistedAssemblyBuilder : AssemblyBuilder
{
+   public PersistedAssemblyBuilder(AssemblyName name, Assembly coreAssembly, IEnumerable<CustomAttributeBuilder>? assemblyAttributes = null);

+   public void Save(Stream stream);
+   public void Save(string assemblyFileName);
+   public MetadataBuilder GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder mappedFieldData);
}