maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Replace gsl::at with a new til::at(span) for pre-checked bounds (#6925)

Browse source 
The recent changes to use gsl::span everywhere added a few bounds checks
along codepaths where we were already checking bounds. Some of them may
be non-obvious to the optimizer, so we can now use til::at to help them
along.

To accomplish this, I've added a new overload of til::at that takes a
span and directly accesses its backing buffer.
This commit is contained in:
Dustin L. Howett 2020-07-15 10:29:36 -07:00 committed by GitHub
parent 80da24ecf8
commit 09471c3753
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 81 additions and 46 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/buffer/out/AttrRow.cpp
View file

@ -250,7 +250,7 @@ void ATTR_ROW::ReplaceAttrs(const TextAttribute& toBeReplacedAttr, const TextAtt
if (newAttrs.size() == 1)
{
// Get the new color attribute we're trying to apply
const TextAttribute NewAttr = gsl::at(newAttrs, 0).GetAttributes();
const TextAttribute NewAttr = til::at(newAttrs, 0).GetAttributes();
// If the existing run was only 1 element...
// ...and the new color is the same as the old, we don't have to do anything and can exit quick.

12
src/buffer/out/OutputCellIterator.cpp
View file

@ -114,7 +114,7 @@ OutputCellIterator::OutputCellIterator(const std::wstring_view utf16Text, const
// - legacyAttrs - One legacy color item per cell
OutputCellIterator::OutputCellIterator(const gsl::span<const WORD> legacyAttrs) noexcept :
_mode(Mode::LegacyAttr),
_currentView(s_GenerateViewLegacyAttr(gsl::at(legacyAttrs, 0))),
_currentView(s_GenerateViewLegacyAttr(til::at(legacyAttrs, 0))),
_run(legacyAttrs),
_attr(InvalidTextAttribute),
_distance(0),
@ -129,7 +129,7 @@ OutputCellIterator::OutputCellIterator(const gsl::span<const WORD> legacyAttrs)
// - charInfos - Multiple cell with unicode text and legacy color data.
OutputCellIterator::OutputCellIterator(const gsl::span<const CHAR_INFO> charInfos) noexcept :
_mode(Mode::CharInfo),
_currentView(s_GenerateView(gsl::at(charInfos, 0))),
_currentView(s_GenerateView(til::at(charInfos, 0))),
_run(charInfos),
_attr(InvalidTextAttribute),
_distance(0),
@ -144,7 +144,7 @@ OutputCellIterator::OutputCellIterator(const gsl::span<const CHAR_INFO> charInfo
// - cells - Multiple cells in a run
OutputCellIterator::OutputCellIterator(const gsl::span<const OutputCell> cells) :
_mode(Mode::Cell),
_currentView(s_GenerateView(gsl::at(cells, 0))),
_currentView(s_GenerateView(til::at(cells, 0))),
_run(cells),
_attr(InvalidTextAttribute),
_distance(0),
@ -265,7 +265,7 @@ OutputCellIterator& OutputCellIterator::operator++()
_pos++;
if (operator bool())
{
_currentView = s_GenerateView(gsl::at(std::get<gsl::span<const OutputCell>>(_run), _pos));
_currentView = s_GenerateView(til::at(std::get<gsl::span<const OutputCell>>(_run), _pos));
}
break;
}
@ -275,7 +275,7 @@ OutputCellIterator& OutputCellIterator::operator++()
_pos++;
if (operator bool())
{
_currentView = s_GenerateView(gsl::at(std::get<gsl::span<const CHAR_INFO>>(_run), _pos));
_currentView = s_GenerateView(til::at(std::get<gsl::span<const CHAR_INFO>>(_run), _pos));
}
break;
}
@ -285,7 +285,7 @@ OutputCellIterator& OutputCellIterator::operator++()
_pos++;
if (operator bool())
{
_currentView = s_GenerateViewLegacyAttr(gsl::at(std::get<gsl::span<const WORD>>(_run), _pos));
_currentView = s_GenerateViewLegacyAttr(til::at(std::get<gsl::span<const WORD>>(_run), _pos));
}
break;
}

8
src/buffer/out/TextColor.cpp
View file

@ -158,9 +158,9 @@ COLORREF TextColor::GetColor(gsl::span<const COLORREF> colorTable,
// If we find a match, return instead the bright version of this color
for (size_t i = 0; i < 8; i++)
{
if (gsl::at(colorTable, i) == defaultColor)
if (til::at(colorTable, i) == defaultColor)
{
return gsl::at(colorTable, i + 8);
return til::at(colorTable, i + 8);
}
}
}
@ -173,11 +173,11 @@ COLORREF TextColor::GetColor(gsl::span<const COLORREF> colorTable,
}
else if (IsIndex16() && brighten)
{
return gsl::at(colorTable, _index | 8);
return til::at(colorTable, _index | 8);
}
else
{
return gsl::at(colorTable, _index);
return til::at(colorTable, _index);
}
}

12
src/host/alias.cpp
View file

@ -149,7 +149,7 @@ std::unordered_map<std::wstring,
if (target.has_value() && target->size() > 0)
{
gsl::at(*target, 0) = UNICODE_NULL;
til::at(*target, 0) = UNICODE_NULL;
}
std::wstring exeNameString(exeName);
@ -211,7 +211,7 @@ std::unordered_map<std::wstring,
{
if (target.size() > 0)
{
gsl::at(target, 0) = ANSI_NULL;
til::at(target, 0) = ANSI_NULL;
}
LockConsole();
@ -451,7 +451,7 @@ void Alias::s_ClearCmdExeAliases()
if (aliasBuffer.has_value() && aliasBuffer->size() > 0)
{
gsl::at(*aliasBuffer, 0) = UNICODE_NULL;
til::at(*aliasBuffer, 0) = UNICODE_NULL;
}
std::wstring exeNameString(exeName);
@ -543,7 +543,7 @@ void Alias::s_ClearCmdExeAliases()
{
if (alias.size() > 0)
{
gsl::at(alias, 0) = '\0';
til::at(alias, 0) = '\0';
}
LockConsole();
@ -698,7 +698,7 @@ void Alias::s_ClearCmdExeAliases()
writtenOrNeeded = 0;
if (aliasExesBuffer.has_value() && aliasExesBuffer->size() > 0)
{
gsl::at(*aliasExesBuffer, 0) = UNICODE_NULL;
til::at(*aliasExesBuffer, 0) = UNICODE_NULL;
}
LPWSTR AliasExesBufferPtrW = aliasExesBuffer.has_value() ? aliasExesBuffer->data() : nullptr;
@ -761,7 +761,7 @@ void Alias::s_ClearCmdExeAliases()
{
if (aliasExes.size() > 0)
{
gsl::at(aliasExes, 0) = '\0';
til::at(aliasExes, 0) = '\0';
}
LockConsole();

4
src/host/dbcs.cpp
View file

@ -67,10 +67,10 @@ DWORD UnicodeRasterFontCellMungeOnRead(const gsl::span<CHAR_INFO> buffer)
for (DWORD iSrc = 0; iSrc < buffer.size(); iSrc++)
{
// If it's not a trailing byte, copy it straight over, stripping out the Leading/Trailing flags from the attributes field.
auto& src{ gsl::at(buffer, iSrc) };
auto& src{ til::at(buffer, iSrc) };
if (!WI_IsFlagSet(src.Attributes, COMMON_LVB_TRAILING_BYTE))
{
auto& dst{ gsl::at(buffer, iDst) };
auto& dst{ til::at(buffer, iDst) };
dst = src;
WI_ClearAllFlags(dst.Attributes, COMMON_LVB_SBCSDBCS);
iDst++;

6
src/host/getset.cpp
View file

@ -1604,7 +1604,7 @@ void DoSrvPrivateRefreshWindow(_In_ const SCREEN_INFORMATION& screenInfo)
if (title.has_value() && title->size() > 0)
{
gsl::at(*title, 0) = ANSI_NULL;
til::at(*title, 0) = ANSI_NULL;
}
// Get the appropriate title and length depending on the mode.
@ -1667,7 +1667,7 @@ void DoSrvPrivateRefreshWindow(_In_ const SCREEN_INFORMATION& screenInfo)
if (title.size() > 0)
{
gsl::at(title, 0) = ANSI_NULL;
til::at(title, 0) = ANSI_NULL;
}
// Figure out how big our temporary Unicode buffer must be to get the title.
@ -1722,7 +1722,7 @@ void DoSrvPrivateRefreshWindow(_In_ const SCREEN_INFORMATION& screenInfo)
// If we didn't copy anything back and there is space, null terminate the given buffer and return.
if (title.size() > 0)
{
gsl::at(title, 0) = ANSI_NULL;
til::at(title, 0) = ANSI_NULL;
written = 1;
}
}

4
src/host/history.cpp
View file

@ -782,7 +782,7 @@ HRESULT GetConsoleCommandHistoryWImplHelper(const std::wstring_view exeName,
writtenOrNeeded = 0;
if (historyBuffer.size() > 0)
{
gsl::at(historyBuffer, 0) = UNICODE_NULL;
til::at(historyBuffer, 0) = UNICODE_NULL;
}
CommandHistory* const CommandHistory = CommandHistory::s_FindByExe(exeName);
@ -859,7 +859,7 @@ HRESULT ApiRoutines::GetConsoleCommandHistoryAImpl(const std::string_view exeNam
{
if (commandHistory.size() > 0)
{
gsl::at(commandHistory, 0) = ANSI_NULL;
til::at(commandHistory, 0) = ANSI_NULL;
}
LockConsole();

37
src/inc/til/at.h
View file

@ -5,17 +5,52 @@
namespace til
{
namespace details
{
// This was lifted from gsl::details::is_span.
template<class T>
struct is_span_oracle : std::false_type
{
};
#ifdef GSL_SPAN_H
template<class ElementType, std::size_t Extent>
struct is_span_oracle<gsl::span<ElementType, Extent>> : std::true_type
{
};
#endif
template<class T>
struct is_span : public is_span_oracle<std::remove_cv_t<T>>
{
};
}
// The at function declares that you've already sufficiently checked that your array access
// is in range before retrieving an item inside it at an offset.
// This is to save double/triple/quadruple testing in circumstances where you are already
// pivoting on the length of a set and now want to pull elements out of it by offset
// without checking again.
// gsl::at will do the check again. As will .at(). And using [] will have a warning in audit.
template<class T>
// This template is explicitly disabled if T is of type gsl::span, as it would interfere with
// the overload below.
template<class T, std::enable_if_t<!details::is_span<T>::value, int> = 0>
constexpr auto at(T& cont, const size_t i) -> decltype(cont[cont.size()])
{
#pragma warning(suppress : 26482) // Suppress bounds.2 check for indexing with constant expressions
#pragma warning(suppress : 26446) // Suppress bounds.4 check for subscript operator.
return cont[i];
}
#ifdef GSL_SPAN_H
// This is an overload of til::at for span that access its backing buffer directly (UNCHECKED)
template<typename ElementType, size_t Extent>
constexpr auto at(gsl::span<ElementType, Extent> span, const std::ptrdiff_t i) -> decltype(span[span.size()])
{
#pragma warning(suppress : 26481) // Suppress bounds.1 check for doing pointer arithmetic
#pragma warning(suppress : 26482) // Suppress bounds.2 check for indexing with constant expressions
#pragma warning(suppress : 26446) // Suppress bounds.4 check for subscript operator.
return span.data()[i];
}
#endif
}

2
src/renderer/gdi/paint.cpp
View file

@ -316,7 +316,7 @@ using namespace Microsoft::Console::Render;
// Convert data from clusters into the text array and the widths array.
for (size_t i = 0; i < cchLine; i++)
{
const auto& cluster = gsl::at(clusters, i);
const auto& cluster = til::at(clusters, i);
// Our GDI renderer hasn't and isn't going to handle things above U+FFFF or sequences.
// So replace anything complicated with a replacement character for drawing purposes.

18
src/terminal/parser/InputStateMachineEngine.cpp
View file

@ -395,7 +395,7 @@ bool InputStateMachineEngine::ActionCsiDispatch(const wchar_t wch,
// Handle intermediate characters, if any
if (!intermediates.empty())
{
switch (static_cast<CsiIntermediateCodes>(gsl::at(intermediates, 0)))
switch (static_cast<CsiIntermediateCodes>(til::at(intermediates, 0)))
{
case CsiIntermediateCodes::MOUSE_SGR:
{
@ -805,7 +805,7 @@ DWORD InputStateMachineEngine::_GetCursorKeysModifierState(const gsl::span<const
DWORD modifiers = 0;
if (_IsModified(parameters.size()) && parameters.size() >= 2)
{
modifiers = _GetModifier(gsl::at(parameters, 1));
modifiers = _GetModifier(til::at(parameters, 1));
}
// Enhanced Keys (from https://docs.microsoft.com/en-us/windows/console/key-event-record-str):
@ -834,7 +834,7 @@ DWORD InputStateMachineEngine::_GetGenericKeysModifierState(const gsl::span<cons
DWORD modifiers = 0;
if (_IsModified(parameters.size()) && parameters.size() >= 2)
{
modifiers = _GetModifier(gsl::at(parameters, 1));
modifiers = _GetModifier(til::at(parameters, 1));
}
// Enhanced Keys (from https://docs.microsoft.com/en-us/windows/console/key-event-record-str):
@ -1347,22 +1347,22 @@ bool InputStateMachineEngine::_GenerateWin32Key(const gsl::span<const size_t> pa
switch (parameters.size())
{
case 6:
key.SetRepeatCount(::base::saturated_cast<WORD>(gsl::at(parameters, 5)));
key.SetRepeatCount(::base::saturated_cast<WORD>(til::at(parameters, 5)));
[[fallthrough]];
case 5:
key.SetActiveModifierKeys(::base::saturated_cast<DWORD>(gsl::at(parameters, 4)));
key.SetActiveModifierKeys(::base::saturated_cast<DWORD>(til::at(parameters, 4)));
[[fallthrough]];
case 4:
key.SetKeyDown(static_cast<bool>(gsl::at(parameters, 3)));
key.SetKeyDown(static_cast<bool>(til::at(parameters, 3)));
[[fallthrough]];
case 3:
key.SetCharData(static_cast<wchar_t>(gsl::at(parameters, 2)));
key.SetCharData(static_cast<wchar_t>(til::at(parameters, 2)));
[[fallthrough]];
case 2:
key.SetVirtualScanCode(::base::saturated_cast<WORD>(gsl::at(parameters, 1)));
key.SetVirtualScanCode(::base::saturated_cast<WORD>(til::at(parameters, 1)));
[[fallthrough]];
case 1:
key.SetVirtualKeyCode(::base::saturated_cast<WORD>(gsl::at(parameters, 0)));
key.SetVirtualKeyCode(::base::saturated_cast<WORD>(til::at(parameters, 0)));
break;
}

6
src/terminal/parser/OutputStateMachineEngine.cpp
View file

@ -403,9 +403,9 @@ bool OutputStateMachineEngine::_IntermediateScsDispatch(const wchar_t wch,
// If we have more than one intermediate, the second intermediate forms part of
// the charset identifier. Otherwise it's identified by just the final character.
const auto charset = intermediates.size() > 1 ? std::make_pair(gsl::at(intermediates, 1), wch) : std::make_pair(wch, L'\0');
const auto charset = intermediates.size() > 1 ? std::make_pair(til::at(intermediates, 1), wch) : std::make_pair(wch, L'\0');
switch (gsl::at(intermediates, 0))
switch (til::at(intermediates, 0))
{
case L'(':
success = _dispatch->Designate94Charset(0, charset);
@ -1628,7 +1628,7 @@ bool OutputStateMachineEngine::s_ParseColorSpec(const std::wstring_view string,
for (size_t component = 0; component < 3; component++)
{
bool foundColor = false;
auto& value = gsl::at(colorValues, component);
auto& value = til::at(colorValues, component);
for (size_t i = 0; i < 3; i++)
{
const wchar_t wch = *curr++;

2
src/terminal/parser/ut_parser/InputEngineTest.cpp
View file

@ -369,7 +369,7 @@ bool TestInteractDispatch::WindowManipulation(const DispatchTypes::WindowManipul
for (size_t i = 0; i < parameters.size(); i++)
{
unsigned short actual;
VERIFY_SUCCEEDED(SizeTToUShort(gsl::at(parameters, i), &actual));
VERIFY_SUCCEEDED(SizeTToUShort(til::at(parameters, i), &actual));
VERIFY_ARE_EQUAL(_testState->_expectedParams[i], actual);
}
return true;

6
src/terminal/parser/ut_parser/OutputEngineTest.cpp
View file

@ -1557,14 +1557,14 @@ class StateMachineExternalTest final
if (i < expectedOptions.size())
{
expectedOption = gsl::at(expectedOptions, i);
expectedOption = til::at(expectedOptions, i);
}
optionsValid = expectedOption == gsl::at(dispatch._options, i);
optionsValid = expectedOption == til::at(dispatch._options, i);
if (!optionsValid)
{
Log::Comment(NoThrowString().Format(L"Graphics option match failed, index [%zu]. Expected: '%d' Actual: '%d'", i, expectedOption, gsl::at(dispatch._options, i)));
Log::Comment(NoThrowString().Format(L"Graphics option match failed, index [%zu]. Expected: '%d' Actual: '%d'", i, expectedOption, til::at(dispatch._options, i)));
break;
}
}

8
src/types/utils.cpp
View file

@ -425,10 +425,10 @@ void Utils::InitializeCampbellColorTableForConhost(const gsl::span<COLORREF> tab
void Utils::SwapANSIColorOrderForConhost(const gsl::span<COLORREF> table)
{
THROW_HR_IF(E_INVALIDARG, table.size() < 16);
std::swap(gsl::at(table, 1), gsl::at(table, 4));
std::swap(gsl::at(table, 3), gsl::at(table, 6));
std::swap(gsl::at(table, 9), gsl::at(table, 12));
std::swap(gsl::at(table, 11), gsl::at(table, 14));
std::swap(til::at(table, 1), til::at(table, 4));
std::swap(til::at(table, 3), til::at(table, 6));
std::swap(til::at(table, 9), til::at(table, 12));
std::swap(til::at(table, 11), til::at(table, 14));
}
// Function Description: