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.

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

Tracking issue for remaining AssemblyBuilder.Save work in .NET 9 #92975

Closed
20 tasks done
buyaa-n opened this issue Oct 3, 2023 · 46 comments
Closed
20 tasks done

Tracking issue for remaining AssemblyBuilder.Save work in .NET 9 #92975

buyaa-n opened this issue Oct 3, 2023 · 46 comments

Comments

@buyaa-n
Copy link
Contributor

buyaa-n commented Oct 3, 2023

See #62956 for work done in .NET 8 for adding AssemblyBuilder.Save(string/stream) support. Remaining work include:

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 3, 2023
@ghost
Copy link

ghost commented Oct 3, 2023

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

Issue Details

See #62956 for what have done for adding AssemblyBuilder.Save(string/stream) support in .NET 8.
Remaining work include:

  • Add ILGenerator implementation
    • Initial ILGenerator implementation, save minimal/simple IL.
    • Calculate maxstack for supported IL.Emit overloads
    • Add support for emitting method, field, constructor call
    • Add local variables support
    • Add support for ControlFlowBuilder branching/scoping
    • Add label (loop) support
    • Add TryCatch block support
  • Add support for remaining members
    • Add ConstructorBuilderImpl and save it to file/stream (this needs minimal IL support)
    • Add PropertyBuilderImpl and save it to file/stream (this also needs minimal IL support)
    • Add EventBuildterImpl and save it to file/stream
    • Add implementation for remaining unimplemented APIs for a MethodBuilder/TypeBuilder/FieldBuilder (like SignatureCallingConvention, *RequiredCustomModifiers, *OptionalCustomModifiers)
  • Add an API proposal for creating PDBs and implement it
  • Entry point support
Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection.Emit

Milestone: -

@masonwheeler
Copy link
Contributor

An important part of Reflection.Emit that I don't see listed here was PDB generation. Is this on the menu anywhere?

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Nov 17, 2023

Look the last bullet of the list.

@masonwheeler
Copy link
Contributor

Oops! Not sure how I missed that. Good to see it got included.

@Xyncgas

This comment was marked as off-topic.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Jan 3, 2024

We are making a good progress, planning to finish the main functionality (excluding entry point and PDB) and make related APIs public in preview 1.

Now it would be great to test the implementation with a real-life scenario. I would really appreciate if you could:

  • Prepare a simple app with your use case and share with me (in this thread or email me)
  • Provide a sample assembly similar what your app generates, we can use for testing.
  • Dogfood the daily build and try out the new AssemblyBuilder implementation yourself and give feedback. (The APIs are still internal so need to use reflection for now as mentioned in the sample)

Thank you in advance!

@masonwheeler
Copy link
Contributor

Here's my real-life scenario: https://github.com/boo-lang/boo

This is not a "simple app" but rather a thorough, comprehensive test of the system: a compiler that uses System.Reflection.Emit as its code generator, with thousands of test cases in its automated test suite, including some scenarios that are supported by refemit but not found in the C# language.

Last I checked, all the tests pass under .NET Framework. So seeing how many pass under this project is a good benchmark for how correct your implementation is.

@masonwheeler
Copy link
Contributor

So when is preview 4 expected to be available?

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Apr 24, 2024

Around 21-May

@masonwheeler
Copy link
Contributor

One bit of API surface that's still noticeably missing is resources. AssemblyBuilder.DefineUnmanagedResource, ModuleBuilder.DefineManifestResource, and ModuleBuilder.DefineResource are all in use in the compiler I'm trying to port, and none of these are available in the .NET Core version. Are there any plans to implement these? If not, what is the "right way" to set up resources in your PE file in Core?

@jkotas
Copy link
Member

jkotas commented Apr 27, 2024

The recommended replacement is to pass the resource blobs as managedResources and nativeResources arguments directly to the ManagedPEBuilder constructor.

@masonwheeler
Copy link
Contributor

