Use 32-bit coordinates throughout the project #13025

lhecker · 2022-05-03T02:05:02Z

Previously this project used a great variety of types to present text buffer
coordinates: short, unsigned short, int, unsigned int, size_t,
ptrdiff_t, COORD/SMALL_RECT (aka short), and more.
This massive commit migrates almost all use of those types over to the
centralized types til::point/size/rect/inclusive_rect and their
underlying type til::CoordType (aka int32_t).

Due to the size of the changeset and statistics I expect it to contain bugs.
The biggest risk I see is that some code potentially, maybe implicitly, expected
arithmetic to be mod 2^16 and that this code now allows it to be mod 2^32.
Any narrowing into short later on would then throw exceptions.

PR Checklist

Closes Rectangle, Point, and Size need concerted structure #4015
I work here
Tests added/passed

Validation Steps Performed

Casual usage of OpenConsole and Windows Terminal. ✅

DHowett · 2022-05-03T02:14:56Z

oh my god I love this image

zadjii-msft · 2022-05-03T11:49:53Z

lhecker · 2022-05-21T14:16:22Z

(A backup of the original submission description)

zadjii-msft · 2022-05-24T11:55:08Z

Codeflow link:

\\codeflow\public\cf.cmd openGitHubPr -webUrl https://github.com/microsoft/terminal/pull/13025

zadjii-msft · 2022-05-26T11:52:49Z

src/buffer/out/cursor.cpp

@@ -13,7 +13,6 @@
 // - ulSize - The height of the cursor within this buffer
 Cursor::Cursor(const ULONG ulSize, TextBuffer& parentBuffer) noexcept :
    _parentBuffer{ parentBuffer },
-    _cPosition{ 0 },


Are we sure this gets 0 initialized always? I know we used to have some weird OS-side bugs where memory wouldn't get 0 initialized. (apologies for needing a refresher on basic language concepts)

Yes, with _cPosition a til::point now, it doesn't accept a single constructor parameter anymore - it needs either 0 (_cPosition{}) or 2 (_cPosition{ 0, 0 }). But if you write the former, you can drop it from the constructor list entirely, since it's already constructed like that by default anyways.

To expand on that:
The old COORD _cPosition needed a manual initialization unlike til::point, because that type has no custom constructor which initializes X/Y to 0, nor does it use C++11's default member initializers (short X = 0;) like til::point does, so the default value for its members is "uninitialized" (which is UB).

The reason the existing code said _cPosition{ 0 } and not _cPosition{} is probably a carry-over from the C code, because C structs don't have default constructors - _cPosition{} is not valid C.
In C, if you pass less arguments than the struct has members, then the remaining fields will be set to 0. RECT{ 0 } is equivalent to RECT{ 0, 0, 0, 0 } and RECT{ 1 } is equivalent to RECT{ 1, 0, 0, 0 }. In C++, the compiler-generated constructor acts equivalent to C's with the addition of the default constructor {} which acts equivalent to { 0 }.

Since til::point has a custom constructor it gets no compiler-generated one and thus it can't be used with C-style constructor arguments anymore ({ 0 }).

src/buffer/out/LineRendition.hpp

src/buffer/out/textBufferCellIterator.cpp

zadjii-msft · 2022-05-26T13:02:24Z

src/buffer/out/UnicodeStorage.hpp

@@ -33,17 +33,17 @@ namespace std
        // - coord - the coord to hash
        // Return Value:
        // - the hashed coord
-        constexpr size_t operator()(const COORD& coord) const noexcept
+        constexpr size_t operator()(const til::point coord) const noexcept


til::point

This lost the & - intentional?

Yeah. Technically it's beneficial to pass primitive structs like that by value at all times, because x64 calling conventions / ABIs pass them via registers. Any x64 ABI will pass the 8 byte large til::point within a single register (RCX on Windows, RDI everywhere else) which means that it's not just super fast, but also that no stack needs to be allocated, etc. This might not matter for functions that are only called from time to time, but some functions we got are called thousands of times per second. This is why I pass til::point and til::size by value throughout the project - it's just consistently very very performant.

Now the problem is that the Windows x64 ABI has a very unfortunate, very important flaw as compared to the SYSV ABI used by every single other x64 OS in the world and it makes me very very sad every single time I'm reminded of this. 🥲
...It can't pass structs larger than 8 byte via registers. With the SYSV ABI a 16 byte struct simply gets passed in the first two 8 byte large registers (RDI/RSI), but on Windows such arguments are passed as "hidden references/pointers" - the argument is still passed via a register (RCX), but the register now contains a pointer to the struct on the caller's stack. The callee will then copy it on their stack to create a by-value copy (via SSE's movups/movaps). Sometimes the optimizer can optimize the copy away, but usually it'll fail at doing so. The difference is a 4-5 fold increase in runtime cost for function calls, just by merely having a single by-value 16-byte struct as an argument.
This is why I pass til::rect and til::inclusive_rect consistently by-reference, because it prevents the VS 2022 (v143) compiler from creating such redundant, very costly stack copies on the callee side.
(I've been cooperating with DevDiv/Intel on optimizing these redundant stack copies away in the most common cases and it should hopefully land in a future VS 2022 update soonish. It reduces the overhead from 4-5x down to around 2x which is then about as good as passing it by-reference.)

Wow that's incredibly thorough. We should have this saved somewhere (niksa.md maybe) as a reference that til::point&size should always be by value and rect always by ref.

Thanks!

Yes, please do write this up as an addendum to Niksa.md

zadjii-msft · 2022-05-26T14:09:41Z

src/host/ut_host/ScreenBufferTests.cpp

-    Log::Comment(NoThrowString().Format(L"Original buffer size: %dx%d", oldSize.width, oldSize.height));
-    Log::Comment(NoThrowString().Format(L"Original viewport: %dx%d", oldView.width, oldView.height));
+    Log::Comment(NoThrowString().Format(L"Original buffer size: %dx%d", oldSize.X, oldSize.Y));
+    Log::Comment(NoThrowString().Format(L"Original viewport: %dx%d", oldView.X, oldView.Y));


this one seems wrong, yea?

If no other issues turn up I might just leave it as it is, because I'm going to fix all those members in a follow-up anyways.

src/renderer/vt/state.cpp

src/terminal/adapter/FontBuffer.cpp

src/terminal/adapter/FontBuffer.hpp

…t-coord

DHowett

96/234

DHowett · 2022-06-01T22:55:16Z

src/types/UiaTextRangeBase.cpp

@@ -983,7 +983,7 @@ CATCH_RETURN();
 // - the text that the UiaTextRange encompasses
 #pragma warning(push)
 #pragma warning(disable : 26447) // compiler isn't filtering throws inside the try/catch
-std::wstring UiaTextRangeBase::_getTextValue(std::optional<unsigned int> maxLength) const
+std::wstring UiaTextRangeBase::_getTextValue(til::CoordType maxLength) const


if I could admit: i think optional captures the essence of this API better than "magic -1" does

I understand where you're coming from, but this method exists only to implement ITextRangeProvider::GetText which says:

The maximum length of the string to return. Use -1 if no limit is required.

I feel like this is a direct translation of this API. Additionally I personally think that negative lengths are very common in C code (especially if cast to unsigned integers).

src/types/UiaTextRangeBase.cpp

src/renderer/uia/UiaRenderer.cpp

src/renderer/base/renderer.cpp

lhecker · 2022-06-02T19:39:24Z

i feel like there's a much better HRESULT for this than "win32 unhandled exception"; perhaps E_BOUNDS or WIN32(ERROR_ARITHMETIC_OVERFLOW)?

I chose this so that it generates the same HRESULT as gsl::narrow would if used with wil's exception helpers. Unfortunately wil has no option to be gsl-aware. I think this consistency is important so that you can see an ERROR_UNHANDLED_EXCEPTION and already know that a narrowing failure is the most likely cause.
I believe it'd be better if we replace all gsl::narrow calls with til::narrow ones which throw an ERROR_ARITHMETIC_OVERFLOW exception so that everything continues to be consistent.
What do you think? 🤔

DHowett

139/237! less than a hundo to go

src/cascadia/WindowsTerminal/AppHost.cpp

src/host/CommandListPopup.cpp

src/host/VtApiRoutines.cpp

src/host/_output.cpp

src/host/convarea.cpp

DHowett

154

src/types/viewport.cpp

src/renderer/gdi/paint.cpp

src/renderer/gdi/math.cpp

src/renderer/gdi/invalidate.cpp

src/host/outputStream.cpp

src/host/ut_host/ClipboardTests.cpp

DHowett

ALRIGHT! Get them concerns addressed and we are a GO!

DHowett · 2022-06-02T22:36:08Z

src/host/ut_host/CommandListPopupTests.cpp

@@ -311,7 +311,7 @@ class CommandListPopupTests

        VERIFY_ARE_EQUAL(popup.Process(cookedReadData), static_cast<NTSTATUS>(CONSOLE_STATUS_WAIT_NO_BLOCK));
        // selection should have moved up a page
-        VERIFY_ARE_EQUAL(static_cast<short>(m_pHistory->GetNumberOfCommands()) - popup.Height() - 1, popup._currentCommand);
+        VERIFY_ARE_EQUAL(static_cast<til::CoordType>(m_pHistory->GetNumberOfCommands()) - popup.Height() - 1, popup._currentCommand);


this isn't exactly a coordinate either...

The popup.Height() is in character cells and so I made it return a til::CoordType.
This change makes it so that the compiler doesn't complain that we should promote integers first before we add/subtract/multiply/divide them.

src/host/ut_host/ScreenBufferTests.cpp

DHowett · 2022-06-02T22:37:28Z

src/host/ut_host/ScreenBufferTests.cpp

@@ -1293,7 +1293,7 @@ void ScreenBufferTests::VtResizeDECCOLM()
    newViewWidth = si.GetViewport().Width();

    VERIFY_IS_FALSE(areMarginsSet());
-    VERIFY_ARE_EQUAL(COORD({ 0, 0 }), getRelativeCursorPosition());
+    VERIFY_ARE_EQUAL(til::point(0, 0), getRelativeCursorPosition());


Curious, using the constructor via function call syntax. You replaced similar thing with point{}; should we here?

The VERIFY_ARE_EQUAL macro blows up if you use curly brackets inside it. Not kidding. IIRC the ({ 0, 0 }) doesn't work because the constructor is explicit and doesn't take a single parameter list argument (unlike structs).

src/host/ut_host/UtilsTests.cpp

DHowett · 2022-06-02T22:45:39Z

src/host/ut_host/UtilsTests.cpp


        return s;
    }

-    void FillBothCoordsSameRandom(COORD* pcoordA, COORD* pcoordB)
+    void FillBothCoordsSameRandom(til::point* pcoordA, til::point* pcoordB)


This seems like a very silly way to do a = rand(); b = a;

DHowett · 2022-06-02T23:02:53Z

src/renderer/base/renderer.cpp

@@ -1262,14 +1237,14 @@ void Renderer::_PaintSelection(_In_ IRenderEngine* const pEngine)
 // - Helper to determine the selected region of the buffer.
 // Return Value:
 // - A vector of rectangles representing the regions to select, line by line.
-std::vector<SMALL_RECT> Renderer::_GetSelectionRects() const
+std::vector<til::rect> Renderer::_GetSelectionRects() const


Double check this code. I saw that INSIDE the renderer they're exclusive now, but via RenderData and OUTSIDE the renderer they're inclusive. That's fine, but look at line R1284 -- we bump up the inclusive SR to be exclusive, and then we construct a til::rect out of it making it exclusive again...

src/buffer/out/textBuffer.cpp

src/cascadia/TerminalCore/Terminal.cpp

DHowett · 2022-06-02T23:10:45Z

src/cascadia/TerminalCore/Terminal.cpp

@@ -329,7 +329,7 @@ std::wstring_view Terminal::GetWorkingDirectory()
        oldRows.mutableViewportTop = oldViewportTop;
        oldRows.visibleViewportTop = newVisibleTop;

-        const std::optional<short> oldViewStart{ oldViewportTop };
+        const std::optional oldViewStart{ oldViewportTop };


I, too, am surprised!

src/cascadia/TerminalCore/Terminal.cpp

DHowett · 2022-06-03T21:00:01Z

Oh no!

StartGroup: FillOutputTests::WriteWideGlyphUnicode
Verify: AreNotEqual(hConsole, INVALID_HANDLE_VALUE): Ensure we got a valid console handle
Verify: IsNotNull(hConsole): Ensure we got a non-null console handle
Verify: Win32BoolSucceeded(FillConsoleOutputCharacterW(hConsole, L'\x304F', 1, { 0, 0 }, &charsWritten))
Error: Verify: AreEqual(1u, charsWritten) - Values (1, 2) [File: C:\a\_work\1\s\src\host\ft_host\API_FillOutputTests.cpp, Function: FillOutputTests::WriteWideGlyphUnicode, Line: 92]
EndGroup: FillOutputTests::WriteWideGlyphUnicode [Failed]

ghost · 2022-06-03T23:02:35Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This is a follow-up of #13025 to make the members of `til::point/size/rect` uniform and consistent without the use of `unions`. The only file that has any changes is `src/host/getset.cpp` where an if condition was simplified. ## Validation Steps Performed * Host unit tests ✅ * Host feature tests ✅ * ControlCore feature tests ✅

lhecker mentioned this pull request May 3, 2022

Merge the TerminalDispatch and AdaptDispatch classes #13024

Merged

6 tasks

lhecker force-pushed the dev/lhecker/4015-32bit-coord branch from 680f4e0 to b70ef3d Compare May 5, 2022 01:27

zadjii-msft added this to the Terminal v1.15 milestone May 6, 2022

lhecker force-pushed the dev/lhecker/4015-32bit-coord branch 9 times, most recently from 2e5808f to 97c4048 Compare May 17, 2022 21:32

lhecker force-pushed the dev/lhecker/4015-32bit-coord branch from ba60e7b to 34fc317 Compare May 18, 2022 14:10

Use 32-bit coordinates throughout the project

6d2f72c

lhecker force-pushed the dev/lhecker/4015-32bit-coord branch from ca6498f to 6d2f72c Compare May 21, 2022 14:15

lhecker changed the title ~~[WIP] Use 32-bit coordinates throughout the project~~ Use 32-bit coordinates throughout the project May 21, 2022

lhecker marked this pull request as ready for review May 21, 2022 15:10

zadjii-msft reviewed May 26, 2022

View reviewed changes

src/buffer/out/LineRendition.hpp Show resolved Hide resolved

zadjii-msft reviewed May 26, 2022

View reviewed changes

src/buffer/out/textBufferCellIterator.cpp Show resolved Hide resolved

zadjii-msft reviewed May 26, 2022

View reviewed changes