Виталий Алферов

16 Мар 2016

Теги:

#CSharp

Проверяем исходный код плагина PVS-Studio с помощью PVS-Studio

16 Мар 2016

Автор: Виталий Алферов

Чуть подробнее о тестировании PVS-Studio
PVS-Studio Plugin
Заключение

Один из вечных вопросов, с которыми мы встречаемся, звучит так - "Вы проверяли PVS-Studio с помощью PVS-Studio? Где статья о результатах проверки?". Да, мы регулярно делаем это, поэтому мы никак не могли написать статью об ошибках, которые нашли сами в себе. Ошибки исправляются разработчиками ещё на этапе написания кода, и мы постоянно забываем в этот момент их выписать. Но читателям в этот раз повезло. Из-за недосмотра C# код плагина для Visual Studio не был добавлен в ежедневные ночные проверки, которые мы проводим. И, соответственно, в отличие от ядра анализатора, ошибки в нем не замечались на протяжении всего развития C# PVS-Studio. Как говорится, нет худа без добра, и благодаря этому вы и читаете данную статью.

0382_Checking_PVS-Studio_plugin_with_PVS-Studio_ru/image1.png

Чуть подробнее о тестировании PVS-Studio

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

Разрабатывая PVS-Studio, мы используем семь методик тестирования:

Статический анализ кода на машинах разработчиков. У всех разработчиков установлен PVS-Studio. Новый или изменённый код сразу проверяется с помощью механизма инкрементального анализа. Проверяется C++ и С# код.
Статический анализ кода при ночных сборках. Если предупреждение не было замечено, то оно выявится на этапе ночной сборки на сервере. PVS-Studio проверяет C# и C++ код. Помимо этого мы дополнительно используем Clang для проверки C++ кода.
Юнит-тесты уровня классов, методов, функций. Не очень развитая система, так как многие моменты сложно тестировать из-за необходимости подготавливать для теста большой объем входных данных. Мы больше полагаемся на высокоуровневые тесты.
Функциональные тесты уровня специально подготовленных и размеченных файлов с ошибками.
Функциональные тесты, подтверждающие, что мы корректно разбираем основные системные заголовочные файлы.
Регрессионные тесты уровня отдельных сторонних проектов и решений (projects and solutions). Самый важный и полезный для нас вид тестирования. Для его осуществления мы регулярно проверяем 105 открытых проектов на C++ и 49 на C#. Сравнивая старые и новые результаты анализа, мы контролируем, что что-то не сломали и оттачиваем новые диагностические сообщения.
Функциональные тесты пользовательского интерфейса расширения - надстройки, интегрируемой в среду Visual Studio.

Как же так получилось, что мы упустили проверку плагина? Сами не знаем. Просто никто не вспомнил добавить на сервере проверку кода плагина. Проверку нового C# анализатора добавили, а плагина - нет. В результате C# анализатор развивался и находил ошибки сам в себе. А плагин, также написанный на C#, остался в стороне. Он практически не изменялся за последнее время. Вот и вышло, что инкрементальный анализ не помог, так как с кодом плагина почти не работали. А ночных проверок не было.

PVS-Studio Plugin

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

Начнем, пожалуй, с реальных ошибок, которые к моменту написания этих строк уже исправлены.

public void ProcessFiles(....)
{
  ....
  int RowsCount = 
    DynamicErrorListControl.Instance.Plog.NumberOfRows;
  if (RowsCount > 20000)
    DatatableUpdateInterval = 30000; //30s
  else if (RowsCount > 100000)
    DatatableUpdateInterval = 60000; //1min
  else if (RowsCount > 200000)
    DatatableUpdateInterval = 120000; //2min
  ....
}

Анализатор, соответственно, выдал два предупреждения:

V3022 Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559

V3022 Expression 'RowsCount > 200000' is always false. ProcessingEngine.cs 561

Человеческий мозг привык думать последовательно - от простого к сложному или, как в данном случае, от меньшего к большему, проверяя все значения. В данном случае подобная логика человека привела к неправильному поведению программы. Ошибка возникла при проверке количества строк в таблице. Программа, проверив первое условие, что строк больше чем 20000, сразу присвоит DatatableUpdateInterval значение в 30 секунд. И, естественно, не станет проверять остальные условия. Даже если RowsCount был 1000000.

Данный код был написан с целью оптимизации вывода сообщений об ошибках в IDE окно PVS-Studio в Visual Studio. Изначально плагин PVS-Studio выдавал результаты анализа сразу по получению. То есть в тот момент, когда эти результаты приходили из процесса cmd (который запускает ядро анализатора). Однако на очень больших проектах мы стали замечать серьёзные "тормоза" интерфейса. Особенно проблемы проявлялись, когда в проектах присутствовало большое количество *.c файлов. Это происходило потому, что *.c файлы проверялись очень быстро, и UI поток оказывался постоянно занят перерисовкой таблицы результатов. Было принято решение добавить задержку между обновлениями, которая увеличивалась бы с ростом числа сообщений. До достижения 20000 сообщений в списке задержка выставлялась в 15 секунд.

В данном случае нам повезло, и мы получим лишь небольшое замедление в программе (тем более, что очень редко мы получаем больше сотни тысяч сообщений после проверки), но данное сообщение предназначено выявлять и более серьезные случаи. Например, как было в проекте от компании Infragistics.

public static double GenerateTemperature(GeoLocation location){
  ....
  else if (location.Latitude > 10 || location.Latitude < 25) 
  ....
  else if (location.Latitude > -40 || location.Latitude < 10)
  ....
}

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

Следующая ошибка более серьезна для нашего проекта:

public bool GeneratePreprocessedFile(....)
{
  ....
  if (info.PreprocessorCommandLine.Contains(" /arch:SSE"))
    ClangCommandLine += " /D \"_M_IX86_FP=1\"";
  else if (info.PreprocessorCommandLine.Contains(" /arch:SSE2"))
    ClangCommandLine += " /D \"_M_IX86_FP=2\"";
  else if (info.PreprocessorCommandLine.Contains(" /arch:IA32"))
    ClangCommandLine += " /U \"_M_IX86_FP\"";
  else if (info.PreprocessorCommandLine.Contains(" /arch:AVX"))
    ClangCommandLine += " /D \"_M_IX86_FP=2\"";
  ....
}

V3053 An excessive check. Examine the conditions containing search for the substrings ' /arch:SSE' and ' /arch:SSE2'. StandaloneProjectItem.cs 229

Хоть ошибка и имеет иной номер, суть её та же. Логика человека - следовать от простого к сложному, подкачала и здесь. Проверив значение "info.PreprocessorCommandLine" на вхождение в него подстроки " /arch:SSE", мы одновременно будем удовлетворять условию в том случае, когда "info.PreprocessorCommandLine" будет содержать следующую подстроку " /arch:SSE2". Как видите, текст, который естественным образом читается, совершенно не соответствует логике, которую мы хотели задать в программе. Даже если мы будем знать, что в PreprocessorCommandLine содержится "/arch:SSE2", то пройдя глазами по коду мы умозрительно прибавим к переменной ClangCommandLine значение " /D \"_M_IX86_FP=2\"", а не " /D \"_M_IX86_FP=1\"";

С точки зрения анализатора, ошибка заключалась в некорректном определении макроса _M_IX86_FP при передаче компилятору флага /arch:SSE2. Дело в том, что для препроцессирования перед непосредственно анализом, PVS-Studio использует Clang вместо cl (стандартный препроцессор Visual C++). Clang работает значительно быстрее. К сожалению, поддержка диалекта языка C++ от Microsoft в Clang до сих пор далека от идеала - поэтому, если Clang "не смог" что-то препроцессировать, PVS-Studio позовёт cl. Поэтому мы и преобразуем флаги компилятора cl в define для Clang. Подробности.

Данная ошибка, скорее всего, никогда не приводила к ошибкам в работе Clang или некорректной выдаче результатов анализа, поэтому она очень долго существовала в нашем коде.

Еще одной реальной ошибкой является вызов функции ProcessAnalyzerOutput.

private void PVSFinishKey(ref Hashtable PathExcludes)
{
  ....
  ProcessAnalyzerOutput(fileName,
                        projectName, 
                        task.strStandardOutput, 
                        task.strStandardError, 
                        false, 
                        ref ParsedOutput, 
                        ref PathExcludes);
}

Заметить ошибку непросто, даже если взглянуть на объявление функции:

private void ProcessAnalyzerOutput(
                        String fileName, 
                        String projectName, 
                        String strStandardError, 
                        String strStandardOutput, 
                        bool LargeFileMode, 
                        ref List<ErrorInfo> ParsedOutputLines, 
                        ref Hashtable PathExcludes)
{
  ....
}

Проблема состоит в несоответствии имен параметров функции и имен аргументов, которые мы туда передаем:

V3066 Possible incorrect order of arguments passed to 'ProcessAnalyzerOutput' method: 'strStandardError' and 'strStandardOutput'. ProcessingEngine.cs 1995

В подобном длинном списке параметров функции всегда сложно обнаружить несоответствие. Даже IntelliSense в подобных случаях не всегда спасает. В больших проектах он бывает подтормаживает, и из-за этого не всегда понятно, на каком элементе ты сейчас находишься.

0382_Checking_PVS-Studio_plugin_with_PVS-Studio_ru/image2.png

Из-за этого может произойти весьма неприятная ситуация. Хотя эта диагностика и относится к третьему уровню, как и все остальные эвристические, но весьма полезна, и не стоит её игнорировать.

Место, в котором была выявлена данная ошибка, является "заглушкой" - в параметры stderr и stdout никогда не приходило что-то кроме пустых строк. Данная ошибка обнаружила бы себя достаточно быстро, когда данной заглушкой начали бы пользоваться.

Еще одна ошибка была выявлена диагностикой под номером V3072, которая на данный момент находится в разработке:

sealed class ProcessingEngine
{
  ....
  private readonly PVSMessageBase _retiredMessageBase; 
  ....
}
public sealed class PVSMessageBase : 
       ContextBoundObject, IDisposable
{
  ....
}

Данная диагностика предназначена для нахождения в классе, не реализующий интерфейс IDisposable, полей, которые имеют тип, реализующие интерфейс IDisposable. Подобный код говорит нам о том, что программист, возможно, забывает очищать некоторые ресурсы после использования объекта своего класса.

В данном случае в классе ProcessingEngine (который не реализует интерфейс IDisposable) присутствует поле класса PVSMessageBase, тип которого реализует интерфейс IDisposable.

Данное срабатывание можно отнести к false positive, вызванным не совсем "опрятной" архитектурой. Класс ProcessingEngine используется в программе как singleton. И поэтому единственный его экземпляр существует в течении всего времени работы программы так же, как и поле PVSMessageBase. Ресурсы будут освобождены после завершения программы.

Ну что ж, назло недоброжелателям и на радость нам, других серьезных ошибок в коде не было найдено. Остальная часть "ошибок" скорее относится к формату - "на всякий случай".

Например, вот в таком выражении:

private int GetSetRemainingClicks(....)
{
  ....
  if ((CurrentRemClicks != 0) || 
      ((CurrentRemClicks == 0) && DecrementCurrent))
  {
    ....
  }
  ....
}

V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. DynamicErrorList.cs 383

Данный код можно смело сократить до:

if (CurrentRemClicks != 0 || DecrementCurrent)
{

Также было найдено несколько "перестраховок":

private void comboBoxFind_KeyDown(object sender, KeyEventArgs e)
{
  ....
  if (e.KeyCode == Keys.Escape)
  {
    if (e.KeyCode == Keys.Escape)
    {
      ProcessingEngine.PluginInstance.HidePVSSearchWindow();
    }
  }
}

А вот здесь еще раз перепроверили очевидную вещь:

public IList<KeyValuePair<String, DataRow>> 
  GetSortedNonRetiredRows()
  {
    if (ei.ProjectNames.Count == 1)
    {
      ....
    }
    else if (ei.ProjectNames.Count > 1)
    {
      ....
    }
    else if (ei.ProjectNames.Count == 0)
    {
      ....
    }
  }

V3022 Expression 'ei.ProjectNames.Count == 0' is always true. PlogController.cs 277

Уж если начал перестраховываться, то надо держаться до конца и проверять всё. Как, например, здесь:

void ProcessVCProjAnalysisIntegration (String Path, bool Remove)
{
  if (Remove)
  {
    ....
  }
  else if (!Remove)
  {
    ....
  }
}

V3022 Expression '!Remove' is always true. VCProjectEngine.cs 733

Иногда код доходит до "весьма странных приведений", но уж если обещали писать обо всем, то пишем:

private bool PostWebRequest(String post_data)
{
  ....
  String Sts = ex.Status.ToString() as string;
  ....
  string sts = wex.Status.ToString() as string;
  ....
}

V3051 An excessive type cast. The object is already of the 'String' type. TrialExtensionRequest.cs 181

V3051 An excessive type cast. The object is already of the 'String' type. TrialExtensionRequest.cs 241

Еще в нашем коде встречаются следующие проверки:

private String Get_StructMemberAlignment()
{
  ....
  if (CompileAsManaged.Equals("false") ||
      String.IsNullOrEmpty(CompileAsManaged))
    Params += " /GR-";
  ....
}

V3027 The variable 'CompileAsManaged' was utilized in the logical expression before it was verified against null in the same logical expression. MSVCParamsGenerator.cs 801

И еще раз:

private String Get_DisableLanguageExtensions()
{
  ....
  else if (DisableLanguageExtensions.Equals("false") ||
           String.IsNullOrEmpty(DisableLanguageExtensions))
  {
  ....
}

V3027 The variable 'DisableLanguageExtensions' was utilized in the logical expression before it was verified against null in the same logical expression. MSVCParamsGenerator.cs 1118

Ошибочно проверять переменную на null после того, как была вызвана функция Equals. На самом деле настоящей ошибки здесь нет, так как согласно API, в переменных "CompileAsManaged" и "DisableLanguageExtensions" не может быть null. Поэтому проверки можно упростить до:

CompileAsManaged == string.Empty
DisableLanguageExtensions == string.Empty

Посмотрим, где еще мы написали код, удостоенный внимания со стороны анализатора:

private static DialogResult ShowModalDialog(....)
{
  ....
  if (buttons == MessageBoxButtons.YesNo || 
     buttons == MessageBoxButtons.YesNoCancel)
       return DialogResult.Yes;
  else if (buttons == MessageBoxButtons.OKCancel)
       return DialogResult.OK;
  else
       return DialogResult.OK;
}

V3004 The 'then' statement is equivalent to the 'else' statement. Utilities.cs 496

Проверка переменной buttons на равенство MessageBoxButtons.OKCancel не имеет смысла т.к. в любом случае мы вернем DialogResult.OK. Как следствие, в нашем случае код сокращается до:

return (buttons == MessageBoxButtons.YesNo || 
       buttons == MessageBoxButtons.YesNoCancel) ?
       DialogResult.Yes : DialogResult.OK;

И последнее. Здесь, скорее всего, виноват рефакторинг:

public bool ReadPlog(....)
{
  ....
  XmlReadMode readMode = XmlReadMode.Auto;
  ....
  readMode = dataset.ReadXml(filename);
  ....
}

V3008 The 'readMode' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 512, 507. PlogController.cs 512

Заключение

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

- ... это не ошибки...

- ... это легко исправить...

- ... 5 лет работало, и никто не жаловался...

Да, действительно, многие ошибки легко исправить, и это очень хорошо. Но вот заметить эту опечатку или ошибку бывает очень непросто. Часто ошибку замечает не сам программист, а тестер или, что еще хуже - пользователь.

Цель нашего анализатора - помочь вам увидеть эти ошибки и опечатки. Согласитесь, гораздо лучше писать большой текст в Microsoft Word, чем в блокноте. А код программ зачастую больше некоторых нашумевших бестселлеров.

PVS-Studio, по факту, и является системой проверки орфографии из Microsoft Word для вашего кода. Он просто подскажет вам, что здесь вы поторопились и опечатались, или, возможно, выразили свою мысль не так, как хотели. Согласитесь - правильное формулирование мысли в тексте книги очень важно для читателя, а правильное формирование логики программы важно для конечного пользователя. Пользуясь PVS-Studio, вы сможете гораздо лучше выразить и сформулировать свою мысль.

Желаем вам вдохновения и четкой мысли, а мы вам в этом поможем.

#CSharp