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

ILGenerator computation of maxstack is too conservative #63805

Closed
shonk-msft opened this issue Jan 14, 2022 · 5 comments · Fixed by #70388
Closed

ILGenerator computation of maxstack is too conservative #63805

shonk-msft opened this issue Jan 14, 2022 · 5 comments · Fixed by #70388

Comments

@shonk-msft
Copy link

Description

When generating a dynamic method both with many unconditional jumps, the ILGenerator's computation of maxstack is way to large - it adds the max stack of each basic block ending with an unconditional transfer (br, ret, throw, etc). If the generated code has many such bbs (eg, from the "then" clauses of if-then-else constructs) this sum can overflow 2^16. When it does, the maxstack recorded/used is the computed value mod 2^16. When that is less than the correct value, the JIT throws an InvalidProgramException.

For example, if there are 2^12 (4096) such blocks, each with a max stack of 2^4 (16), the computed maxstack is 2^16. That value mod 2^16 is zero. Since that is less than the actual (16 or more), the JIT will throw.

See the attached example code. This is written as a "test" that passes when the bug is present. The line enclosed by Assert.ThrowsException really should not throw.

ClrBugTests.cs.txt

Reproduction Steps

Build and run the unit test code above with .Net Core (verified with 3.1, and many later versions).

Expected behavior

The line marked with Assert.ThrowsException should not throw.

Actual behavior

An InvalidProgramException is thrown. Also, the computed max stack values (as written to console) are way too large. The actual max stack needed for this dynamic method is 4, and it doesn't depend on the number of basic blocks (the num parameter). The computed max stack values increase with num.

Regression?

No response

Known Workarounds

No good ones. This is blocking.

Configuration

No response

Other information

I'm working on a fix by fixing the computation in ILGenerator.cs.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 14, 2022
@ghost
Copy link

ghost commented Jan 14, 2022

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

Issue Details

Description

When generating a dynamic method both with many unconditional jumps, the ILGenerator's computation of maxstack is way to large - it adds the max stack of each basic block ending with an unconditional transfer (br, ret, throw, etc). If the generated code has many such bbs (eg, from the "then" clauses of if-then-else constructs) this sum can overflow 2^16. When it does, the maxstack recorded/used is the computed value mod 2^16. When that is less than the correct value, the JIT throws an InvalidProgramException.

For example, if there are 2^12 (4096) such blocks, each with a max stack of 2^4 (16), the computed maxstack is 2^16. That value mod 2^16 is zero. Since that is less than the actual (16 or more), the JIT will throw.

See the attached example code. This is written as a "test" that passes when the bug is present. The line enclosed by Assert.ThrowsException really should not throw.

ClrBugTests.cs.txt

Reproduction Steps

Build and run the unit test code above with .Net Core (verified with 3.1, and many later versions).

Expected behavior

The line marked with Assert.ThrowsException should not throw.

Actual behavior

An InvalidProgramException is thrown. Also, the computed max stack values (as written to console) are way too large. The actual max stack needed for this dynamic method is 4, and it doesn't depend on the number of basic blocks (the num parameter). The computed max stack values increase with num.

Regression?

No response

Known Workarounds

No good ones. This is blocking.

Configuration

No response

Other information

I'm working on a fix by fixing the computation in ILGenerator.cs.

Author: shonk-msft
Assignees: -
Labels:

area-System.Reflection.Emit, untriaged

Milestone: -

@jkotas jkotas added the bug label Jan 14, 2022
@steveharter
Copy link
Member

See also #62913 for discussion on how maxstack is (or can be) calculated.

@steveharter
Copy link
Member

I'm working on a fix by fixing the computation in ILGenerator.cs.

@shonk-msft any update?

@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Feb 15, 2022
@steveharter steveharter added this to the 7.0.0 milestone Feb 15, 2022
@shonk-msft
Copy link
Author

shonk-msft commented May 13, 2022

Hi @steveharter, sorry for the delay. I implemented a fix long ago but haven't gotten around to submitting the PR. I'll be handing this off to a co-worker Monday who should be more responsive :-).

panguye added a commit to panguye/runtime that referenced this issue Jun 7, 2022
When generating a dynamic method both with many unconditional jumps, the
ILGenerator's computation of maxstack is way to large - it adds the max
stack of each basic block ending with an unconditional transfer
(br, ret, throw, etc). If the generated code has many such bbs
(eg, from the "then" clauses of if-then-else constructs) this sum can
overflow 2^16. When it does, the maxstack recorded/used is the computed
value mod 2^16. When that is less than the correct value, the
JIT throws an InvalidProgramException.

Keep track of the stack depth at various positions and use it to
calculate an adjustment to the depth.

Fix dotnet#63805
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 7, 2022
jkotas pushed a commit that referenced this issue Jun 22, 2022
* Fix ILGenerator maxstack computation

When generating a dynamic method both with many unconditional jumps, the
ILGenerator's computation of maxstack is way to large - it adds the max
stack of each basic block ending with an unconditional transfer
(br, ret, throw, etc). If the generated code has many such bbs
(eg, from the "then" clauses of if-then-else constructs) this sum can
overflow 2^16. When it does, the maxstack recorded/used is the computed
value mod 2^16. When that is less than the correct value, the
JIT throws an InvalidProgramException.

Keep track of the stack depth at various positions and use it to
calculate an adjustment to the depth.

Fix #63805

* Fix System.Linq.Expressions tests

* Remove stack handling in MethodBuilder
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 22, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants