From b65ffdb2810b3e6673e84e4daeed863efc04b246 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Tue, 29 Nov 2022 22:07:37 +0000 Subject: [PATCH] Merged PR 8189936: [Git2Git] Lift an optional check out of RefreshRowIDs loop to fix a crash In the most recent compiler ingestion into Windows ("LKG14"), we found that this particular construction--checking an optional for a value during this range-for loop--resulted in bad code generation. When optimized, it generates code that looks effectively like this: ```c++ if (!newRowWidth.has_value()) { while (true) { // do the row stuff... ++it; } } ``` The loop never exits, and `_RefreshRowIDs` walks off the end of the buffer. Whoops. This commit fixes that issue by tricking the optimizer to go another way. Leonard tells me it's harmless to call `Resize` a bunch of times, even if it's a no-op, so I trust that this change results in the right outcome with none of the crashing. Fixes MSFT-41456525 Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_we_adept_e4d2 c2b3697c867bddf5660da8b222e99ff4bfd1ea5b (cherry picked from commit 3c104440a897e551b867225d9fb8cfe4ff158dd8) --- src/buffer/out/textBuffer.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 21f153d1794..ce7a8ed980d 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -1032,6 +1032,7 @@ void TextBuffer::_RefreshRowIDs(std::optional newRowWidth) { std::unordered_map rowMap; til::CoordType i = 0; + til::CoordType newWidth = newRowWidth.value_or(_storage.at(0).size()); for (auto& it : _storage) { // Build a map so we can update Unicode Storage @@ -1043,12 +1044,13 @@ void TextBuffer::_RefreshRowIDs(std::optional newRowWidth) // Also update the char row parent pointers as they can get shuffled up in the rotates. it.GetCharRow().UpdateParent(&it); - // Resize the rows in the X dimension if we have a new width - if (newRowWidth.has_value()) - { - // Realloc in the X direction - THROW_IF_FAILED(it.Resize(newRowWidth.value())); - } + // Realloc in the X direction + // BODGY: We unpack the optional early and resize here unconditionally + // due to a codegen issue in LKG14. We used to check the optional in + // every iteration of the loop, but that resulted in the optimizer + // emitting a copy of the loop, used when the optional was empty, that + // never exited. Oops. + THROW_IF_FAILED(it.Resize(newWidth)); } // Give the new mapping to Unicode Storage