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

Implement remaining ILGenertorImpl methods, excluding scope. Add max stack calculation for branching. #96362

Merged
merged 5 commits into from
Jan 2, 2024

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Dec 29, 2023

  • Adding implementations for Emit(OpCode opcode, SignatureHelper signature), EmitCalli(OpCode opcode, CallingConvention unmanagedCallConv, Type? returnType, Type[]? parameterTypes) and EmitCalli(OpCode opcode, CallingConventions callingConvention, Type? returnType, Type[]? parameterTypes, Type[]? optionalParameterTypes)
  • Refactored member reference logic completely to cover method+optionalParameters reference scenario, also refactored/fixed all ModuleBuilder.Get***MetadataToken implementations as needed
  • No-op (instead of throwing NotImplementedException) for BeginScope(), EndScope() and UsingNamespace(string usingNamespace), their implementations will be added with PDB support.
  • Refactored max stack calculation to account forward and backward branching (followed the runtime approach and imported tests), switch casing etc and test
  • Fixed the bug that was causing BadImageFormatException when loading generated assemblies with Assembly.LoadFrom(path) for run. Some tests now run the loaded assembly.

Contributes to #92975

@ghost
Copy link

ghost commented Dec 29, 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
  • Adding implementations for Emit(OpCode opcode, SignatureHelper signature), EmitCalli(OpCode opcode, CallingConvention unmanagedCallConv, Type? returnType, Type[]? parameterTypes) and EmitCalli(OpCode opcode, CallingConventions callingConvention, Type? returnType, Type[]? parameterTypes, Type[]? optionalParameterTypes)
  • Refactored member reference logic completely to cover method+optionalParameters reference scenario, also refactored/fixed all ModuleBuilder.Get***MetadataToken implementations as needed
  • No-op (instead of throwing NotImplementedException) for BeginScope(), EndScope() and UsingNamespace(string usingNamespace), their implementations will be added with PDB support.
  • Refactored max stack calculation to account forward and backward branching, switch casing etc and test
  • Fixed the bug that was causing BadImageFormatException when loading generated assemblies with Assembly.LoadFrom(path) for run. Some tests no run the loaded assembly.

Contributes to #92975

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection.Emit

Milestone: -

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM; minor comments \ questions, Although there is a test failure related to this PR.

public override void UsingNamespace(string usingNamespace) => throw new NotImplementedException();
public override void UsingNamespace(string usingNamespace)
{
// TODO: No-op, will be implemented wit PDB support
Copy link
Member

Choose a reason for hiding this comment

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

Why not throw NIE?

ilg.Emit(OpCodes.Ldarg_0);
ilg.Emit(OpCodes.Brfalse, lab);

// The label is marked with a larger stack depth, one. This IL is invalid.
Copy link
Member

Choose a reason for hiding this comment

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

So we don't detect \ throw on these cases, and CreateTypeInfo() doesn't either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are imported from runtime ILGenerator tests

// The label is marked with a larger stack depth, one. This IL is invalid.
ilg.Emit(OpCodes.Ldc_I4_1);

Personally, I am not sure this exact test makes sense, though runtime doesn't detect or throw ...

@steveharter
Copy link
Member

Note the test error: AssemblySaveILGeneratorTests.SimpleForLoopTest has failed.

@buyaa-n buyaa-n merged commit 93caf8d into dotnet:main Jan 2, 2024
170 of 176 checks passed
@buyaa-n buyaa-n deleted the emit_calli branch January 2, 2024 17:26
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants