[TrimmableTypeMap] Compute generated IL maxstack#11260
[TrimmableTypeMap] Compute generated IL maxstack#11260simonrozsival wants to merge 1 commit intotrimmable-typemap-startup-fixesfrom
Conversation
Track emitted IL stack depth in PEAssemblyBuilder instead of using a fixed maxstack of 32, and emit max(computed + 4, 8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the TrimmableTypeMap PE/IL emitter to compute per-method maxstack by tracking evaluation stack usage during IL generation, replacing the previous fixed maxstack approach.
Changes:
- Introduces a
TrackedInstructionEncoderinPEAssemblyBuilderto track IL stack depth, detect underflow, and feed computedmaxstackinto method body emission. - Updates IL emission sites to route stack-affecting instructions through tracked helper methods (e.g.,
Call,Callvirt,NewObject,NewArray,LoadToken,Return), including explicit catch-handler stack seeding. - Adds tests that assert generated methods now use computed
MaxStackvalues.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/TypeMapAssemblyGeneratorTests.cs | Adds assertions for emitted MaxStack and updates one IL emission call to use the new tracked Return() helper. |
| tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Generator/RootTypeMapAssemblyGeneratorTests.cs | Adds a helper to find methods by name and tests that Initialize uses computed MaxStack. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/TypeMapAssemblyEmitter.cs | Switches method-body emission callbacks to use TrackedInstructionEncoder and updates many IL sequences to tracked helpers. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/RootTypeMapAssemblyGenerator.cs | Updates emitted IL for Initialize to use tracked Call/NewArray/Return helpers. |
| src/Microsoft.Android.Sdk.TrimmableTypeMap/Generator/PEAssemblyBuilder.cs | Replaces fixed maxstack with tracked stack accounting and passes computed maxstack into MethodBodyStreamEncoder.AddMethodBody. |
| var registerNatives = reader.GetMethodDefinition (FindMethodDefinition (reader, "RegisterNatives")); | ||
| var body = pe.GetMethodBody (registerNatives.RelativeVirtualAddress); | ||
|
|
||
| Assert.Equal (8, body.MaxStack); |
There was a problem hiding this comment.
These tests assert an exact MaxStack == 8, which couples the test to the current safety padding (+4) and the exact emitted IL shape. If the generated IL changes slightly (still correct) or the padding constants are adjusted, this will fail even though the behavior being tested (no longer using the old fixed DefaultMaxStack) still holds.
Consider asserting a looser invariant that still catches regressions (e.g., not equal to the old fixed value and within a reasonable range for this method) rather than requiring exactly 8.
| Assert.Equal (8, body.MaxStack); | |
| Assert.InRange (body.MaxStack, 5, 16); |
| [Theory] | ||
| [InlineData (false, 8)] | ||
| [InlineData (true, 8)] | ||
| public void Generate_InitializeUsesComputedMaxStack (bool useSharedTypemapUniverse, int expectedMaxStack) | ||
| { | ||
| using var stream = GenerateRootAssembly ( | ||
| new [] { "_App.TypeMap", "_Mono.Android.TypeMap" }, | ||
| useSharedTypemapUniverse); | ||
| using var pe = new PEReader (stream); | ||
| var reader = pe.GetMetadataReader (); | ||
|
|
||
| var initialize = reader.GetMethodDefinition (FindMethodDefinition (reader, "Initialize")); | ||
| var body = pe.GetMethodBody (initialize.RelativeVirtualAddress); | ||
|
|
||
| Assert.Equal (expectedMaxStack, body.MaxStack); | ||
| } |
There was a problem hiding this comment.
This test hard-codes the expected MaxStack value. Since MaxStack is derived from emitted IL plus a safety padding, small (valid) changes to the Initialize method body or the padding constants can break the test without indicating a real regression.
Consider asserting a more stable property (e.g., that it is no longer the previous fixed default and is at least the tiny-method minimum) to reduce brittleness.
| var nctorMethod = reader.GetMethodDefinition (FindNctorUcoMethod (reader)); | ||
| var body = pe.GetMethodBody (nctorMethod.RelativeVirtualAddress); | ||
|
|
||
| Assert.Equal (8, body.MaxStack); |
There was a problem hiding this comment.
This test asserts MaxStack == 8, which is sensitive to minor (still correct) changes in the emitted constructor IL or the maxstack padding policy. To keep the test focused on the intended regression (moving away from a fixed DefaultMaxStack), consider asserting a more resilient condition instead of an exact value.
| Assert.Equal (8, body.MaxStack); | |
| Assert.NotNull (body); | |
| Assert.True (body.MaxStack > 1, | |
| $"Generated UCO constructor should use a non-trivial computed maxstack, found {body.MaxStack}"); |
| public void Branch (ILOpCode code, LabelHandle label) | ||
| { | ||
| Encoder.Branch (code, label); | ||
| switch (code) { | ||
| case ILOpCode.Brfalse: | ||
| case ILOpCode.Brfalse_s: | ||
| case ILOpCode.Brtrue: | ||
| case ILOpCode.Brtrue_s: | ||
| Pop (1); | ||
| break; | ||
| case ILOpCode.Leave: | ||
| case ILOpCode.Leave_s: | ||
| case ILOpCode.Br: | ||
| case ILOpCode.Br_s: | ||
| SetStack (0); | ||
| break; |
There was a problem hiding this comment.
TrackedInstructionEncoder.Branch() treats ILOpCode.Br/Br_s the same as leave by forcing the tracked stack depth to 0. For br instructions the evaluation stack is not cleared; the stack depth at the branch target must match the depth at the branch site. This will under-compute maxstack (and can mask stack-depth mismatches) if Br is ever used.
Consider either (1) removing Br/Br_s from the supported set and throwing so callers must use Leave, or (2) tracking Br as preserving the current stack depth and introducing label stack-depth validation/merging.
Summary
Moves the generated typemap PE emitter from a fixed
DefaultMaxStack = 32to per-method IL evaluation-stack tracking.This PR is based on #11252 /
trimmable-typemap-startup-fixes.InstructionEncoderwith a tracked emitter inPEAssemblyBuildercall,callvirt,newobj,newarr,ldtoken,stobj,ret, etc.) through explicit helpersmax(computed + 4, 8)so generated fat bodies get a computed value with safety padding and the ECMA-335 tiny-method floorValidation
dotnet test tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests.csproj -v minimaldotnet new maui) built as Releasenet11.0-androidforandroid-arm64with CoreCLR/trimmable typemapsemulator-5554com.companyname.mauimaxstacksmoke/crc64d3fac713b535959c.MainActivityStatus: ok,LaunchState: COLD,TotalTime: 1610ms