gh-jbevain/cecil
Watch
1
0
Fork
You've already forked cecil
 0
Code Issues 79 Pull requests 35 Releases 6 Wiki Activity

Removed MaxStackSize setter. #892

Open
kekyo wants to merge 1 commit from kekyo/fix-maxstack-ignored into master
pull from: kekyo/fix-maxstack-ignored
merge into: gh-jbevain:master
Conversation 7 Commits 1 Files changed 2 +1 -2
kekyo commented 2023-01-18 22:46:23 -06:00 (Migrated from github.com)

When generating assemblies using cecil, I manually specified .maxstack for MethodBody, but it is not reflected. It is always overwritten with the calculated result. This problem has been fixed in this PR.

Example:

var method = new MethodDefinition(...);
var body = method.Body;

body.MaxStackSize = 100;

// ...

module.Write("foo.dll");
When generating assemblies using cecil, I manually specified `.maxstack` for MethodBody, but it is not reflected. It is always overwritten with the calculated result. This problem has been fixed in this PR. Example: ```csharp var method = new MethodDefinition(...); var body = method.Body; body.MaxStackSize = 100; // ... module.Write("foo.dll"); ```
ltrzesniewski (Migrated from github.com) reviewed 2023-01-19 11:02:34 -06:00
Mono.Cecil.Cil/CodeWriter.cs
ltrzesniewski (Migrated from github.com) commented 2023-01-19 11:02:33 -06:00

The CodeReader sets max_stack_size to a nonzero value for methods with a fat header:

github.com/jbevain/cecil@870ce3ed2b/Mono.Cecil.Cil/CodeReader.cs (L118)

This will certainly break workflows such as loading an assembly, adding instructions which increase the max stack size, then writing the assembly. The writer would assume the max stack size was set explicitly by the user.

The following would be a safer alternative:

			body.max_stack_size = Math.Max(body.max_stack_size, max_stack);

But then, changes that decrease the max stack size wouldn't be taken into account automatically. So maybe you need a "maxstack is set explicitly" flag.

Also, note this only applies to fat headers. Cecil will always write a tiny header if possible. I'm actually curious why you need this change in the first place.

The `CodeReader` sets `max_stack_size` to a nonzero value for methods with a fat header: https://github.com/jbevain/cecil/blob/870ce3ed2bbc8cea09f91d328e941a0fa5a0cd21/Mono.Cecil.Cil/CodeReader.cs#L118 This will certainly break workflows such as loading an assembly, adding instructions which increase the max stack size, then writing the assembly. The writer would assume the max stack size was set explicitly by the user. The following would be a safer alternative: ```suggestion body.max_stack_size = Math.Max(body.max_stack_size, max_stack); ``` But then, changes that decrease the max stack size wouldn't be taken into account automatically. So maybe you need a _"maxstack is set explicitly"_ flag. Also, note this only applies to fat headers. Cecil will always write a tiny header if possible. I'm actually curious why you need this change in the first place.
kekyo (Migrated from github.com) reviewed 2023-01-19 19:00:01 -06:00
Mono.Cecil.Cil/CodeWriter.cs
kekyo (Migrated from github.com) commented 2023-01-19 19:00:01 -06:00

I'm writing an assembler tooling and using cecil for output. One time I noticed that the value of .maxstack output by cecil sometimes differed from the expected value.

As it turns out, the problem was a mistake in my assembler code, but it made me think a bit about calculating the value of maxstack. For example, which is a rather aggressive:

int32 foo(int32 count)
{
  .locals ([0] int32 i)

  ldarg.0   ; i = count
  stloc.0

L1:
  ldc.i4.1  ; [increase ev stack]

  ldloc.0   ; i--
  ldc.i4.1
  sub
  stloc.0

  ldloc.0   ; if (i != 0) goto L1
  ldc.i4.0
  ceq
  brfalse L1

  ; (snip: decrease ev stack by count)

  ret
}

Even if a static execution flow analysis is performed, complete maxstack size cannot be calculated in advance. Therefore, the person writing the assembly code must manually set an appropriate size for maxstack.

Of course in most cases, the size should be set to the size obtained from the static execution flow analysis, so I believe we only need to deal with this exceptional situation. So, I explicitly specified a value for the MaxStackSize property only if a size was specified, but this was not reflected.

This will certainly break workflows such as loading an assembly, adding instructions which increase the max stack size, then writing the assembly. The writer would assume the max stack size was set explicitly by the user.

I completely agree and it was my mistake that the existing implementation was affected. May I make additional changes? I will use the "maxstack is set explicitly" flag as suggested.

I'm writing an assembler tooling and using cecil for output. One time I noticed that the value of `.maxstack` output by cecil sometimes differed from the expected value. As it turns out, the problem was a mistake in my assembler code, but it made me think a bit about calculating the value of maxstack. For example, which is a rather aggressive: ``` int32 foo(int32 count) { .locals ([0] int32 i) ldarg.0 ; i = count stloc.0 L1: ldc.i4.1 ; [increase ev stack] ldloc.0 ; i-- ldc.i4.1 sub stloc.0 ldloc.0 ; if (i != 0) goto L1 ldc.i4.0 ceq brfalse L1 ; (snip: decrease ev stack by count) ret } ``` Even if a static execution flow analysis is performed, complete maxstack size cannot be calculated in advance. Therefore, the person writing the assembly code must manually set an appropriate size for maxstack. Of course in most cases, the size should be set to the size obtained from the static execution flow analysis, so I believe we only need to deal with this exceptional situation. So, I explicitly specified a value for the `MaxStackSize` property only if a size was specified, but this was not reflected. > This will certainly break workflows such as loading an assembly, adding instructions which increase the max stack size, then writing the assembly. The writer would assume the max stack size was set explicitly by the user. I completely agree and it was my mistake that the existing implementation was affected. May I make additional changes? I will use the "maxstack is set explicitly" flag as suggested.
kekyo commented 2023-01-19 20:53:27 -06:00 (Migrated from github.com)

I wasn't sure if the setter for the MaxStackSize property should check to see if the value has been changed, but I determined that the intention was to change it when the setter was called. How about this?

I wasn't sure if the setter for the `MaxStackSize` property should check to see if the value has been changed, but I determined that the intention was to change it when the setter was called. How about this?
ltrzesniewski (Migrated from github.com) reviewed 2023-01-20 04:03:16 -06:00
Mono.Cecil.Cil/CodeWriter.cs
ltrzesniewski (Migrated from github.com) commented 2023-01-20 04:03:16 -06:00

complete maxstack size cannot be calculated in advance

Oh, you're misunderstanding what maxstack means. It's not the max size of the execution stack, but the max size of the IL evaluation stack. The IL evaluation stack size is constant for each IL instruction.

See ECMA-335 III.1.7.4:

[Note: Maxstack is related to analysis of the program, not to the size of the stack at runtime. It does not specify the maximum size in bytes of a stack frame, but rather the number of items that
shall be tracked by an analysis tool. end note]

[Rationale: By analyzing the CIL stream for any method, it is easy to determine how many items
will be pushed on the CIL Evaluation stack. However, specifying that maximum number ahead
of time helps a CIL-to-native-code compiler (especially a simple one that does only a single pass
through the CIL stream) in allocating internal data structures that model the stack and/or
verification algorithm. end rationale]

TL;DR: It can always be calculated statically, and Cecil does the right thing. 🙂

May I make additional changes?

I'm not the maintainer, just a guy on the internet trying to help.

> complete maxstack size cannot be calculated in advance Oh, you're misunderstanding what maxstack means. It's not the max size of the _execution stack_, but the max size of the IL _evaluation stack_. The IL evaluation stack size is constant for each IL instruction. See ECMA-335 III.1.7.4: > [Note: Maxstack is related to analysis of the program, not to the size of the stack at runtime. It does not specify the maximum size in bytes of a stack frame, but rather the number of items that shall be tracked by an analysis tool. end note] > > [Rationale: By analyzing the CIL stream for any method, it is easy to determine how many items will be pushed on the CIL Evaluation stack. However, specifying that maximum number ahead of time helps a CIL-to-native-code compiler (especially a simple one that does only a single pass through the CIL stream) in allocating internal data structures that model the stack and/or verification algorithm. end rationale] TL;DR: It can always be calculated statically, and Cecil does the right thing. 🙂 > May I make additional changes? I'm not the maintainer, just a guy on the internet trying to help.
kekyo (Migrated from github.com) reviewed 2023-01-20 04:36:06 -06:00
Mono.Cecil.Cil/CodeWriter.cs
kekyo (Migrated from github.com) commented 2023-01-20 04:36:06 -06:00

I'm not the maintainer, just a guy on the internet trying to help.

Thanks for reviewing my code! I await comments from the maintainer.

I understand the difference between the runtime stack (as indicated by rsp and rbp in x64) and the evaluation stack. And I also understand that the topic about maxstack is about the evaluation stack.

The sample code indicates that "If the max stack size of the evaluation stack changes at "run-time", max stack size cannot be calculated by static analysis."

I could not read the meaning from the description in the ECMA specification that the evaluation stack consumption is always statically determined. In fact, if we write code like the sample code above, we will see that it is not possible to estimate it in advance. And it causes InvalidProgramException when maxstack is insufficient.

I thought there might be a need to explicitly specify maxstack when outputting assemblies.

> I'm not the maintainer, just a guy on the internet trying to help. Thanks for reviewing my code! I await comments from the maintainer. I understand the difference between the runtime stack (as indicated by `rsp` and `rbp` in x64) and the evaluation stack. And I also understand that the topic about maxstack is about the evaluation stack. The sample code indicates that "If the max stack size of the evaluation stack changes at "run-time", max stack size cannot be calculated by static analysis." I could not read the meaning from the description in the ECMA specification that the evaluation stack consumption is always statically determined. In fact, if we write code like the sample code above, we will see that it is not possible to estimate it in advance. And it causes `InvalidProgramException` when maxstack is insufficient. I thought there might be a need to explicitly specify maxstack when outputting assemblies.
ltrzesniewski (Migrated from github.com) reviewed 2023-01-20 08:25:10 -06:00
Mono.Cecil.Cil/CodeWriter.cs
ltrzesniewski (Migrated from github.com) commented 2023-01-20 08:25:09 -06:00

Ok, I see. If you're trying to write custom tooling (something that replaces the .NET JIT), then this makes sense. Your code could accept a variable evaluation stack depth.

Just be aware this is not correct ECMA-335 IL though, and is not accepted by the .NET JIT. You could support that as a custom extension to the standard.

Here's an example project: ConsoleApp.zip, it does an inifite loop while pushing an item to the stack. The .NET JIT immediately rejects it with an InvalidProgramException.

See the III.1.7.5 paragraph from ECMA-335 which defines the related correctness rule (emphasis mine):

It shall be possible, with a single forward-pass through the CIL instruction stream for any
method, to infer the exact state of the evaluation stack at every instruction (where by “state” we
mean the number and type of each item on the evaluation stack).

In particular, if that single-pass analysis arrives at an instruction, call it location X, that
immediately follows an unconditional branch, and where X is not the target of an earlier branch
instruction, then the state of the evaluation stack at X, clearly, cannot be derived from existing
information. In this case, the CLI demands that the evaluation stack at X be empty.

Following on from this rule, it would clearly be invalid CIL if a later branch instruction to X
were to have a non-empty evaluation stack

[Rationale: This constraint ensures that CIL code can be processed by a simple CIL-to-nativecode compiler. It ensures that the state of the evaluation stack at the beginning of each CIL can
be inferred from a single, forward-pass analysis of the instruction stream. end rationale]

[Note: the stack state at location X in the above can be inferred by various means: from a
previous forward branch to X; because X marks the start of an exception handler, etc. end note]

The rules for merging evaluation stack frames described in III.1.8.1.3 could also be relevant, even though they determine verifiability (and not correctness).