@jkotas Sure, but where do you get "the resource blobs" from? There doesn't seem to be much information out there on the subject. Googling "c# create resource blobs" gives you a bunch of results about using Azure blob storage, and "c# create resources" gives you results about working with RESX files.

@Xyncgas
Copy link

Xyncgas commented Apr 27, 2024

Creating resource blobs in the context of .NET typically refers to embedding resources directly into your assembly as binary data. This could include images, XML files, localization strings, or any other data that you want to include with your application.

@jkotas
Copy link
Member

jkotas commented Apr 28, 2024

where do you get "the resource blobs" from?

  • If you called ModuleBuilder.DefineManifestResource before, the stream is the blob.
  • If you called ModuleBuilder.DefineResource before, create ResourceWriter and save it into a memory stream that will be the blob. (ModuleBuilder.DefineResource was a thin wrapper over ResourceWriter and DefineManifestResource.)
  • Add entries for all resources using MetadataBuilder.AddManifestResource. All streams have to be concatenated together into one BlobBuilder that you pass into ManagedPEBuilder. The offset within the BlobBuilder is argument for AddManifestResource.

@buyaa-n Could you please look into creating a sample for this?

@masonwheeler
Copy link
Contributor

@jkotas Thanks, that's very helpful. And what about the unmanaged resources? It's not at all clear from the API or documentation how ResourceSectionBuilder is supposed to work. I have a bunch of bytes already that are trivial to write to a BlobBuilder, but what in the world am I supposed to do with those two offset values?!?

@jkotas
Copy link
Member

jkotas commented Apr 28, 2024

If you have the raw bytes, you can just write them out. Here is how it is done in Roslyn: https://github.com/dotnet/roslyn/blob/7a96b1530744f6637ad6bb6bf5b8dfa33ea15b81/src/Compilers/Core/Portable/PEWriter/PeWriter.cs#L431-L447 .

The two offsets are for cases where the native resources have relocs.

@masonwheeler
Copy link
Contributor

@jkotas OK, thanks!

@buyaa-n
Copy link
Contributor Author

buyaa-n commented May 15, 2024

@buyaa-n Could you please look into creating a sample for this?

Added with dotnet/docs#40735

@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 16, 2024

An important part of Reflection.Emit that I don't see listed here was PDB generation. Is this on the menu anywhere?

@masonwheeler and others, have you tried the PDB generation with the new PersistedAssemblyBuilder? How was it and any feedback so far?

@dotnet dotnet unlocked this conversation Aug 16, 2024
@leppie
Copy link
Contributor

leppie commented Aug 18, 2024

An important part of Reflection.Emit that I don't see listed here was PDB generation. Is this on the menu anywhere?

@masonwheeler and others, have you tried the PDB generation with the new PersistedAssemblyBuilder? How was it and any feedback so far?

I gave it a quick shot on the first preview it came out, but was hoping an extension for the Save method would be added to behave like .NET Desktop.

Not sure if something was added or if someone has made one.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 21, 2024

was hoping an extension for the Save method would be added to behave like .NET Desktop.

Are you talking about Save(String, PortableExecutableKinds, ImageFileMachine) constructor?

Instead that we added GenerateMetadata overloads that outputs generated metadata which user could use populating the assembly with various settings they want, not only limited to PortableExecutableKinds, ImageFileMachine enums

public sealed class PersistedAssemblyBuilder : AssemblyBuilder
{
    public MetadataBuilder GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder mappedFieldData);
    public MetadataBuilder GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder mappedFieldData, out MetadataBuilder pdbBuilder) { }
}

See examples here: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-reflection-emit-persistedassemblybuilder#emit-symbols-and-generate-pdb

@leppie
Copy link
Contributor

leppie commented Aug 21, 2024

Instead that we added GenerateMetadata overloads

That is seriously complicated :D

I was hoping for Save(string, bool emitDebugInfo) that takes care of all the GenerateMetadata stuff in a way PDB files was done in .NET 4.x . Or something on DefineDynamicModule .

Hoping for too much?

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 23, 2024

That is seriously complicated :D

I was hoping for Save(string, bool emitDebugInfo) that takes care of all the GenerateMetadata stuff in a way PDB files was done in .NET 4.x . Or something on DefineDynamicModule .

Hoping for too much?

Well, we could add an API that cover the basic scenarios by defaulting assembly and PDB generation options. The question is which options should be defaulted and which options would be available for the user, somebody should file an issue for that as a start.

Meanwhile you could create and use a helper method that does what that Save(string, bool emitDebugInfo) method would do for you, for example:

public static void Save(PersistedAssemblyBuilder ab, string assemblyFileName, bool emitDebugInfo)
{
    MetadataBuilder metadataBuilder = ab.GenerateMetadata(out BlobBuilder ilStream, out _, out MetadataBuilder pdbBuilder);

    BlobBuilder portablePdbBlob = new BlobBuilder();
    PortablePdbBuilder portablePdbBuilder = new PortablePdbBuilder(pdbBuilder, metadataBuilder.GetRowCounts(), entryPoint: default);
    BlobContentId pdbContentId = portablePdbBuilder.Serialize(portablePdbBlob);
    using FileStream pdbFileStream = new FileStream($"{assemblyFileName}.pdb", FileMode.Create, FileAccess.Write);
    portablePdbBlob.WriteContentTo(pdbFileStream);

    DebugDirectoryBuilder debugDirectoryBuilder = new DebugDirectoryBuilder();
    debugDirectoryBuilder.AddCodeViewEntry($"{assemblyFileName}.pdb", pdbContentId, portablePdbBuilder.FormatVersion);

    ManagedPEBuilder peBuilder = new ManagedPEBuilder(
                    header: new PEHeaderBuilder(imageCharacteristics: Characteristics.ExecutableImage | Characteristics.Dll),
                    metadataRootBuilder: new MetadataRootBuilder(metadataBuilder),
                    ilStream: ilStream,
                    debugDirectoryBuilder: debugDirectoryBuilder);

    BlobBuilder peBlob = new BlobBuilder();
    peBuilder.Serialize(peBlob);
    using var dllFileStream = new FileStream($"{assemblyFileName}.dll", FileMode.Create, FileAccess.Write);
    peBlob.WriteContentTo(dllFileStream);
}

@leppie
Copy link
Contributor

leppie commented Aug 29, 2024

use a helper method

Thanks, helped a lot.

There were 2 issues, but seems to work from a small testcase.

  1. field data was not mapped
  2. emitDebugInfo was not used

Here is my current version's body (got some other stuff around it):

MetadataBuilder metadataBuilder = ab.GenerateMetadata(out BlobBuilder ilStream, out BlobBuilder fieldData, out MetadataBuilder pdbBuilder);

DebugDirectoryBuilder debugDirectoryBuilder = null;

if (emitDebugInfo)
{
  BlobBuilder portablePdbBlob = new BlobBuilder();
  PortablePdbBuilder portablePdbBuilder = new PortablePdbBuilder(pdbBuilder, metadataBuilder.GetRowCounts(), entryPoint: default);
  BlobContentId pdbContentId = portablePdbBuilder.Serialize(portablePdbBlob);
  using FileStream pdbFileStream = new FileStream($"{assemblyFileName}.pdb", FileMode.Create, FileAccess.Write);
  portablePdbBlob.WriteContentTo(pdbFileStream);

  debugDirectoryBuilder = new DebugDirectoryBuilder();
  debugDirectoryBuilder.AddCodeViewEntry($"{assemblyFileName}.pdb", pdbContentId, portablePdbBuilder.FormatVersion);
}

ManagedPEBuilder peBuilder = new ManagedPEBuilder(
                header: new PEHeaderBuilder(imageCharacteristics: Characteristics.ExecutableImage | Characteristics.Dll),
                metadataRootBuilder: new MetadataRootBuilder(metadataBuilder),
                ilStream: ilStream,
                mappedFieldData: fieldData,
                debugDirectoryBuilder: debugDirectoryBuilder);

BlobBuilder peBlob = new BlobBuilder();
peBuilder.Serialize(peBlob);
using var dllFileStream = new FileStream($"{assemblyFileName}.dll", FileMode.Create, FileAccess.Write);
peBlob.WriteContentTo(dllFileStream);

@leppie
Copy link
Contributor

leppie commented Aug 30, 2024

@buyaa-n Is there anything planned for embedded/manifest resources? Or is this something we need to add manually too with the PE builder?

Not a deal breaker for me as I can just put it in initialized data.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Aug 30, 2024

Nothing planned for now as user can add them manually, there is an instruction above: #92975 (comment)

And a sample added in doc: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-reflection-emit-persistedassemblybuilder#add-resources-with-persistedassemblybuilder

@leppie
Copy link
Contributor

leppie commented Sep 4, 2024

@buyaa-n I think I have found an issue, but unfortunately I dont have a small repro.

Steps for repro:

  1. Say you create 2 assemblies, and 1 depend on another. B depends on A.
  2. You create A
  3. You create B, calls a method in A
  4. You save A
  5. You save B (kaboom)

B references a MethodBuilderImpl in A. The module scopename refers to the correct filename, but Name says "<In Memory Module>". When emitted, it strangely emits the static constructor of the class I am calling from in B.

This scenario works correctly on .NET 2/4. And if I rememebr correctly it even allows for cyclic references where A and B can both reference each other.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Sep 4, 2024

Thanks @leppie, there is no valid token populated until the assembly is saved, could you try:

  1. Say you create 2 assemblies, and 1 depend on another. B depends on A.
  2. You create A
  3. You save A
  4. You create B, calls a method in A
  5. You save B

And if I remember correctly it even allows for cyclic references where A and B can both reference each other.

With current design probably we could not support cyclic references

@jkotas
Copy link
Member

jkotas commented Sep 4, 2024

With current design probably we could not support cyclic references

Why not? #92975 (comment) sounds like a bug to me. The ref tokens are separate from the def token. We should be able to populated the ref token independently on the def token to allow creating cycles between assemblies.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Sep 4, 2024

I will see what I can for fix.

@leppie
Copy link
Contributor

leppie commented Sep 5, 2024

I will see what I can for fix.

Not sure if this is related, but I feel it might be.

This time I am trying to emit a cast to a type in a referenced assembly (same scenario as above).

System.NotSupportedException: The invoked member is not supported in a dynamic module.
   at System.Reflection.Emit.TypeBuilderImpl.GetElementType()
   at System.Reflection.Emit.ModuleBuilderImpl.IsConstructedFromTypeBuilder(Type type)
   at System.Reflection.Emit.ModuleBuilderImpl.TryGetTypeHandle(Type type)
   at System.Reflection.Emit.ILGeneratorImpl.Emit(OpCode opcode, Type cls)

From what @jkotas said, this should also be a ref and not a def token I think.

Update: Debugged some more and it seems all my issues are related. Seems the modulebuilder only tracks assembly references from 'files' and anything referenced from another assemblybuilder is leading to unexpected behavior, ie exceptions, doing something wrong, etc. Hope you can fix this before final release.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Sep 10, 2024

Thank you @leppie for reporting the bug, and @jkotas for pointers, the fix is up and also created separate issue for tacking the issue and porting the fix into 9.0

@leppie
Copy link
Contributor

leppie commented Sep 20, 2024

Can confirm there is no more outstanding issues for me.

@buyaa-n if you interested in the result of your work: https://github.com/IronScheme/IronScheme/releases/tag/1.0.385

@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants