Skip to content

Commit

Permalink
Merged PR 8189936: [Git2Git] Lift an optional check out of RefreshRow…
Browse files Browse the repository at this point in the history
…IDs 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
  • Loading branch information
DHowett committed Nov 29, 2022
1 parent b6feabe commit 3c10444
Showing 1 changed file with 8 additions and 6 deletions.
14 changes: 8 additions & 6 deletions src/buffer/out/textBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1032,6 +1032,7 @@ void TextBuffer::_RefreshRowIDs(std::optional<til::CoordType> newRowWidth)
{
std::unordered_map<til::CoordType, til::CoordType> 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
Expand All @@ -1043,12 +1044,13 @@ void TextBuffer::_RefreshRowIDs(std::optional<til::CoordType> 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
Expand Down

0 comments on commit 3c10444

Please sign in to comment.