From dd2fbef39dd477a92d0fda9839532c27159a99c6 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Wed, 9 Oct 2019 11:01:15 -0700 Subject: [PATCH] Move the VT parser's parse state into instanced storage (#3110) The VT parser used to be keeping a boolean used to determine whether it was in bulk or single-character parse mode in a function-level static. That turned out to not be great. Fixes #3108; fixes #3073. --- src/terminal/parser/stateMachine.cpp | 19 ++- src/terminal/parser/stateMachine.hpp | 4 + .../parser/ut_parser/Parser.UnitTests.vcxproj | 1 + .../Parser.UnitTests.vcxproj.filters | 8 +- .../parser/ut_parser/StateMachineTest.cpp | 113 ++++++++++++++++++ src/terminal/parser/ut_parser/sources | 1 + 6 files changed, 134 insertions(+), 12 deletions(-) create mode 100644 src/terminal/parser/ut_parser/StateMachineTest.cpp diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 085db8cadf4..0a656342b7f 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -25,7 +25,8 @@ StateMachine::StateMachine(IStateMachineEngine* const pEngine) : // rgusParams Initialized below _sOscNextChar(0), _sOscParam(0), - _currRunLength(0) + _currRunLength(0), + _fProcessingIndividually(false) { ZeroMemory(_pwchOscStringBuffer, sizeof(_pwchOscStringBuffer)); ZeroMemory(_rgusParams, sizeof(_rgusParams)); @@ -1346,20 +1347,16 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) _pwchSequenceStart = rgwch; _currRunLength = 0; - // This should be static, because if one string starts a sequence, and the next finishes it, - // we want the partial sequence state to persist. - static bool s_fProcessIndividually = false; - for (size_t cchCharsRemaining = cch; cchCharsRemaining > 0; cchCharsRemaining--) { - if (s_fProcessIndividually) + if (_fProcessingIndividually) { // If we're processing characters individually, send it to the state machine. ProcessCharacter(*_pwchCurr); _pwchCurr++; if (_state == VTStates::Ground) // Then check if we're back at ground. If we are, the next character (pwchCurr) { // is the start of the next run of characters that might be printable. - s_fProcessIndividually = false; + _fProcessingIndividually = false; _pwchSequenceStart = _pwchCurr; _currRunLength = 0; } @@ -1371,13 +1368,13 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) FAIL_FAST_IF(!(_pwchSequenceStart + _currRunLength <= rgwch + cch)); _pEngine->ActionPrintString(_pwchSequenceStart, _currRunLength); // ... print all the chars leading up to it as part of the run... _trace.DispatchPrintRunTrace(_pwchSequenceStart, _currRunLength); - s_fProcessIndividually = true; // begin processing future characters individually... + _fProcessingIndividually = true; // begin processing future characters individually... _currRunLength = 0; _pwchSequenceStart = _pwchCurr; ProcessCharacter(*_pwchCurr); // ... Then process the character individually. if (_state == VTStates::Ground) // If the character took us right back to ground, start another run after it. { - s_fProcessIndividually = false; + _fProcessingIndividually = false; _pwchSequenceStart = _pwchCurr + 1; _currRunLength = 0; } @@ -1391,13 +1388,13 @@ void StateMachine::ProcessString(const wchar_t* const rgwch, const size_t cch) } // If we're at the end of the string and have remaining un-printed characters, - if (!s_fProcessIndividually && _currRunLength > 0) + if (!_fProcessingIndividually && _currRunLength > 0) { // print the rest of the characters in the string _pEngine->ActionPrintString(_pwchSequenceStart, _currRunLength); _trace.DispatchPrintRunTrace(_pwchSequenceStart, _currRunLength); } - else if (s_fProcessIndividually) + else if (_fProcessingIndividually) { // One of the "weird things" in VT input is the case of something like // alt+[. In VT, that's encoded as `\x1b[`. However, that's diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index ce6980191e5..a4e71a69bd9 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -150,5 +150,9 @@ namespace Microsoft::Console::VirtualTerminal const wchar_t* _pwchCurr; const wchar_t* _pwchSequenceStart; size_t _currRunLength; + + // This is tracked per state machine instance so that separate calls to Process* + // can start and finish a sequence. + bool _fProcessingIndividually; }; } diff --git a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj index dd7acda4668..65773c0f48c 100644 --- a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj +++ b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj @@ -12,6 +12,7 @@ + Create diff --git a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters index 6aeb1ba04c5..ecc61100147 100644 --- a/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters +++ b/src/terminal/parser/ut_parser/Parser.UnitTests.vcxproj.filters @@ -15,7 +15,13 @@ - + + Source Files + + + Source Files + + Source Files diff --git a/src/terminal/parser/ut_parser/StateMachineTest.cpp b/src/terminal/parser/ut_parser/StateMachineTest.cpp new file mode 100644 index 00000000000..7235b1ecce0 --- /dev/null +++ b/src/terminal/parser/ut_parser/StateMachineTest.cpp @@ -0,0 +1,113 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" +#include "WexTestClass.h" +#include "../../inc/consoletaeftemplates.hpp" + +#include "stateMachine.hpp" + +using namespace WEX::Common; +using namespace WEX::Logging; +using namespace WEX::TestExecution; + +namespace Microsoft +{ + namespace Console + { + namespace VirtualTerminal + { + class StateMachineTest; + class TestStateMachineEngine; + }; + }; +}; + +using namespace Microsoft::Console::VirtualTerminal; + +class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStateMachineEngine +{ +public: + bool ActionExecute(const wchar_t /* wch */) override { return true; }; + bool ActionExecuteFromEscape(const wchar_t /* wch */) override { return true; }; + bool ActionPrint(const wchar_t /* wch */) override { return true; }; + bool ActionPrintString(const wchar_t* const /* rgwch */, + size_t const /* cch */) override { return true; }; + + bool ActionPassThroughString(const wchar_t* const /* rgwch */, + size_t const /* cch */) override { return true; }; + + bool ActionEscDispatch(const wchar_t /* wch */, + const unsigned short /* cIntermediate */, + const wchar_t /* wchIntermediate */) override { return true; }; + + bool ActionClear() override { return true; }; + + bool ActionIgnore() override { return true; }; + + bool ActionOscDispatch(const wchar_t /* wch */, + const unsigned short /* sOscParam */, + wchar_t* const /* pwchOscStringBuffer */, + const unsigned short /* cchOscString */) override { return true; }; + + bool ActionSs3Dispatch(const wchar_t /* wch */, + const unsigned short* const /* rgusParams */, + const unsigned short /* cParams */) override { return true; }; + + bool FlushAtEndOfString() const override { return false; }; + bool DispatchControlCharsFromEscape() const override { return false; }; + bool DispatchIntermediatesFromEscape() const override { return false; }; + + // ActionCsiDispatch is the only method that's actually implemented. + bool ActionCsiDispatch(const wchar_t /* wch */, + const unsigned short /* cIntermediate */, + const wchar_t /* wchIntermediate */, + _In_reads_(cParams) const unsigned short* const rgusParams, + const unsigned short cParams) override + { + csiParams.emplace(rgusParams, rgusParams + cParams); + return true; + } + + // This will only be populated if ActionCsiDispatch is called. + std::optional> csiParams; +}; + +class Microsoft::Console::VirtualTerminal::StateMachineTest +{ + TEST_CLASS(StateMachineTest); + + TEST_CLASS_SETUP(ClassSetup) + { + return true; + } + + TEST_CLASS_CLEANUP(ClassCleanup) + { + return true; + } + + TEST_METHOD(TwoStateMachinesDoNotInterfereWithEachother); +}; + +void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother() +{ + auto firstEnginePtr{ std::make_unique() }; + // this dance is required because StateMachine presumes to take ownership of its engine. + const auto& firstEngine{ *firstEnginePtr.get() }; + StateMachine firstStateMachine{ firstEnginePtr.release() }; + + auto secondEnginePtr{ std::make_unique() }; + const auto& secondEngine{ *secondEnginePtr.get() }; + StateMachine secondStateMachine{ secondEnginePtr.release() }; + + firstStateMachine.ProcessString(L"\x1b[12"); // partial sequence + secondStateMachine.ProcessString(L"\x1b[3C"); // full sequence on second parser + firstStateMachine.ProcessString(L";34m"); // completion to previous partial sequence on first parser + + std::vector expectedFirstCsi{ 12u, 34u }; + std::vector expectedSecondCsi{ 3u }; + + VERIFY_ARE_EQUAL(expectedFirstCsi, firstEngine.csiParams); + VERIFY_ARE_EQUAL(expectedSecondCsi, secondEngine.csiParams); +} diff --git a/src/terminal/parser/ut_parser/sources b/src/terminal/parser/ut_parser/sources index 32867cc0ff3..5e7f6c61e11 100644 --- a/src/terminal/parser/ut_parser/sources +++ b/src/terminal/parser/ut_parser/sources @@ -23,6 +23,7 @@ SOURCES = \ $(SOURCES) \ OutputEngineTest.cpp \ InputEngineTest.cpp \ + StateMachineTest.cpp \ # The InputEngineTest requires VTRedirMapVirtualKeyW, which means we need the # ServiceLocator, which means we need the entire host and all it's dependencies,