Мы используем куки, чтобы пользоваться сайтом
было удобно.
Вебинар: C++ и неопределённое поведение - 27.02
Этой статьёй мы начинаем трилогию об игровом движке Nau Engine. В первой части мы сосредоточимся на его функциональности, уделяя особое внимание трём ключевым блокам ошибок: проблемам с памятью, копипасте и логическим ошибкам.
Nau Engine — это игровой движок, разработанный для упрощения процесса создания и поддержки игр. Он основан на трёх принципах: универсальность, доступность и кроссплатформенность.
1. Универсальность: Nau Engine предлагает инструменты для всех этапов создания игры — от разработки до пострелизной поддержки. Это включает системы для аналитики, обработки ошибок и обновления контента.
2. Доступность: движок нацелен на разработчиков с разным уровнем опыта. Он предоставляет интуитивно понятные инструменты и шаблоны, что помогает новичкам легко начать, а более опытным пользователям — углубляться в детали.
3. Кроссплатформенность: Nau Engine позволяет адаптировать игры для различных платформ, таких как ПК, мобильные устройства и веб, что делает разработку более гибкой и удобной.
Особое внимание стоит уделить адаптированной ECS (Entity Component System) библиотеке от движка Dagor, которая интегрирована в Nau Engine. Эта библиотека не только обеспечивает высокую производительность, но и навевает приятные воспоминания о таких культовых играх, как:
В целом Nau Engine стремится помогать разработчикам на всех этапах создания игр, обеспечивая необходимыми инструментами и возможностями для реализации креативных идей.
Следует отметить, что для анализа использовалось состояние репозитория на момент коммита 020cbfe.
Фрагмент N1
NAU_ASSERT(irRequest.usage.access == req.usage.access
&& irRequest.usage.type == irRequest.usage.type);Предупреждение PVS-Studio: V501 There are identical sub-expressions 'irRequest.usage.type' to the left and to the right of the '==' operator. irGraphBuilder.cpp 719
В этом фрагменте нас интересует макрос NAU_ASSERT, который служит для проверки условий во время выполнения кода. Если быть точнее, то интересно нам второе сравнение внутри этого макроса, а именно: irRequest.usage.type == irRequest.usage.type. При отдельном написании сразу становится видно, что в левой и правой части сравнения используется одно и то же выражение, а следовательно, условие всегда будет истинным.
Вероятно, автор кода хотел сравнить irRequest.usage.type с req.usage.type, чтобы проверить, совпадают ли типы в двух разных объектах:
NAU_ASSERT(irRequest.usage.access == req.usage.access
&& irRequest.usage.type == req.usage.type);Фрагмент N2
bool VFXModFXInstance::deserialize(const nau::DataBlock* blk)
{
....
m_life.part_life_min = blk->getReal("lifeMin", 5.0f);
m_life.part_life_max = blk->getReal("lifeMin", 5.0f);
....
}Предупреждение PVS-Studio: V656 Variables 'm_life.part_life_min', 'm_life.part_life_max' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'blk->getReal("lifeMin", 5.0f)' expression. Check lines: 94, 95. vfx_mod_fx_instance.cpp 95
В функции VFXModFXInstance::deserialize наблюдается проблема, связанная с инициализацией переменных m_life.part_life_min и m_life.part_life_max. Из-за копипасты обе переменные получают свои значения через вызов одной и той же функции с использованием одного и того же ключа "lifeMin". Очевидно, что для минимального и максимального времени жизни частиц должны использоваться разные значения. Поэтому можем предположить, что во втором случае вместо "lifeMin" должен использоваться ключ "lifeMax":
m_life.part_life_min = blk->getReal("lifeMin", 5.0f);
m_life.part_life_max = blk->getReal("lifeMax", 5.0f);Фрагмент N3
Material* Material::clone() const
{
auto material = new (std::nothrow) Material();
if (material)
{
....
material->_textureSlots = material->_textureSlots;
material->_textureSlotIndex = material->_textureSlotIndex;
....
}
....
}Предупреждения PVS-Studio:
V570 The 'material->_textureSlots' variable is assigned to itself. CCMaterial.cpp 527
V570 The 'material->_textureSlotIndex' variable is assigned to itself. CCMaterial.cpp 528
Здесь происходит присваивание переменных _textureSlots и _textureSlotIndex самим себе, что не имеет смысла и может быть ошибкой. Судя по названию функции-члена класса (clone), нужно скопировать состояния объекта в новый. Исходя из этого, исправление может быть таким:
material->_textureSlots = _textureSlots;
material->_textureSlotIndex = _textureSlotIndex;Фрагмент N4
template <typename T>
requires(std::is_enum_v<T>)
class TypedFlag
{
public:
using EnumType = T;
using ValueType = std::underlying_type_t<T>;
....
private:
ValueType m_value = 0;
....
friend TypedFlag<T> operator|(TypedFlag<T> value, TypedFlag<T> flags)
{
}
....
};Предупреждение PVS-Studio: V591 Non-void function should return a value. typed_flag.h 145
В перегрузке оператора | для шаблона класса TypedFlag отсутствует возвращаемое значение, что может привести к ошибкам в логике работы программы. Особенно интересна эта ситуация тем, что для оператора |= логика реализована:
friend TypedFlag<T>& operator|=(TypedFlag<T>& value, T flag)
{
return value.set(flag);
}Есть несколько способов исправить эту проблему.
Способ N1. Реализовать функциональность операторов. Они должны возвращать результат, который соответствует логике объединения флагов. Например:
return TypedFlag<T> { value }.set(flags);Способ N2. Удалить оператор. Если функционал оператора | не нужен, его можно удалить, чтобы избежать путаницы или, например, можно объявить оператор как удалённый:
friend TypedFlag<T> operator|(TypedFlag<T> value, TypedFlag<T> flags) = delete;Таким образом, вы можете гарантировать, что код будет более предсказуемым и понятным для других разработчиков.
И вот ещё случаи:
Фрагмент N5
EntityId EntityManager::createEntitySync(....)
{
....
if (EASTL_UNLIKELY(result != RequestResources::Loaded))
{
#if NAU_DEBUG
if (result == RequestResources::Loaded)
{
....
}
#endif
if (result == RequestResources::Error)
{
....
}
}
....
}Предупреждение PVS-Studio: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 711, 714. entityManager2.cpp 711
В коде присутствует логическая ошибка, которая может ввести в заблуждение. Рассмотрим детали:
if (EASTL_UNLIKELY(result != RequestResources::Loaded)): оно проверяет, что результат не равен RequestResources::Loaded. Если это условие истинно, значит, ресурсы не загружены.#if NAU_DEBUG: внутри него есть проверка if (result == RequestResources::Loaded), которая никогда не будет истинной, так как мы уже проверили, что result не равен RequestResources::Loaded. Это создаёт противоречие и может ввести в заблуждение.Фрагмент N6
IGenSave* create_async_writer(....)
{
AsyncWriterCB* ret = new AsyncWriterCB(buf_size);
if (!ret->open(fname, mode))
{
if (ret)
{
delete ret;
ret = nullptr;
}
}
return ret;
}Предупреждение PVS-Studio: V668 There is no sense in testing the 'ret' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. asyncWrite.cpp 363
В этом коде происходит проверка указателя ret на nullptr после вызова new. Это может быть источником ошибок, поскольку указатель ret всегда будет указывать на выделенную память, если выделение прошло успешно. В случае если память не удастся выделить, будет выброшено исключение, и код ниже не будет выполнен.
Кроме того, указатель ret разыменовывается до проверки, что делает саму проверку if (ret) ещё более бессмысленной. Если бы ret действительно был равен nullptr, программа бы уже аварийно завершилась при попытке разыменования.
И вот ещё случаи:
Фрагмент N7
void Properties::skipWhiteSpace()
{
signed char c;
do
{
c = readChar();
} while (isspace(c) && c != EOF);
....
if (c != EOF)
{
....
}
}Предупреждения PVS-Studio:
V739 EOF should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type. CCProperties.cpp 464
V739 EOF should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type. CCProperties.cpp 468
Функция-член Properties::skipWhiteSpace использует переменную типа signed char для хранения символа, считанного с помощью функции Properties::readChar. Последняя, судя по комментарию, симулирует поведение std::getchar:
//
// Stream simulation
//
signed char Properties::readChar()
{
if (eof())
return EOF;
return _data->_bytes[(*_dataIdx)++];
}Код содержит две ошибки:
Ошибка N1. Функция std::getchar неспроста возвращает значения типа int. Функция возвращает символ (1 байт) или EOF в результате ошибки. EOF — константа, имеющая негативное значение, обычно -1. Поэтому перед тем как преобразовывать результат std::getchar к символу, нужно отсечь вариант с EOF:
if (int res = std::getchar(); res != EOF)
{
char ch = res;
// your logic with character
}Что может произойти, если не обработать такую ситуацию? Пользователи, использующие Extended ASCII Codes, иногда сталкиваются с ошибкой, когда один из символов их алфавита некорректно обрабатывается программами. Например, последняя буква русского алфавита в кодировке Windows-1251 как раз имеет код 0xFF и воспринимается некоторыми программами как конец файла.
Корректная имплементация функции Properties::readChar должна выглядеть так:
//
// Stream simulation
//
int Properties::readChar()
{
if (eof())
return EOF;
return _data->_bytes[(*_dataIdx)++];
}Ошибка N2. Функция Properties::skipWhiteSpace после исправления Properties::readChar также должна работать с типом int до тех пор, пока не отсечёт вариант с EOF:
void Properties::skipWhiteSpace()
{
int c;
do
{
c = readChar();
}
while (c != EOF && isspace(c));
....
if (c != EOF)
{
// Now we can cast 'c' to 'signed char'
signed char ch = c;
....
}
}Фрагмент N8
void Sweep::EdgeEvent(....)
{
....
if (o1 == COLLINEAR) {
if( triangle->Contains(&eq, p1)) {
....
} else {
std::runtime_error("EdgeEvent - collinear points not supported");
assert(0);
}
return;
}
....
if (o2 == COLLINEAR) {
if (triangle->Contains(&eq, p2)){
....
} else {
std::runtime_error("EdgeEvent - collinear points not supported");
assert(0);
}
return;
}
....
}Предупреждения PVS-Studio:
V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); sweep.cc 123
V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); sweep.cc 140
В функции-члене Sweep::EdgeEvent создаются объекты исключений std::runtime_error при обнаружении коллинеарных точек, но они не выбрасываются. Это приводит к тому, что программа может продолжать выполнение, игнорируя возникшую ошибку, что может вызвать некорректное поведение.
Исправленный код:
throw std::runtime_error("EdgeEvent - collinear points not supported");Фрагмент N9
std::size_t UniformLocation::operator()(const UniformLocation &uniform) const
{
return (((size_t) shaderStage) & 0xF)
|((size_t)(location[0] << 4))
|((size_t)(location[1] << 8));
}Предупреждения PVS-Studio:
V1028 Possible overflow. Consider casting operands of the 'location[0] << 4' operator to the 'size_t' type, not the result. Types.cpp 40
V1028 Possible overflow. Consider casting operands of the 'location[1] << 8' operator to the 'size_t' type, not the result. Types.cpp 40
Оператор UniformLocation::operator() выполняет битовые операции над значениями, полученными из переменных shaderStage и location, чтобы вернуть уникальный идентификатор.
Проблема заключается в том, что операции сдвига (<<) могут привести к переполнению, если значения location[0] или location[1] слишком велики. Следует привести location[0] и location[1] к типу size_t перед выполнением операции сдвига:
std::size_t UniformLocation::operator()(const UniformLocation &uniform) const
{
return (((size_t) shaderStage) & 0xF)
|((size_t)(location[0]) << 4)
|((size_t)(location[1]) << 8);
}Фрагмент N10
char** m_memBlock = nullptr;
template<class Type>
class ThreadLocalValue final
{
....
private:
void resizeLines(size_t sizeReq)
{
....
auto newSize = sizeReq + 1;
....
m_memBlock = static_cast<char**>(realloc(m_memBlock,
sizeof(char*) * newSize));
....
}
....
};Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_memBlock' is lost. Consider assigning realloc() to a temporary pointer. thread_local_value.h 235
В данном коде m_memBlock представляет собой массив указателей на char, а метод resizeLines изменяет его размер с помощью realloc.
Если realloc успешно выделяет память, он может вернуть либо тот же указатель, либо новый, переместив данные. Однако, если выделение памяти не удаётся, realloc возвращает nullptr, а прежнее значение m_memBlock теряется.
Чтобы избежать утечки, необходимо сначала присвоить результат realloc временной переменной, проверить его на nullptr и только затем обновлять m_memBlock.
И вот ещё случаи:
Фрагмент N11
IAssetContainerLoader* loader = nullptr;
const auto importSettingsProviders = ....;
RuntimeReadonlyDictionary::Ptr importSettings;
for (const auto& importSettingsProvider : importSettingsProviders)
{
if (importSettings =
importSettingsProvider->getAssetImportSettings(containerPath,
*loader);
importSettings)
{
break;
}
}Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'loader' might take place. asset_file_content_provider.cpp 34
В начале кода указатель loader инициализируется значением nullptr. Затем в цикле вызывается функция-член getAssetImportSettings, при этом аргументом передаётся разыменованный указатель loader. Странно, что между этими строками он никаким образом не модифицируется. Получаем гарантированное неопределённое поведение.
И вот ещё случай:
Фрагмент N12
struct DelayedEntityCreationChunk
{
....
DelayedEntityCreationChunk(uint16_t cap) : capacity(cap)
{
queue = (DelayedEntityCreation*)
malloc(capacity * sizeof(DelayedEntityCreation));
}
....
};Предупреждение PVS-Studio: V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. entityManager.h 1391
В этом фрагменте анализатор ругается на использование malloc для выделения памяти под массив объектов классов с конструкторами и деструкторами. В общем случае так делать не стоит, лучше применять другие способы создания объектов на куче. Однако в данной ситуации это ложное срабатывание, так как создание объектов происходит позже через метод emplace_back (примерно то же самое происходит и в векторе).
Тут у читателя может возникнуть вопрос: "А зачем тогда вообще включать этот фрагмент в статью?". Я собирался убрать его, но тут моё внимание привлекло ещё кое-что в этом классе:
struct DelayedEntityCreationChunk
{
....
DelayedEntityCreation* queue = nullptr;
uint16_t readFrom = 0, writeTo = 0, capacity;
DelayedEntityCreationChunk(uint16_t cap) :
capacity(cap)
{
queue = (DelayedEntityCreation*)
malloc(capacity * sizeof(DelayedEntityCreation));
}
~DelayedEntityCreationChunk()
{
for(auto i = begin(), e = end(); i != e; ++i)
i->~DelayedEntityCreation();
free(queue);
}
DelayedEntityCreationChunk(const DelayedEntityCreationChunk&) = delete;
DelayedEntityCreationChunk&
operator=(const DelayedEntityCreationChunk&) = delete;
DelayedEntityCreationChunk(DelayedEntityCreationChunk&& a)
{
memcpy(this, &a, sizeof(DelayedEntityCreationChunk));
memset(&a, 0, sizeof(DelayedEntityCreationChunk));
}
DelayedEntityCreationChunk& operator=(DelayedEntityCreationChunk&& a)
{
alignas(DelayedEntityCreationChunk)
char buf[sizeof(DelayedEntityCreationChunk)];
memcpy(buf, this, sizeof(DelayedEntityCreationChunk));
memcpy(this, &a, sizeof(DelayedEntityCreationChunk));
memcpy(&a, buf, sizeof(DelayedEntityCreationChunk));
return *this;
}
....
template <typename... Args>
bool emplace_back(EntityId eid, Args &&...args)
{
DAECS_EXT_ASSERT(!full());
new (queue + (writeTo++)) DelayedEntityCreation(eid,
eastl::forward<Args>(args)...);
return full();
}
....
};Какими свойствами обладает этот класс:
noexcept, что в некоторых ситуациях не есть хорошо.При этом можно заметить, что при работе с объектами этого класса в конструкторе и операторе перемещения активно используются memcpy и memset. Стандарт чётко регламентирует их поведение только для тривиально копируемых типов, в ином случае оно может быть не определено.
Рекомендую переделать код на примерно следующий:
struct DelayedEntityCreationChunk
{
....
DelayedEntityCreation* queue = nullptr;
uint16_t readFrom = 0, writeTo = 0, capacity;
....
public:
DelayedEntityCreationChunk(DelayedEntityCreationChunk &&a) noexcept
: queue { std::exchange(a.queue, {}) }
, readFrom { std::exchange(a.readFrom, {}) }
, writeTo { std::exchange(a.writeTo, {}) }
, capacity { std::exchange(a.capacity, {}) }
{
}
void swap(DelayedEntityCreationChunk &other) noexcept
{
auto lhs = std::tie(queue, readFrom, writeTo, capacity);
auto rhs = std::tie(other.queue, other.readFrom,
other.writeTo, other.capacity);
std::swap(lhs, rhs);
}
DelayedEntityCreationChunk&
operator=(DelayedEntityCreationChunk &&a) noexcept
{
if (this == std::addressof(a)) return *this;
auto tmp = std::move(a);
swap(tmp);
return *this;
}
....
};В первой части нашего исследования игрового движка Nau Engine мы погрузились в его функциональность, уделив внимание проблемам с памятью, копипасте и логическим ошибкам. Эти проблемы могут стать настоящими камнями преткновения для разработчиков, стремящихся создавать увлекательные игры. Хотя Nau Engine предлагает множество заманчивых возможностей, обнаруженные недостатки могут существенно повлиять на производительность и общее впечатление от игр.
В следующей части нашей трилогии мы будем искать пути оптимизации и улучшения работы движка.
Благодарю за внимание!
0
0
0
22
0
237
0
1098
0
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0