Ok, I see. If you're trying to write custom tooling (something that replaces the .NET JIT), then this makes sense. Your code could accept a variable evaluation stack depth. Just be aware this is not correct ECMA-335 IL though, and is not accepted by the .NET JIT. You could support that as a custom extension to the standard. Here's an example project: [ConsoleApp.zip](https://github.com/jbevain/cecil/files/10467025/ConsoleApp.zip), it does an inifite loop while pushing an item to the stack. The .NET JIT immediately rejects it with an `InvalidProgramException`. See the III.1.7.5 paragraph from ECMA-335 which defines the related correctness rule (emphasis mine): > **It shall be possible, with a single forward-pass through the CIL instruction stream for any method, to infer the exact state of the evaluation stack at every instruction (where by “state” we mean the number and type of each item on the evaluation stack).** > > In particular, if that single-pass analysis arrives at an instruction, call it location X, that immediately follows an unconditional branch, and where X is not the target of an earlier branch instruction, then the state of the evaluation stack at X, clearly, cannot be derived from existing information. In this case, the CLI demands that the evaluation stack at X be empty. > > Following on from this rule, it would clearly be invalid CIL if a later branch instruction to X were to have a non-empty evaluation stack > > [Rationale: This constraint ensures that CIL code can be processed by a simple CIL-to-nativecode compiler. It ensures that the state of the evaluation stack at the beginning of each CIL can be inferred from a single, forward-pass analysis of the instruction stream. end rationale] > > [Note: the stack state at location X in the above can be inferred by various means: from a previous forward branch to X; because X marks the start of an exception handler, etc. end note] The rules for merging evaluation stack frames described in III.1.8.1.3 could also be relevant, even though they determine verifiability (and not correctness).
kekyo (Migrated from github.com) reviewed 2023-01-20 20:56:10 -06:00
Mono.Cecil.Cil/CodeWriter.cs
kekyo (Migrated from github.com) commented 2023-01-20 20:56:09 -06:00

Thanks for showing the pointer. I referred to the text of ECMA, as it seems I only looked at the quoted part and misinterpreted it.

And I understood that in III.1.7.4 and III.1.7.5, the statically analyzed maxstack must be set and its value must be correct.
I now understand that this means that an instruction stream like the sample code I showed can be written, but it is not guaranteed (nor should it be) that the CLR can execute it. Your point is absolutely correct!!! 😄 If someone asks me, I can now explain to them that they should not write such CIL code.

So, back to cecil, I always feel that the MaxStackSize property should not have a setter, since the maxstack can be calculated by static analysis. The CoreReader stores the value directly in the backing store of the MaxStackSize property. So without a setter, cecil users will be able to treat MaxStackSize as a view in the true sense.

I pushed the code modified to do so. Thank you explain it real issue again!

Thanks for showing the pointer. I referred to the text of ECMA, as it seems I only looked at the quoted part and misinterpreted it. And I understood that in III.1.7.4 and III.1.7.5, the statically analyzed maxstack must be set and its value must be correct. I now understand that this means that an instruction stream like the sample code I showed can be written, but it is not guaranteed (nor should it be) that the CLR can execute it. Your point is absolutely correct!!! 😄 If someone asks me, I can now explain to them that they should not write such CIL code. So, back to cecil, I always feel that the `MaxStackSize` property should not have a setter, since the maxstack can be calculated by static analysis. The `CoreReader` stores the value directly in the backing store of the `MaxStackSize` property. So without a setter, cecil users will be able to treat `MaxStackSize` as a view in the true sense. I pushed the code modified to do so. Thank you explain it real issue again!
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'kekyo/fix-maxstack-ignored:kekyo/fix-maxstack-ignored'
git switch 'kekyo/fix-maxstack-ignored'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'master'
git merge --no-ff 'kekyo/fix-maxstack-ignored'
git switch 'kekyo/fix-maxstack-ignored'
git rebase 'master'
git switch 'master'
git merge --ff-only 'kekyo/fix-maxstack-ignored'
git switch 'kekyo/fix-maxstack-ignored'
git rebase 'master'
git switch 'master'
git merge --no-ff 'kekyo/fix-maxstack-ignored'
git switch 'master'
git merge --squash 'kekyo/fix-maxstack-ignored'
git switch 'master'
git merge --ff-only 'kekyo/fix-maxstack-ignored'
git switch 'master'
git merge 'kekyo/fix-maxstack-ignored'
git push origin 'master'
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1.0

   
bug

   
enhancement

   
invalid
No labels
1.0
bug
enhancement
invalid
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-jbevain/cecil!892
No description provided.