Skip to content

Commit

Permalink
Replace gsl::at with a new til::at(span) for pre-checked bounds (#6925)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@DHowett
DHowett authored Jul 15, 2020
1 parent 80da24e commit 09471c3
Show file tree
Hide file tree
Showing 14 changed files with 81 additions and 46 deletions.
2 changes: 1 addition & 1 deletion src/buffer/out/AttrRow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 6 additions & 6 deletions src/buffer/out/OutputCellIterator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down
8 changes: 4 additions & 4 deletions src/buffer/out/TextColor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand All @@ -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);
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/host/alias.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -543,7 +543,7 @@ void Alias::s_ClearCmdExeAliases()
{
if (alias.size() > 0)
{
gsl::at(alias, 0) = '\0';
til::at(alias, 0) = '\0';
}

LockConsole();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -761,7 +761,7 @@ void Alias::s_ClearCmdExeAliases()
{
if (aliasExes.size() > 0)
{
gsl::at(aliasExes, 0) = '\0';
til::at(aliasExes, 0) = '\0';
}

LockConsole();
Expand Down
4 changes: 2 additions & 2 deletions src/host/dbcs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand Down
6 changes: 3 additions & 3 deletions src/host/getset.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/host/history.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
37 changes: 36 additions & 1 deletion src/inc/til/at.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 changes: 1 addition & 1 deletion src/renderer/gdi/paint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
18 changes: 9 additions & 9 deletions src/terminal/parser/InputStateMachineEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
{
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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;
}

Expand Down
6 changes: 3 additions & 3 deletions src/terminal/parser/OutputStateMachineEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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++;
Expand Down
2 changes: 1 addition & 1 deletion src/terminal/parser/ut_parser/InputEngineTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions src/terminal/parser/ut_parser/OutputEngineTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/types/utils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit 09471c3

Please sign in to comment.