Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: Fix operand evaluation order for GT_INDEX_ADDR (port of #20047)#20083

Closed
AndyAyersMS wants to merge 1 commit into
dotnet:release/2.1from
AndyAyersMS:PortFixFor20040ToRelease2.1
Closed

JIT: Fix operand evaluation order for GT_INDEX_ADDR (port of #20047)#20083
AndyAyersMS wants to merge 1 commit into
dotnet:release/2.1from
AndyAyersMS:PortFixFor20040ToRelease2.1

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

We need to evaluate the array operand first, and it's op1. So evaluate
in that order, and don't allow reversal.

Closes #20040.

We need to evaluate the array operand first, and it's op1. So evaluate
in that order, and don't allow reversal.

Closes #20040.
@AndyAyersMS AndyAyersMS added area-CodeGen Servicing-consider Issue for next servicing release review labels Sep 20, 2018
@AndyAyersMS
Copy link
Copy Markdown
Member Author

Description

In "minopts" mode the jit could generate silent bad code for certain sequences where a local was passed by-reference to a call and then used to index an array.

Customer Impact

Impacts simple examples built from Orleans when tiering is enabled. They found and reported the bug.

Codegen bug leasds to wrong behavior or unexpected exceptions. Type and memory safety are not compromised.

If the customer encounters this bug only with tiering enabled they can work around it by disabling tiering.

Tiering is not enabled by default in 2.1 but minopts mode is reachable by other means (eg very large method bodies) and is intended as the high-reliability jit mode. So recommending a fix here even though the likelihood of this surfacing in 2.1 without enabling tiering is small.

Tiering is enabled by default in 2.2 currently.

Regression?

This change in minopts behavior was introduced by #13245 in 2.1, so it is a regression vs 2.0 behavior.

Risk

Fairly low. The jit change prevents an invalid reordering of expressions. A run through ~190K methods jitted with minopts showed about 1K methods with code diffs.

cc @RussKeldorph

@AndyAyersMS
Copy link
Copy Markdown
Member Author

We are going to fix this in 2.2.

@danmoseley danmoseley removed the Servicing-consider Issue for next servicing release review label Oct 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants