Форум программистов, компьютерный форум, киберфорум
el_programmer
Войти
Регистрация
Восстановить пароль
Блоги Сообщество Поиск Заказать работу  
PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#.

PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.

Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.

Ищем ошибки в игровом движке Xenko

Запись от el_programmer размещена 11.03.2016 в 09:54
Показов 3152 Комментарии 1

Ищем ошибки в игровом движке Xenko

Автор: Васильев Сергей

Движков с открытым исходным кодом, написанных на C++, куда больше, чем аналогичных движков, написанных на C#. Но есть исключения. Xenko – один из движков, написанных на C# и имеющих открытый исходный код. О том, что же интересного удалось найти в коде этого движка, будет рассказано в этой статье.


Анализируемый проект


Xenko (ранее известный как Paradox) - кроссплатформенный игровой движок, позволяющий разрабатывать игры, используя для этого язык программирования C#. Движок позволяет разрабатывать как 2D, так и 3D-игры под разные платформы: Android, iOS, Windows Desktop, Windows Phone, PlayStation 4. В будущем также планируется поддержка MacOSX и Linux. Исходный код движка доступен в репозитории на GitHub. Большая часть кода (89% по информации с GitHub), написана на C#.


Инструмент анализа

Проект проверялся с помощью анализатора PVS-Studio. Помимо уже привычных ошибок (вроде V3001), в нём обнаружились подозрительные фрагменты кода, диагностируемые правилами из последнего релиза анализатора.


Все диагностические сообщения содержат документацию, в которой приводятся примеры ошибок, их описание, а также способы их исправления.

Дабы не быть голословным, предлагаю посмотреть, что же нашлось интересного.


Подозрительные фрагменты кода

Часто ошибки ведут к более серьёзным последствиям, чем кажется на первый взгляд. Для того, чтобы лучше понять их суть и способы исправления, рекомендую ознакомиться с документацией к диагностическим правилам.

C#
1
2
3
4
5
6
7
8
9
10
public bool CanHandleRequest(TexImage image, IRequest request)
{
  ....
  return SupportFormat(compress.Format) && 
         SupportFormat(image.Format);
  ....
  return SupportFormat(converting.Format) && 
         SupportFormat(converting.Format);   //<=
  ....
}
Предупреждение PVS-Studio:V3001 There are identical sub-expressions 'SupportFormat(converting.Format)' to the left and to the right of the '&&' operator. SiliconStudio.TextureConverter DxtTexLib.cs 141

Часто люди думают: "Ну, проверили два раза условие, ничего страшного в этом нет". На первый взгляд - да, и порой это действительно так. Но чаще проблема заключается в ином - условие перепутали, следовательно, допущена логическая ошибка и логика программы не соответствует задуманной. Так же и здесь. Два раза выполняется проверка подусловия за счёт вызова метода 'SupportFormat(converting.Format)', но скорее всего во втором случае подразумевался вызов следующего вида: 'SupportFormat(image.Format)'. Тогда всё выражение примет вид:

C#
1
2
return SupportFormat(converting.Format) && 
       SupportFormat(image.Format);
Схожая ошибка (при чём в этом же методе):

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
public enum Rescaling
{
  Box = 0,
  Bicubic = 1,
  Bilinear = 2,
  BSpline = 3,
  CatmullRom = 4,
  Lanczos3 = 5,
  Nearest,
}
 
public bool CanHandleRequest(TexImage image, IRequest request)
{
  ....
  return rescale.Filter == Filter.Rescaling.Box     || 
         rescale.Filter == Filter.Rescaling.Bicubic || //<=
         rescale.Filter == Filter.Rescaling.Bicubic || //<=
         rescale.Filter == Filter.Rescaling.Nearest;
  ....
}
Предупреждение PVS-Studio:V3001 There are identical sub-expressions 'rescale.Filter == Filter.Rescaling.Bicubic' to the left and to the right of the '||' operator. SiliconStudio.TextureConverter DxtTexLib.cs 148

В коде, приведённом в статье, ошибку видно невооружённым глазом. А вот в файле с этим кодом она, мягко говоря, не бросается в глаза. Отчасти в этом "заслуга" форматирования - в файле это выражение записано в одну строку, так что дублирование подвыражений без детального просмотра кода можно и не заметить. Думаю, подразумевался другой элемент перечисления, например, 'BSpline'.

Вообще в больших выражениях достаточно легко допустить подобную ошибку, как это будет видно на следующем примере. Попробуйте найти ошибку самостоятельно, не читая предупреждения анализатора и пояснений к коду:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
public static ContainmentType BoxContainsSphere(
                                ref BoundingBox box, 
                                ref BoundingSphere sphere)
{
  ....
  if ((((box.Minimum.X + sphere.Radius <= sphere.Center.X)  &&    
        (sphere.Center.X <= box.Maximum.X - sphere.Radius)) &&   
       ((box.Maximum.X - box.Minimum.X > sphere.Radius)     &&
       (box.Minimum.Y + sphere.Radius <= sphere.Center.Y))) &&  
      (((sphere.Center.Y <= box.Maximum.Y - sphere.Radius)  && 
        (box.Maximum.Y - box.Minimum.Y > sphere.Radius))    &&
      (((box.Minimum.Z + sphere.Radius <= sphere.Center.Z)  &&  
      (sphere.Center.Z <= box.Maximum.Z - sphere.Radius))   && 
        (box.Maximum.X - box.Minimum.X > sphere.Radius))))
  ....
}
Предупреждение PVS-Studio:V3001 There are identical sub-expressions 'box.Maximum.X - box.Minimum.X > sphere.Radius' to the left and to the right of the '&&' operator. SiliconStudio.Core.Mathematics Collision.cs 1322

Разобраться в таком коде явно непросто... Давайте попробуем упростить выражение, заменив подвыражение простыми буквами (опустив при этом скобки). Тогда получится следующий код:

C#
1
if (A && B && C && D && E && F && G && H && C)
Несмотря на то, что количество подвыражений по-прежнему впечатляет, ошибку заметить проще. В коде два раза проверяется подвыражение 'C', соответствующее 'box.Maximum.X - box.Minimum.X > sphere.Radius'. Если внимательно посмотреть на исходное выражение, можно понять, что на самом деле вместо данного подвыражения должно находиться следующее:

C#
1
box.Maximum.Z - box.Minimum.Z > sphere.Radius
Идём дальше:

C#
1
2
3
4
5
6
7
8
9
10
....
/// <exception cref="System.ArgumentNullException">
/// key is null.</exception>
public bool Remove(KeyValuePair<TKey, Tvalue> item)
{
  if (item.Key == null ||
      item.Key == null)
    throw new ArgumentException();
  ....
}
Предупреждение PVS-Studio:V3001 There are identical sub-expressions 'item.Key == null' to the left and to the right of the '||' operator. SiliconStudio.Core MultiValueSortedDictionary.cs 318

Как минимум - условие выглядит странно. Можно было бы предположить, что здесь тоже подразумевалось другое подвыражение, но судя по комментарию - нет. Выходит, это опечатка, хотя не совсем понятно, как её можно было допустить. В любом случае, код необходимо поправить.

Нередко ошибаются и в присваиваниях, когда выполняется присваивание объекта самому себе. В подобных случаях, глядя со стороны, бывает сложно сказать, как правильно исправить код. Посмотрим на несколько таких мест:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
public ParameterComposedKey(ParameterKey key, string name, 
                            int indexer)
{
  Key = key;
  Name = name;
  Indexer = indexer;
 
  unchecked
  {
    hashCode = hashCode = Key.GetHashCode();
    hashCode = (hashCode * 397) ^ Name.GetHashCode();
    hashCode = (hashCode * 397) ^ Indexer;
  }
}
Предупреждение PVS-Studio:V3005 The 'hashCode' variable is assigned to itself. SiliconStudio.Xenko ParameterKeys.cs 346

Поле 'hashCode' присваивается само себе. Как минимум - это лишнее присваивание, но скорее всего в методе хеширования допущена ошибка. Видится несколько вариантов исправления:
  1. Убрать лишнее присваивание;
  2. Заменить первое присваивание на подвыражение, аналогичное стоящим ниже (hashCode * 397);
  3. Возможно, стоит также вызвать метод 'GetHashCode()' у свойства 'Indexer'.

Как правильно поступить в данном случае - решать автору кода.

В коде встречались выражения, значение которых всегда было истинным или ложным. Подобные случаи выявляет диагностика V3022, и дальше будут приведены некоторые фрагменты кода, которые удалось обнаружить с её помощью.

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
private void SetTime(CompressedTimeSpan timeSpan)
{
  ....
  while (....)
  {
    var moveNextFrame = currentKeyFrame.MoveNext();
    if (!moveNextFrame)
    {
      ....  
      break;      
    }        
    var keyFrame = moveNextFrame ? currentKeyFrame.Current :  
                                   data.ValueNext;
    ....
  }
  ....
}
Предупреждение PVS-Studio:V3022 Expression 'moveNextFrame' is always true. SiliconStudio.Xenko.Engine AnimationChannel.cs 314

В тернарном операторе переменная 'moveNextFrame' всегда будет иметь значение 'true'. В ином случае будет осуществлён выход из цикла до выполнения рассматриваемого оператора. Таким образом, если выполнение всё же дойдёт до тернарного оператора, то объект 'keyFrame' всегда будет иметь одно и то же значение - 'currentKeyFrame.Current'.

Предупреждения для фрагментов кода, в которых прослеживается схожая ситуация:
  • V3022 Expression 'inputTexture.Dimension == TextureDimension.TextureCube' is always true. SiliconStudio.Xenko.Engine LambertianPrefilteringNoCompute.cs 66
  • V3022 Expression 'inputTexture.Dimension == TextureDimension.TextureCube' is always true. SiliconStudio.Xenko.Engine LambertianPrefilteringSH.cs 72

Продолжим обзор:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
public enum Diff3ChangeType
{
  None,
  Children,
  MergeFromAsset1,
  MergeFromAsset2,
  MergeFromAsset1And2,
  Conflict,
  ConflictType,
  ConflictArraySize,
  InvalidNodeType,
}
 
private static bool CheckVisitChildren(Diff3Node diff3)
{
  return diff3.ChangeType == Diff3ChangeType.Children || 
         diff3.ChangeType != Diff3ChangeType.None;
}
Предупреждение PVS-Studio:V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. SiliconStudio.Assets Diff3Node.cs 70

Данное выражение или избыточно, или содержит ошибку. Если истинно первое подвыражение, то второе подвыражение также всегда будет истинным (впрочем, до его вычисления выполнение не дойдёт). Данное выражение можно упростить до 'diff3.ChangeType != Diff3ChangeType.None'. Скорее всего, в рассматриваемом случае реализована просто излишняя проверка, хотя в некоторых случаях это может свидетельствовать об иной ошибке - когда проверяется не та переменная. Подробнее об этом написано в документации к диагностике.

Нашлась пара интересных мест со строками форматирования:

C#
1
2
3
4
5
6
7
8
9
10
11
public string ToString(string format, IFormatProvider formatProvider)
{
  if (format == null)
    return ToString(formatProvider);
 
  return string.Format(formatProvider,
                       "Red:{1} Green:{2} Blue:{3}",
                       R.ToString(format, formatProvider),
                       G.ToString(format, formatProvider), 
                       B.ToString(format, formatProvider));
}
Предупреждение PVS-Studio:V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 4. Present: 3. SiliconStudio.Core.Mathematics Color3.cs 765

Параметры строки форматирования индексируются с {0}, здесь же индексация начинается с {1}. В данном случае строка форматирования ожидает 4 аргумента, но представлены только 3, из-за чего будет сгенерировано исключение типа 'FormatException'. Для исправления данной ошибки необходимо правильно пронумеровать индексы в строке форматирования.

C#
1
"Red:{0} Green:{1} Blue:{2}"
Другой пример:

C#
1
2
3
4
5
6
7
8
public static bool IsValidNamespace(string text, out string error)
{
  ....
  error = items.Where(s => !IsIdentifier(s))
               .Select(item => string.Format("[{0}]", item, text))
               .FirstOrDefault();
  ....
}
Предупреждение PVS-Studio:V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 1. Present: 2. SiliconStudio.Core.Design NamingHelper.cs 56

С точностью противоположная ситуация - строка форматирования требует наличия 1 аргумента, однако метод содержит 2 - 'item' и 'text'. В данном случае ненужный аргумент попросту будет проигнорирован, но такой код должен навести на подозрения. В лучшем случае второй аргумент просто является лишним и его можно удалить, в худшем - строка форматирования составлена ошибочно.

C#
1
2
3
4
5
6
7
8
9
private bool requestedExit;
public void MainLoop(IGameDebuggerHost gameDebuggerHost)
{
  ....
  while (!requestedExit)
  {
    Thread.Sleep(10);
  }
}
Предупреждение PVS-Studio:V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. SiliconStudio.Xenko.Debugger GameDebuggerTarget.cs 225

Этот цикл ожидает некого события извне и должен выполняться до тех пор, пока переменная 'requestedExit' имеет значение 'false'. Однако после оптимизации данный цикл может превратиться в бесконечный из-за того, что компилятор может закешировать значение переменной 'requestedExit'. Данные ошибки достаточно трудно отлавливаемы, так как именно из-за кеширования, проводимого при оптимизации, поведение программы 'Debug' и 'Release' версиях может отличаться. Для исправления ошибки можно добавить модификатор 'volatile' при объявлении поля или же использовать специализированные средства синхронизации. Подробнее об этом можно прочитать в документации к данной диагностике.

Следующий странный фрагмент кода:

C#
1
2
3
4
5
6
7
8
private void QuickSort(List<TexImage> list, int left, int right)
{
  int i = left;
  int j = right;
  double pivotValue = ((left + right) / 2);
  int x = list[(int)pivotValue].DataSize;
  ....
}
Предупреждение PVS-Studio:V3041 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. SiliconStudio.TextureConverter AtlasTexLibrary.cs 422

Сразу хочу сказать, что переменная 'pivotValue', кроме как в приведённом фрагменте нигде не используется. Данная переменная имеет тип 'double', однако при её инициализации будет производится целочисленное деление, так как типы всех переменных, участвующих в инициализирующем выражении - целочисленные. Более того, далее выполняется обратное приведение данной переменной обратно к типу 'int'. Таким образом, можно было бы сразу использовать тип 'int' при объявлении переменной 'pivotValue', или же использовать инициализирующее выражение для вычисления индекса массива. Так или иначе, код выглядит странно и его стоит упростить.

Следующее предупреждение связано с подсистемой WPF:

C#
1
2
3
4
5
6
7
8
9
10
public static readonly DependencyProperty KeyProperty = 
  DependencyProperty.Register("Key", 
                              typeof(object),
                              typeof(TextBoxKeyUpCommandBehavior), 
                              new PropertyMetadata(Key.Enter));
 
public Key Key { 
  get { return (Key)GetValue(KeyProperty); } 
  set { SetValue(KeyProperty, value); } 
}
Предупреждение PVS-Studio:V3046 WPF: the type registered for DependencyProperty does not correspond with the type of the property used to access it. SiliconStudio.Presentation TextBoxKeyUpCommandBehavior.cs 18

При регистрации свойства зависимости было указано, что свойство хранит значение типа 'object'. Таким образом, в данное свойство может быть записано значение любого типа, однако при попытке обращения к нему возможно возникновение исключения в случае, если тип объекта, записанного в свойство, невозможно привести к типу 'Key'. О том, что при регистрации свойства в качестве хранимого типа необходимо задать 'Key' свидетельствует и то, что в качестве значения по умолчанию для свойства указано значение 'Key.Enter'.


Новые диагностические правила

Как я упоминал в начале статьи, в проекте удалось обнаружить места, выявленные новыми диагностическими сообщениями, добавленными в последнем релизе PVS-Studio. Обзор некоторых таких мест приведён ниже.

Встретилось несколько фрагментов кода, в которых один из параметров метода перезаписывался, но перед этим его значение никак не использовалось. Таким образом, значение, пришедшее в метод, попросту теряется:

C#
1
2
3
4
5
6
7
8
9
10
11
12
13
internal delegate void InternalValueChangedDelegate(
  InternalValue internalValue, object oldValue);
 
private static InternalValueChangedDelegate  
CreateInternalValueChangedEvent(
  ParameterKey key, 
  InternalValueChangedDelegate internalEvent, 
  ValueChangedDelegate originalEvent)
{
    internalEvent = (internalValue, oldValue) => 
      originalEvent(key, internalValue, oldValue);
    return internalEvent;
}
Предупреждение PVS-Studio: V3061 Parameter 'internalEvent' is always rewritten in method body before being used. SiliconStudio.Xenko ParameterCollection.cs 1158

Данный код выглядит странно, так как объект 'internalEvent' нигде не используется, сразу же перезаписывается, после чего возвращается из метода. Тогда из сигнатуры метода вообще можно было бы убрать этот параметр, а тело метода упростить до следующего вида:

C#
1
2
return (internalValue, oldValue) => 
  originalEvent(key, internalValue, oldValue);
Но возможно, что ошибка более интересна и на самом деле данный метод предназначался для создания цепочки делегатов. В таком случае решением проблемы будет исправление знака '=' на '+='.

Встретились ещё 2 случая с перезаписью параметра:

C#
1
2
3
4
5
6
7
8
9
10
private void Load(TexImage image, DxtTextureLibraryData libraryData, 
                  LoadingRequest loader)
{
  ....
  libraryData = new DxtTextureLibraryData(); //<=
  image.LibraryData[this] = libraryData;
 
  libraryData.Image = new ScratchImage();
  ....
}
Предупреждение PVS-Studio:V3061 Parameter 'libraryData' is always rewritten in method body before being used. SiliconStudio.TextureConverter DxtTexLib.cs 213

Параметр 'libraryData' перезаписывается до того, как его значение где-то используется. При этом он не отмечен модификаторами 'ref' или 'out'. Это очень странно, так как значение, принимаемое методом, попросту теряется.

Предупреждение, выданное на схожий код: V3061 Parameter 'libraryData' is always rewritten in method body before being used. SiliconStudio.TextureConverter FITexLib.cs 244

Противоположная ситуация - метод принимает аргумент, значение которого не используется:

C#
1
2
3
4
5
6
7
8
9
10
11
private static ImageDescription 
CreateDescription(TextureDimension dimension, 
                  int width, int height, int depth, ....)
 
public static Image New3D(int width, int height, int depth, ....)
{
    return new Image(CreateDescription(TextureDimension.Texture3D,  
                                       width, width, depth,  
                                       mipMapCount, format, 1), 
                     dataPointer, 0, null, false);
}
Предупреждение PVS-Studio:V3065 Parameter 'height' is not utilized inside method's body. SiliconStudio.Xenko Image.cs 473

Как видно из предупреждения анализатора, параметр 'height' нигде не используется. Вместо него в метод 'CreateDescription' дважды передаётся параметр 'width', что может свидетельствовать о наличии ошибки. Правильный вызов метода 'CreateDescription' может выглядеть так:

C#
1
2
CreateDescription(TextureDimension.Texture3D,
                  width, height, depth, mipMapCount, format, 1)

Заключение

Было интересно проверить игровой движок, написанный на C#. Все допускают ошибки, и различные инструменты позволяют минимизировать их количество, и статический анализатор - один из таких инструментов. И чем раньше будет найдена ошибка, тем дешевле обойдётся её исправление.

Конечно, не все ошибки, найденные в проекте, были выписаны в статье. Во-первых, это сильно бы увеличило размер статьи, во-вторых - некоторые из диагностических сообщений являются специфичными, т.е. актуальными для определённых типов проектов, поэтому могут быть интересны не всем. Но, безусловно, разработчикам (а возможно и просто любопытным программистам) будет интересно посмотреть все подозрительные места, найденные анализатором. Это можно сделать, загрузив пробную версию анализатора.
Миниатюры
Нажмите на изображение для увеличения
Название: image1 (5).png
Просмотров: 810
Размер:	107.4 Кб
ID:	3661   Нажмите на изображение для увеличения
Название: image2.png
Просмотров: 723
Размер:	29.8 Кб
ID:	3662   Нажмите на изображение для увеличения
Название: image3.png
Просмотров: 751
Размер:	131.4 Кб
ID:	3663  

Нажмите на изображение для увеличения
Название: image5.png
Просмотров: 1010
Размер:	260.2 Кб
ID:	3664  
Размещено в Без категории
Надоела реклама? Зарегистрируйтесь и она исчезнет полностью.
Всего комментариев 1
Комментарии
  1. Старый комментарий
    Аватар для Eva Rosalene
    Собственно ссылка на источник: https://habrahabr.ru/company/p... og/278881/
    Запись от Eva Rosalene размещена 12.03.2016 в 00:40 Eva Rosalene вне форума
 
Новые блоги и статьи
20. Мат мед. Абсентеизм как отдельный тип простоя
anaschu 29.05.2026
Апдейт модели: исправленные баги, абсентеизм и новые механизмы Продолжаю развивать ранее описанную модель рабочего коллектива на AnyLogic. За последние несколько дней был проведён серьёзный. . .
19. здоровье, усталость и психотип работника влияют на производительность предприятия, и наоборот, производительность на здоровье, усталось и психотип
anaschu 28.05.2026
Дискретно-событийная модель рабочего коллектива на AnyLogic: здоровье, выгорание, психотипы и микростимуляция Привет, коллеги. Хочу поделиться итогами нескольких недель работы над симуляционной. . .
"Прокси" для последовательного порта
Eddy_Em 28.05.2026
Эту штуку написал я достаточно давно. Но сейчас вот понадобилось настроить датчик грозы, но при этом не отключать его от "метеодемона". Соответственно, надо запустить этот "прокси": метеодемон будет. . .
Рефакторинг программы уравнивания.
Massaraksh7 26.05.2026
Пример по предыдущей записи в блоге. Но, надо заметить, что, во-первых, там оптимизация не только математики, но и работы с базой данных, и с графами, а во-вторых, это ещё не всё.
Использование TThread в Lazarus для математических вычислений.
Massaraksh7 25.05.2026
Производя рефакторинг своих программ на предмет ускорения их работы, обратил внимание на такой аспект, как сокращение времени матвычислений. Дело в том, что приходится работать с большими матрицами. . .
Модель здравосохранения 18. Чем здоровее работник, тем быстрее выгорает
anaschu 24.05.2026
Имитационная модель корпоративного здравоохранения: что показывает математика Сегодня в модели рабочего коллектива на AnyLogic появились три новые механики — выгорание через накопленную усталость,. . .
Модель здравосохранения 17. Планы на выгорание
anaschu 23.05.2026
Вот конкретная схема реализации: В классе Работник добавить: накопленнаяУсталость — растёт каждый час работы, снижается в перерывы и болезни коэффициентПрезентеизма — снижает продуктивность. . .
Изменение цветов в палитре gif файла aka фавикона
russiannick 23.05.2026
Изменение цветов в палитре gif файла, юзаемого как фавиконка в составе html-файла, помещенная в base64, средствами нативного Java Script, навеянное сном в майский день. Для работы необходим браузер,. . .
КиберФорум - форум программистов, компьютерный форум, программирование
Powered by vBulletin
Copyright ©2000 - 2026, CyberForum.ru