[release/8.0-staging] JIT: Fix loop recognition bug in .NET 8

[AndyAyersMS]

A loop preheader can't be a callfinally pair tail.

Fixes #108811.

Customer Impact

• Customer reported
• Found internally

Silent bad code generation. Programs may return incorrect results after running for a while. Failure happens once key methods are tiered up.

At a source level this requires a try/finally (say from a using) with a loop with invariant expression immediately thereafter, for instance:

using (...)
{ 

}

while (...)
{
    = ... <some loop invariant computation>
}

In the JIT IR, the using is commonly followed with code to invoke the finally, and this finally invocation code sequence has special restrictions that are not always being honored.

In particular the optimizer may attempt to hoist the invariant computation out of the loop and inadvertently place the computation in a "callfinally pair tail" block that is supposed to remain empty. And doing hosting this way leads to improper code generation for the hoisted value.

Similar constructs can arise from other source patterns or via inlining.

Regression

• Yes
• No

From the linked bug, the customer indicates the program ran properly on .NET 6.

Testing

Verified the customer test case is now fixed. Issue was flagged by a checked build of the jit, so we know it is uncommon.

We currently do not have SPMI artifacts for .NET 8 but can try and recreate them to verify that there are no unexpected changes.

This part of the JIT was substantially revised in .NET 9, which does not have this bug.

Risk

Low, this inhibits recognition of certain problematic loops and ensuing loop optimizations.

[Copilot]

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• src/tests/JIT/Regression/JitBlue/Runtime_108811/Runtime_108811.csproj: Language not supported

[Copilot]

The while-loop condition checks the array element before confirming that decryptedByteCount is greater than 0, which could lead to an out-of-range access. Consider swapping the operands to 'while (decryptedByteCount > 0 && decryptedData[decryptedByteCount - 1] == 0)'.

Suggested change

	while (decryptedData[decryptedByteCount - 1] == 0 && decryptedByteCount > 0)
	while (decryptedByteCount > 0 && decryptedData[decryptedByteCount - 1] == 0)

[AndyAyersMS]

@BruceForstall PTAL
cc @dotnet/jit-contrib

fyi @jeffschwMSFT

[jeffschwMSFT]

lgtm. we will take for consideration in 8.0.x

[JulieLeeMSFT]

@carlossanlop, please merge this PR.

[carlossanlop]

Hi @JulieLeeMSFT ! PR owners can merge their own PRs in the staging branches whenever they're ready. No need to wait for me to merge them. All team members have merge permission here.

[carlossanlop]

@JulieLeeMSFT I think I see why you're asking for me to merge. The PR's "Squash & merge" button is blocked by Build Analysis: This leg always fails in release branches by design, because we need PR owners to look at the failures and confirm nothing is caused by their change. If it looks good and existing failures are all unrelated, you can add the /ba-g <text> comment to make that leg pass.

[carlossanlop]

I see a new failure but it does not seem related to this PR. Let me create a known build issue for it, give me a few mins:

Exception Message

Algorithm 'Aes' is not supported on this platform.

CallStack

   at System.Security.Cryptography.AesImplementation.CreateTransformCore(CipherMode cipherMode, PaddingMode paddingMode, Byte[] key, Byte[] iv, Int32 blockSize, Int32 paddingSize, Int32 feedback, Boolean encrypting)
   at System.Security.Cryptography.AesImplementation.CreateTransform(Byte[] rgbKey, Byte[] rgbIV, Boolean encrypting)
   at System.Security.Cryptography.AesImplementation.CreateDecryptor(Byte[] rgbKey, Byte[] rgbIV)
   at Runtime_108811.Decrypt(Byte[] buffer, String key, String iv, Byte[]& decryptedData)
   at Runtime_108811.Test()
   at Program.<>c__DisplayClass0_0.<<Main>$>g__TestExecutor5|6(TestFilter filter, StreamWriter tempLogSw, StreamWriter statsCsvSw)

[AndyAyersMS]

/ba-g

Failure is #114123

Failure is the issue Carlos spotted (a test case added for this bug, evidently not wasm compatible?)

[carlossanlop]

Ah didn't notice that issue @AndyAyersMS. I ended up opening an issue explicitly mentioning the 'Aes not supported on this platform' error: #114552

Edit: I saw your edit 😄

[AndyAyersMS]

Ah didn't notice that issue @AndyAyersMS . I ended up opening an issue explicitly mentioning the 'Aes not supported on this platform' error: #114552

No you were right -- the issue is the test I just added for this bug doesn't seem to be wasm-friendly. The test case is not in any other branch yet (this bug doesn't happen in main or with .net 9 so this was not a backport).

I don't know the right exclusion syntax just yet...

[AndyAyersMS]

I don't know the right exclusion syntax just yet...

Think it's just adding to issues.target. Is there time to push a fix for this? Or should I merge and PR that separately?

[akoeplinger]

Code complete is on Monday April 14th so there is time. If you run into problems with issues.targets you can alternatively use if (OperatingSystem.IsBrowser()) to skip the code.

[AndyAyersMS]

runtime (Build browser-wasm linux Release AllSubsets_Mono_RuntimeTests monointerpreter) is dead but CI doesn't realize it.

[akoeplinger]

yeah, Build Analysis agrees. this is good to go.