From 71732fd176f9a0d7a6ff8f60b8ec2a01ed081e9d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 9 Aug 2023 17:31:04 +0200 Subject: [PATCH 1/2] JIT: Fix emitSplit to properly handle the remainder emitSplit did not handle the edge case where the last instruction group of the function/funclet resulted in the size to overflow the max. Use a lambda so we can call the necessary code both as part of the loop and after the loop. Fix #85063 --- src/coreclr/jit/emit.cpp | 101 ++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 48 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 97de8d503606c9..6a34dcc5a2c039 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2964,68 +2964,70 @@ void emitter::emitSplit(emitLocation* startLoc, UNATIVE_OFFSET curSize; UNATIVE_OFFSET candidateSize; - for (igPrev = NULL, ig = igLastReported = igStart, igLastCandidate = NULL, candidateSize = 0, curSize = 0; - ig != igEnd && ig != NULL; igPrev = ig, ig = ig->igNext) - { - // Keep looking until we've gone past the maximum split size - if (curSize >= maxSplitSize) + auto splitIfNecessary = [&]() { + if (curSize < maxSplitSize) { - bool reportCandidate = true; + return; + } - // Is there a candidate? - if (igLastCandidate == NULL) - { + // Is there a candidate? + if (igLastCandidate == NULL) + { #ifdef DEBUG - if (EMITVERBOSE) - printf("emitSplit: can't split at IG%02u; we don't have a candidate to report\n", ig->igNum); + if (EMITVERBOSE) + printf("emitSplit: can't split at IG%02u; we don't have a candidate to report\n", ig->igNum); #endif - reportCandidate = false; - } + return; + } - // Don't report the same thing twice (this also happens for the first block, since igLastReported is - // initialized to igStart). - if (igLastCandidate == igLastReported) - { + // Don't report the same thing twice (this also happens for the first block, since igLastReported is + // initialized to igStart). + if (igLastCandidate == igLastReported) + { #ifdef DEBUG - if (EMITVERBOSE) - printf("emitSplit: can't split at IG%02u; we already reported it\n", igLastCandidate->igNum); + if (EMITVERBOSE) + printf("emitSplit: can't split at IG%02u; we already reported it\n", igLastCandidate->igNum); #endif - reportCandidate = false; - } + return; + } - // Don't report a zero-size candidate. This will only occur in a stress mode with JitSplitFunctionSize - // set to something small, and a zero-sized IG (possibly inserted for use by the alignment code). Normally, - // the split size will be much larger than the maximum size of an instruction group. The invariant we want - // to maintain is that each fragment contains a non-zero amount of code. - if (reportCandidate && (candidateSize == 0)) - { + // Don't report a zero-size candidate. This will only occur in a stress mode with JitSplitFunctionSize + // set to something small, and a zero-sized IG (possibly inserted for use by the alignment code). Normally, + // the split size will be much larger than the maximum size of an instruction group. The invariant we want + // to maintain is that each fragment contains a non-zero amount of code. + if (candidateSize == 0) + { #ifdef DEBUG - if (EMITVERBOSE) - printf("emitSplit: can't split at IG%02u; zero-sized candidate\n", igLastCandidate->igNum); + if (EMITVERBOSE) + printf("emitSplit: can't split at IG%02u; zero-sized candidate\n", igLastCandidate->igNum); #endif - reportCandidate = false; - } + return; + } + +// Report it! - // Report it! - if (reportCandidate) - { #ifdef DEBUG - if (EMITVERBOSE) - { - printf("emitSplit: split at IG%02u is size %d, %s than requested maximum size of %d\n", - igLastCandidate->igNum, candidateSize, (candidateSize >= maxSplitSize) ? "larger" : "less", - maxSplitSize); - } + if (EMITVERBOSE) + { + printf("emitSplit: split at IG%02u is size %x, %s than requested maximum size of %x\n", + igLastCandidate->igNum, candidateSize, (candidateSize >= maxSplitSize) ? "larger" : "less", + maxSplitSize); + } #endif - // hand memory ownership to the callback function - emitLocation* pEmitLoc = new (emitComp, CMK_Unknown) emitLocation(igLastCandidate); - callbackFunc(context, pEmitLoc); - igLastReported = igLastCandidate; - igLastCandidate = NULL; - curSize -= candidateSize; - } - } + // hand memory ownership to the callback function + emitLocation* pEmitLoc = new (emitComp, CMK_Unknown) emitLocation(igLastCandidate); + callbackFunc(context, pEmitLoc); + igLastReported = igLastCandidate; + igLastCandidate = NULL; + curSize -= candidateSize; + }; + + for (igPrev = NULL, ig = igLastReported = igStart, igLastCandidate = NULL, candidateSize = 0, curSize = 0; + ig != igEnd && ig != NULL; igPrev = ig, ig = ig->igNext) + { + splitIfNecessary(); + assert(curSize < maxSplitSize); // Update the current candidate to be this block, if it isn't in the middle of a // prolog or epilog, which we can't split. All we know is that certain @@ -3046,6 +3048,9 @@ void emitter::emitSplit(emitLocation* startLoc, curSize += ig->igSize; } // end for loop + + splitIfNecessary(); + assert(curSize < maxSplitSize); } /***************************************************************************** From 3255c86fdfdcf73ae6e44397cb56cbc9cf37bed9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 9 Aug 2023 18:38:19 +0200 Subject: [PATCH 2/2] Remove unsafe assert --- src/coreclr/jit/emit.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 6a34dcc5a2c039..f963ec94453293 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3027,7 +3027,6 @@ void emitter::emitSplit(emitLocation* startLoc, ig != igEnd && ig != NULL; igPrev = ig, ig = ig->igNext) { splitIfNecessary(); - assert(curSize < maxSplitSize); // Update the current candidate to be this block, if it isn't in the middle of a // prolog or epilog, which we can't split. All we know is that certain