Repository navigation
Minimal ILGenerator implementation for new AssemblyBuilder.Save - #92846
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-reflection-emit Issue DetailsMore tests will be added Contributes to #83988
|
| private static int AddMethodBody(MethodBuilderImpl method, ILGeneratorImpl il, MethodBodyStreamEncoder methodBodyEncoder) => | ||
| methodBodyEncoder.AddMethodBody( | ||
| instructionEncoder: il.Instructions, | ||
| maxStack: 8, // TODO |
There was a problem hiding this comment.
What happens if 8 is exceeded? If there is an exception or some other issue, we should probably try to fix this for this PR.
There was a problem hiding this comment.
If we can set maxstack == the number of opcodes for now that should protect us against any InvalidProgramException thrown by the JIT etc.
There was a problem hiding this comment.
I can do that until I have got a better handling
|
|
||
| if ((_methodImplFlags & MethodImplAttributes.CodeTypeMask) != MethodImplAttributes.IL || | ||
| (_methodImplFlags & MethodImplAttributes.Unmanaged) != 0 || | ||
| (_attributes & MethodAttributes.PinvokeImpl) != 0) |
There was a problem hiding this comment.
The CoreLib implementation also has a check for DllImport here. Is it intentionally omitted?
There was a problem hiding this comment.
We are setting the _attributes with MethodAttributes.PinvokeImpl when DllImportAttribute is set, no need to keep another boolean flag for this
steveharter
left a comment
There was a problem hiding this comment.
LGTM pending the discussion on a value for maxstack that shouldn't result in an exception if exceeded.
|
The failures not related and known |
…t/ILGeneratorImpl.cs
Basically, replacing the ILGenerator.Emit overloads implementaiton with
InstructionEncoderwhich will be written into the file when saving the methods in a type using MethodBodyStreamEncoderContributes to #83988