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

Emit/Call member references in new ILGenerator #94116

Merged
merged 5 commits into from Oct 30, 2023

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Oct 27, 2023

Add implementation for Emit(OpCode opcode, ConstructorInfo con), Emit(OpCode opcode, FieldInfo field), Emit(OpCode opcode, MethodInfo meth), Emit(OpCode opcode, Type cls) and EmitCall(OpCode opcode, MethodInfo methodInfo, Type[]? optionalParameterTypes)

Contributes to #92975

@ghost
Copy link

ghost commented Oct 27, 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

Add implementation for Emit(OpCode opcode, ConstructorInfo con), Emit(OpCode opcode, FieldInfo field), Emit(OpCode opcode, MethodInfo meth), Emit(OpCode opcode, Type cls) and EmitCall(OpCode opcode, MethodInfo methodInfo, Type[]? optionalParameterTypes)

Contributes to #92975

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection.Emit

Milestone: -

{
ArgumentNullException.ThrowIfNull(con);

if (!(opcode.Equals(OpCodes.Call) || opcode.Equals(OpCodes.Callvirt) || opcode.Equals(OpCodes.Newobj)))
Copy link
Member Author

Choose a reason for hiding this comment

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

Runtime implementation Asserts this instead of throwing:

if (opcode.StackBehaviourPush == StackBehaviour.Varpush)
{
// Instruction must be one of call or callvirt.
Debug.Assert(opcode.Equals(OpCodes.Call) ||
opcode.Equals(OpCodes.Callvirt),
"Unexpected opcode encountered for StackBehaviour of VarPush.");
stackchange++;
}
if (opcode.StackBehaviourPop == StackBehaviour.Varpop)
{
// Instruction must be one of call, callvirt or newobj.
Debug.Assert(opcode.Equals(OpCodes.Call) ||
opcode.Equals(OpCodes.Callvirt) ||
opcode.Equals(OpCodes.Newobj),
"Unexpected opcode encountered for StackBehaviour of VarPop.");

I think this should throw instead, same as in the EmitCall(OpCode opcode, MethodInfo methodInfo, Type[]? optionalParameterTypes):
if (!(opcode.Equals(OpCodes.Call) || opcode.Equals(OpCodes.Callvirt) || opcode.Equals(OpCodes.Newobj)))
throw new ArgumentException(SR.Argument_NotMethodCallOpcode, nameof(opcode));


int stackchange = 0;
// Push the return value if there is one.
stackchange++;
Copy link
Member

Choose a reason for hiding this comment

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

Are we just assuming there is a return value then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, as it is a constructor

WriteCustomAttributes(method._customAttributes, methodHandle);
_nextMethodDefRowId++;
MethodDefinitionHandle handle = AddMethodDefinition(method, method.GetMethodSignatureBlob(), offset, _nextParameterRowId);
Debug.Assert(method._handle == handle);
Copy link
Member

Choose a reason for hiding this comment

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

Why was this Assert added? Can it be added to AddMethodDefinition() instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why was this Assert added?

To make sure the method handle populated with AddMethodDefinition is in sync with prepopulated one.

Can it be added to AddMethodDefinition() instead?

It could, but AddMethodDefinition is a one liner that just calls MetadataBuilder.AddMethodDefinition, if you are OK with, don't really want to add another row.

@buyaa-n buyaa-n merged commit 8d59c90 into dotnet:main Oct 30, 2023
109 checks passed
@buyaa-n buyaa-n deleted the call_members branch October 30, 2023 20:45
@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 2023
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.

None yet

2 participants