Skip to content

Commit

Permalink
JIT: Add bounds check for ordinals in ThreeOptLayout::ConsiderEdge (d…
Browse files Browse the repository at this point in the history
…otnet#111719)

When setting up the data structures needed for 3-opt layout, we allocate an array of basic blocks that is large enough to fit everything reachable via a RPO traversal. However, we only initialize as much of the array as needed to fit the blocks hot enough to be considered for reordering, so we frequently have uninitialized space at the end of the array. Thus, our current strategy to check if a block is in the set of candidates takes two steps:

Check if the block's ordinal is less than the number of hot blocks. If it isn't, then it cannot possibly be in the set.
If the ordinal is valid, check if the block in the array matches the block in question.
We were neglecting to do the first step in ThreeOptLayout::ConsiderEdge, which meant in rare cases, we would compare a block outside the candidate set to some uninitialized part of the array, and the comparison would happen to be equal. Thus, we would later consider creating fallthrough along a flow edge that breaks 3-opt's invariants.

To solve this, we can either add the bounds check mentioned above, or we can default-initialize the entire block array to ensure comparisons outside the candidate set are with nullptr. I've decided to pursue the former.

Fixes dotnet#111563.
  • Loading branch information
amanasifkhalid authored Jan 23, 2025
1 parent e20ee22 commit c5bb505
Showing 1 changed file with 6 additions and 1 deletion.
7 changes: 6 additions & 1 deletion src/coreclr/jit/fgopt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5173,7 +5173,12 @@ void Compiler::ThreeOptLayout::ConsiderEdge(FlowEdge* edge)
assert(dstPos < compiler->m_dfsTree->GetPostOrderCount());

// Don't consider edges to or from outside the hot range (i.e. ordinal doesn't match 'blockOrder' position).
if ((srcBlk != blockOrder[srcPos]) || (dstBlk != blockOrder[dstPos]))
if ((srcPos >= numCandidateBlocks) || (srcBlk != blockOrder[srcPos]))
{
return;
}

if ((dstPos >= numCandidateBlocks) || (dstBlk != blockOrder[dstPos]))
{
return;
}
Expand Down

0 comments on commit c5bb505

Please sign in to comment.