В настоящее время нет необходимости реализовывать физику объектов с нуля для разработки игр, поскольку для этого существует множество библиотек. Bullet активно использовался во многих ААА-играх, проектах виртуальной реальности, различных симуляторах и машинном обучении. И он до сих пор используется, являясь, например, одним из движков Red Dead Redemption и Red Dead Redemption 2. Так почему бы не проверить Bullet с PVS-Studio, чтобы увидеть, какие ошибки может обнаружить статический анализ в таком крупномасштабном проекте по моделированию физики.
Эта библиотека свободно распространяется, поэтому каждый может использовать ее в своих проектах по своему желанию. Помимо Red Dead Redemption, этот физический движок также используется в киноиндустрии для создания спецэффектов. Например, его использовали при съемках фильма Гая Ричи Шерлок Холмс для расчета столкновений.
Если вы впервые встречаете статью, где PVS-Studio проверяет проекты, сделаю небольшое отступление. PVS-Studio — статический анализатор кода, помогающий находить ошибки, дефекты и потенциальные уязвимости в исходном коде программ на языках C, C++, C#, Java. Статический анализ — это своего рода автоматизированный процесс проверки кода.
Разогрев
Пример 1:
Начнем с забавной ошибки:
V624 Вероятно, опечатка в константе 3.141592538. Рассмотрите возможность использования константы M_PI из ‹math.h›. PhysicsClientC_API.cpp 4109
B3_SHARED_API void b3ComputeProjectionMatrixFOV(float fov, ....)
{
float yScale = 1.0 / tan((3.141592538 / 180.0) * fov / 2);
....
}Небольшая опечатка в значении Пи (3,141592653…). В дробной части отсутствует 7-я цифра — она должна быть равна 6.
Возможно, ошибка в десятимиллионной дроби после запятой не приведет к каким-либо существенным последствиям, но все же следует использовать уже существующие библиотечные константы, в которых нет опечаток. Существует константа M_PI для числа Пи из заголовка math.h.
Копировать вставить
Пример 2:
Иногда анализатор позволяет найти ошибку косвенно. Например, здесь в функцию передаются три связанных аргумента halfExtentsX, halfExtentsY, halfExtentsZ, но последний нигде в функции не используется. Вы можете заметить, что переменная halfExtentsY используется дважды при вызове метода addVertex. Так что, возможно, это ошибка копирования и здесь следует использовать забытый аргумент.
V751 Параметр halfExtentsZ не используется внутри тела функции. TinyRenderer.cpp 375
void TinyRenderObjectData::createCube(float halfExtentsX,
float halfExtentsY,
float halfExtentsZ,
....)
{
....
m_model->addVertex(halfExtentsX * cube_vertices_textured[i * 9],
halfExtentsY * cube_vertices_textured[i * 9 + 1],
halfExtentsY * cube_vertices_textured[i * 9 + 2],
cube_vertices_textured[i * 9 + 4],
....);
....
}Пример 3:
Анализатор также обнаружил следующий интересный фрагмент, который я покажу сначала в исходном виде.
Видите эту длиннющую очередь?
Очень странно, что программист решил написать такое длинное условие в одну строку. Но неудивительно, что в него, скорее всего, проскочила ошибка.
На этой строке анализатор выдал следующие предупреждения.
V501 Слева и справа от оператора && одинаковые подвыражения rotmat.Column1().Norm() ‹ 1.0001. LinearR4.cpp 351
V501 Слева и справа от оператора && одинаковые подвыражения 0.9999 ‹ rotmat.Column1().Norm(). LinearR4.cpp 351
Если записать все это в понятном «табличном» виде, то можно увидеть, что все те же проверки применяются к Столбцу1. Последние два сравнения показывают, что есть Столбец1 и Столбец2. Скорее всего, третье и четвертое сравнения должны были проверить значение Column2.
Column1().Norm() < 1.0001 && 0.9999 < Column1().Norm()
&& Column1().Norm() < 1.0001 && 0.9999 < Column1().Norm()
&&(Column1() ^ Column2()) < 0.001 && (Column1() ^ Column2()) > -0.001
In this form, the same comparisons become much more noticeable.Пример 4:
Ошибка того же рода:
V501 Слева и справа от оператора && одинаковые подвыражения cs.m_fJacCoeffInv[0] == 0. b3CpuRigidBodyPipeline.cpp 169
float m_fJacCoeffInv[2];
static inline void b3SolveFriction(b3ContactConstraint4& cs, ....)
{
if (cs.m_fJacCoeffInv[0] == 0 && cs.m_fJacCoeffInv[0] == 0)
{
return;
}
....
}В этом случае один и тот же элемент массива проверяется дважды. Скорее всего, условие должно было выглядеть так: cs.m_fJacCoeffInv[0] == 0 && cs.m_fJacCoeffInv[1] == 0. Это классический пример ошибки копирования-вставки.
Пример 5:
Так же обнаружился такой дефект:
V517 Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверить строки: 79, 112. main.cpp 79
int main(int argc, char* argv[])
{
....
while (serviceResult > 0)
{
serviceResult = enet_host_service(client, &event, 0);
if (serviceResult > 0)
{
....
}
else if (serviceResult > 0)
{
puts("Error with servicing the client");
exit(EXIT_FAILURE);
}
....
}
....
}Функция enet_host_service, результат которой присваивается serviceResult, возвращает 1 в случае успешного завершения и -1 в случае неудачи. Скорее всего, ветка else if должна была отреагировать на отрицательное значение serviceResult, но условие проверки дублировалось. Вероятно, это также ошибка копирования-вставки.
Аналогичное предупреждение анализатора есть, но рассматривать его более подробно в рамках данной статьи нет смысла.
V517 Обнаружено использование шаблона if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверить строки: 151, 190. PhysicsClientUDP.cpp 151
Over the top: превышение границ массива
Пример 6:
Одна из неприятных ошибок при поиске — переполнение массива. Эта ошибка часто возникает из-за сложной индексации в цикле.
Здесь, в условиях цикла, верхний предел переменной dofIndex равен 128, а степень свободы — 4 включительно. Но m_desiredState также содержит только 128 элементов. В результате индекс [dofIndex+dof] может привести к переполнению массива.
V557 Возможен переполнение массива. Значение индекса dofIndex + dof может достигать 130. PhysicsClientC_API.cpp 968
#define MAX_DEGREE_OF_FREEDOM 128 double m_desiredState[MAX_DEGREE_OF_FREEDOM];B3_SHARED_API int b3JointControl(int dofIndex, double* forces, int dofCount, ....) { .... if ( (dofIndex >= 0) && (dofIndex < MAX_DEGREE_OF_FREEDOM ) && dofCount >= 0 && dofCount <= 4) { for (int dof = 0; dof < dofCount; dof++) { command->m_sendState.m_desiredState[dofIndex+dof] = forces[dof]; .... } } .... }
Пример 7:
Аналогичная ошибка, но теперь она вызвана суммированием не при индексации массива, а в условии. Если файл имеет имя максимальной длины, терминальный ноль будет записан вне массива (Off-by-one Error). Конечно, переменная len будет равна MAX_FILENAME_LENGTH только в исключительных случаях, но это не устраняет ошибку, а просто делает ее редкой.
V557 Возможен переполнение массива. Значение индекса len может достигать 1024. PhysicsClientC_API.cpp 5223
#define MAX_FILENAME_LENGTH MAX_URDF_FILENAME_LENGTH 1024 struct b3Profile { char m_name[MAX_FILENAME_LENGTH]; int m_durationInMicroSeconds; };int len = strlen(name); if (len >= 0 && len < (MAX_FILENAME_LENGTH + 1)) { command->m_type = CMD_PROFILE_TIMING; strcpy(command->m_profile.m_name, name); command->m_profile.m_name[len] = 0; }
Один раз отмерь, семь раз отрежь
Пример 8:
В тех случаях, когда вам нужно многократно использовать результат работы какой-либо функции или использовать переменную, для доступа к которой необходимо пройти всю цепочку вызовов, следует использовать временные переменные для оптимизации и лучшей читаемости кода. Анализатор нашел более 100 мест в коде, где можно сделать такое исправление.
V807 Снижение производительности. Рассмотрите возможность создания указателя, чтобы избежать повторного использования выражения m_app-›m_renderer-›getActiveCamera(). InverseKinematicsExample.cpp 315
virtual void resetCamera()
{
....
if (....)
{
m_app->m_renderer->getActiveCamera()->setCameraDistance(dist);
m_app->m_renderer->getActiveCamera()->setCameraPitch(pitch);
m_app->m_renderer->getActiveCamera()->setCameraYaw(yaw);
m_app->m_renderer->getActiveCamera()->setCameraPosition(....);
}
}Одна и та же цепочка вызовов используется здесь много раз и может быть заменена одним указателем.
Пример 9:
V810 Снижение производительности. Несколько раз вызывалась функция btCos(euler_out.pitch) с одинаковыми аргументами. Результат, возможно, следует сохранить во временную переменную, которую затем можно будет использовать при вызове функции btAtan2. btMatrix3x3.h 576
V810 Снижение производительности. Несколько раз вызывалась функция btCos(euler_out2.pitch) с одинаковыми аргументами. Результат, возможно, следует сохранить во временную переменную, которую затем можно будет использовать при вызове функции btAtan2. btMatrix3x3.h 578
void getEulerZYX(....) const { .... if (....) { .... } else { .... euler_out.roll = btAtan2(m_el[2].y() / btCos(euler_out.pitch), m_el[2].z() / btCos(euler_out.pitch)); euler_out2.roll = btAtan2(m_el[2].y() / btCos(euler_out2.pitch), m_el[2].z() / btCos(euler_out2.pitch)); euler_out.yaw = btAtan2(m_el[1].x() / btCos(euler_out.pitch), m_el[0].x() / btCos(euler_out.pitch)); euler_out2.yaw = btAtan2(m_el[1].x() / btCos(euler_out2.pitch), m_el[0].x() / btCos(euler_out2.pitch));} .... }
В этом случае вы можете создать две переменные и сохранить в них значения, возвращаемые функцией btCos для euler_out.pitch и euler_out2.pitch. четырехкратного вызова функции для каждого аргумента.
Утечка
Пример 10:
В проекте было обнаружено множество ошибок следующего рода:
V773 Выход из области видимости указателя импортер без освобождения памяти. Возможна утечка памяти. SerializeSetup.cpp 94
void SerializeSetup::initPhysics() { .... btBulletWorldImporter* importer = new btBulletWorldImporter(m_dynamicsWorld); .... fclose(file);m_guiHelper->autogenerateGraphicsObjects(m_dynamicsWorld); }
Память не была освобождена из указателя importer здесь. Это может привести к утечке памяти. А для физического движка это может быть плохой тенденцией. Чтобы избежать утечки, достаточно добавить delete importer после того, как переменная станет ненужной. Но, конечно, лучше использовать умные указатели.
C++ живет своим собственным кодом
Пример 11:
Следующая ошибка появляется в коде из-за того, что правила C++ не всегда совпадают с математическими правилами или «здравым смыслом». Вы заметите, где в этом маленьком фрагменте кода содержится ошибка?
btAlignedObjectArray<btFractureBody*> m_fractureBodies;void btFractureDynamicsWorld::fractureCallback() { for (int i = 0; i < numManifolds; i++) { .... int f0 = m_fractureBodies.findLinearSearch(....); int f1 = m_fractureBodies.findLinearSearch(....);if (f0 == f1 == m_fractureBodies.size()) continue; .... } .... }
Анализатор выдает следующее предупреждение:
V709 Обнаружено подозрительное сравнение: ‘f0 == f1 == m_fractureBodies.size()’. Помните, что a == b == c не равно a == b && b == c. btFractureDynamicsWorld.cpp 483
Может показаться, что условие проверяет, что f0 равно f1 и равно количеству элементов в m_fractureBodies. Кажется, это сравнение должно было проверять, находятся ли f0 и f1 в конце массива m_fractureBodies, так как они содержат найденную позицию объекта методом findLinearSearch(). Но на самом деле это выражение превращается в проверку на равенство f0 и f1 m_fractureBodies.size(), а затем в проверку на посмотрите, равен ли m_fractureBodies.size() результату f0 == f1. В результате третий операнд здесь сравнивается с 0 или 1.
Красивая ошибка! И, к счастью, довольно редко. Пока мы встречали его только в двух проектах с открытым исходным кодом, и интересно, что все они были игровыми движками.
Пример 12:
При работе со строками часто лучше использовать возможности класса string. Итак, для следующих двух случаев лучше заменить strlen(MyStr.c_str()) и val = “” на MyStr.length()иval .clear() соответственно.
V806 Снижение производительности. Выражение вида strlen(MyStr.c_str()) можно переписать как MyStr.length(). RobotLoggingUtil.cpp 213
FILE* createMinitaurLogFile(const char* fileName,
std::string& structTypes,
....)
{
FILE* f = fopen(fileName, "wb");
if (f)
{
....
fwrite(structTypes.c_str(), strlen(structTypes.c_str()), 1, f);
....
}
....
}V815 Снижение производительности. Рассмотрите возможность замены выражения val = «» на val.clear(). b3CommandLineArgs.h 40
void addArgs(int argc, char **argv)
{
....
std::string val;
....
val = "";
....
}Были и другие предупреждения, но я думаю, что мы можем остановиться здесь. Как видите, статический анализ кода позволяет обнаружить широкий спектр разнообразных ошибок.
Интересно читать про разовые проверки проекта, но это неправильный способ использования статических анализаторов кода. И об этом мы поговорим ниже.
Ошибки, обнаруженные до нас
Интересно было попробовать найти ошибки или дефекты, которые уже были исправлены, но которые смог обнаружить статический анализатор в свете недавней статьи Ошибки, которые не находит статический анализ кода, потому что он не используется.
Пул-реквестов в репозиторий было не много и многие из них связаны с внутренней логикой движка. Но были и ошибки, которые анализатор смог обнаружить.
Пример 13:
char m_deviceExtensions[B3_MAX_STRING_LENGTH];void b3OpenCLUtils_printDeviceInfo(cl_device_id device) { b3OpenCLDeviceInfo info; b3OpenCLUtils::getDeviceInfo(device, &info); .... if (info.m_deviceExtensions != 0) { .... } }
В комментарии к запросу сказано, что нужно было проверить массив на то, что он не пустой, а вместо этого была выполнена бессмысленная проверка указателя, которая всегда возвращала true. Вот что вам говорит предупреждение PVS-Studio об оригинальной проверке:
V600 Рассмотрим осмотр состояния. Указатель info.m_deviceExtensions всегда не равен NULL. b3OpenCLUtils.cpp 551
Пример 14:
Можете ли вы выяснить, в чем проблема со следующей функцией?
inline void Matrix4x4::SetIdentity()
{
m12 = m13 = m14 = m21 = m23 = m24 = m13 = m23 = m41 = m42 = m43 = 0.0;
m11 = m22 = m33 = m44 = 1.0;
}Анализатор выдает следующие предупреждения:
V570 Одно и то же значение дважды присваивается переменной m23. ЛинейныйR4.h 627
V570 Одно и то же значение дважды присваивается переменной m13. ЛинейныйR4.h 627
Повторные задания при такой форме записи трудно отследить невооруженным глазом и, как следствие, некоторые элементы матрицы не получили начального значения. Эта ошибка была исправлена табличной формой записи заданий:
m12 = m13 = m14 =
m21 = m23 = m24 =
m31 = m32 = m34 =
m41 = m42 = m43 = 0.0;Пример 15:
Следующая ошибка в одном из условий функции btSoftBody::addAeroForceToNode() привела к очевидной ошибке. Согласно комментарию в пулреквесте, силы прикладывались к объектам не с той стороны.
struct eAeroModel { enum _ { V_Point, V_TwoSided, .... END }; };void btSoftBody::addAeroForceToNode(....) { .... if (....) { if (btSoftBody::eAeroModel::V_TwoSided) { .... } .... } .... }
PVS-Studio также мог найти эту ошибку и выдать следующее предупреждение:
V768 Константа перечисления V_TwoSided используется как переменная логического типа. btSoftBody.cpp 542
Фиксированная проверка выглядит так:
if (m_cfg.aeromodel == btSoftBody::eAeroModel::V_TwoSided)
{
....
}Вместо эквивалентности свойства объекта одному из перечислителей проверялся сам перечислитель V_TwoSided.
Понятно, что я не смотрел все пулл-реквесты, потому что не в этом был смысл. Я просто хотел показать вам, что регулярное использование статического анализатора кода позволяет обнаруживать ошибки на самой ранней стадии. Это правильный способ использования статического анализа кода. Статический анализ должен быть встроен в процесс DevOps и служить основным фильтром ошибок. Все это хорошо описано в статье Внедряйте статический анализ в процесс, а не просто ищите с его помощью баги.
Вывод
Судя по некоторым пулл-реквестам, проект иногда проверяется через различные инструменты анализа кода, но исправления вносятся не постепенно, а группами и с большими интервалами. В некоторых запросах в комментарии указано, что изменения внесены только для подавления предупреждений. Такой подход к использованию анализа значительно снижает его полезность, поскольку именно регулярные проверки проекта позволяют сразу исправлять ошибки, а не ждать появления каких-либо явных багов.
Следите за нами и подписывайтесь на наши аккаунты и каналы в социальных сетях: Instagram, Twitter, Facebook, Telegram. Мы будем рады быть с вами, где бы вы ни были, и держать вас в курсе событий.
