Находим ошибки в коде проекта LLVM с помощью анализатора PVS-Studio31.10.2016 16:48

Около двух месяцев назад я написал статью о проверке компилятора GCC с помощью анализатора PVS-Studio. Идея статьи была следующая: предупреждения GCC — это хорошо, но недостаточно. Надо использовать специализированные инструменты анализа кода, например, PVS-Studio. В качестве подтверждения я показал ошибки, которые PVS-Studio смог найти в коде GCC. Ряд читателей заметили, что качество кода GCC и его диагностики так себе, в то время как компилятор Clang современен, качественен, свеж и молод. В общем Clang — это ого-го! Что ж, значит пришло время мне проверить с помощью PVS-Studio проект LLVM.

Проверяем LLVM с помощью Linux версии PVS-Studio

Думаю, мало тех, кто не знает, что такое LLVM. Тем не менее, сохраню традицию кратко описывать проект, который был проверен.

LLVM (Low Level Virtual Machine) — универсальная система анализа, трансформации и оптимизации программ, реализующая виртуальную машину с RISC-подобными инструкциями. Может использоваться как оптимизирующий компилятор этого байткода в машинный код для различных архитектур, либо для его интерпретации и JIT-компиляции (для некоторых платформ). В рамках проекта LLVM был разработан фронтенд Clang для языков C, C++ и Objective-C, транслирующий исходные коды в байткод LLVM, и позволяющий использовать LLVM в качестве полноценного компилятора.

Официальный сайт: http://llvm.org/

Проверке подвергалась ревизия 282481. Анализ проводился новой версией PVS-Studio, работающей под Linux. Поскольку PVS-Studio for Linux — это новый продукт, то ниже я расскажу подробнее как выполнялась проверка. Уверен, это покажет, что использовать наш анализатор в Linux совсем не сложно, и вы можете, не откладывая, попробовать проверить собственный проект.

Linux-версия анализатора доступна для скачивания на следующей странице: http://www.viva64.com/ru/pvs-studio-download-linux/

Предыдущие проекты мы проверяли с помощью универсального механизма, который отслеживает запуски компилятора. В этот раз мы воспользуемся для проверки информацией, которую PVS-Studio возьмёт из JSON Compilation Database. Подробности можно почерпнуть из раздела документации «Как запустить PVS-Studio в Linux».

В LLVM 3.9 полностью отказались от autoconf в пользу CMake, и это стало хорошим поводом опробовать в действии поддержку JSON Compilation Database. Что это такое? Это формат, используемый утилитами Clang. В нём хранится список вызовов компилятора в следующем виде:

[
  {
    "directory": "/home/user/llvm/build",
    "command": "/usr/bin/c++ .... file.cc",
    "file": "file.cc"
  },
  ....
]

Для CMake-проектов получить такой файл очень просто — достаточно выполнить генерацию проекта с дополнительной опцией:

cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ../llvm

После этого в текущем каталоге появится compile_commands.json. Он и нужен нам для проверки. Так как некоторые проекты используют кодогенерацию, сначала выполним сборку.

make -j8

Теперь всё готово для анализа. Запускается он одной строчкой:

pvs-studio-analyzer analyze -l ~/PVS-Studio.lic -o PVS-Studio.log -j

Для проектов, не использующих CMake, получить compile_commands.json можно с помощью утилиты Bear. Но для сложных сборочных систем, активно использующих переменные окружения или кросс-компиляцию, полученные команды не всегда предоставляют подробную информацию о юните трансляции.

Примечание N1. Как работать с отчетом PVS-Studio в Linux.

Примечание N2. Мы оказываем качественную и быструю поддержку нашим клиентам и потенциальным пользователям. Поэтому, если вам что-то непонятно или что-то не получается, напишите нам в поддержку. Вам понравится наш сервис.

Результаты проверки

Кстати, это уже не первая проверка LLVM. Статьи, написанные по мотивам предыдущих проверок:

• PVS-Studio vs Clang (2011);
• Статический анализ следует применять регулярно (2012).

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

Всё, лирика закончилась, перейду к самому интересному. Рассмотрим подозрительные места в коде LLVM, на которые указал мне PVS-Studio.

Небитовые поля

В коде имеется вот такое перечисление:

enum Type {
  ST_Unknown, // Type not specified
  ST_Data,
  ST_Debug,
  ST_File,
  ST_Function,
  ST_Other
};

Это, если так можно сказать, «классическое перечисление». Каждому имени в перечислении присваивается целочисленное значение, которое соответствует определенному месту в порядке значений в перечислении:

• ST_Unknown = 0
• ST_Data = 1
• ST_Debug = 2
• ST_File = 3
• ST_Function = 4
• ST_Other = 5

Ещё раз подчеркну, что это просто перечисление, а не набор масок. Если бы эти константы можно было сочетать между собой, то они бы являлись степенью числа 2.

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

void MachODebugMapParser::loadMainBinarySymbols(....)
{
  ....
  SymbolRef::Type Type = *TypeOrErr;
  if ((Type & SymbolRef::ST_Debug) ||
      (Type & SymbolRef::ST_Unknown))
    continue;
  ....
}

Предупреждение PVS-Studio: V616 The 'SymbolRef: ST_Unknown' named constant with the value of 0 is used in the bitwise operation. MachODebugMapParser.cpp 448

Вспомним, что константа ST_Unknown равна нулю. Следовательно, выражение можно сократить:

if (Type & SymbolRef::ST_Debug)

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

if ((Type == SymbolRef::ST_Debug) || (Type == SymbolRef::ST_Unknown))

Чтобы избежать подобных ошибок, думаю, следовало использовать enum class. В этом случае некорректное выражение просто бы не скомпилировалось.

Одноразовые циклы

Функция не очень сложная, поэтому я решил привести её целиком. Прежде чем читать статью дальше, предлагаю самостоятельно догадаться, что здесь подозрительно.

Parser::TPResult Parser::TryParseProtocolQualifiers() {
  assert(Tok.is(tok::less) && "Expected '<' for qualifier list");
  ConsumeToken();
  do {
    if (Tok.isNot(tok::identifier))
      return TPResult::Error;
    ConsumeToken();
    
    if (Tok.is(tok::comma)) {
      ConsumeToken();
      continue;
    }
    
    if (Tok.is(tok::greater)) {
      ConsumeToken();
      return TPResult::Ambiguous;
    }
  } while (false);
  
  return TPResult::Error;
}

Предупреждение PVS-Studio: V696 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. Check lines: 1642, 1649. ParseTentative.cpp 1642

Разработчики LLVM, конечно, сразу смогут понять, есть здесь ошибка или нет. Мне же придётся поиграть в детектива. Рассматривая этот код, я рассуждал следующим образом. Функция должна прочитать открывающуюся скобку '<', затем она в цикле читает идентификаторы и запятые. Если запятой нет, то ожидается закрывающаяся скобка. Если что-то пошло не так, то функция возвращает код ошибки. Я думаю, что был задуман следующий алгоритм работы функции (псевдокод):

• Начало цикла:
• Прочитать идентификатор. Если это не идентификатор, то вернуть статус ошибки.
• Прочитать запятую. Если это запятая, перейти к началу цикла.
• Ага, у нас не запятая. Если это закрывающаяся скобка, то все хорошо, выходим из функции.
• Иначе вернем статус ошибки.

Беда в том, что цикл пытаются возобновить с помощью оператора continue. Он передает управление вовсе не на начало тела цикла, а на проверку условия продолжения цикла. А условие у всегда false. В результате цикл сразу завершается и алгоритм выглядит следующим образом:

• Начало цикла:
• Прочитать идентификатор. Если это не идентификатор, то вернуть статус ошибки.
• Прочитать запятую. Если это запятая, завершить цикл и вернуть из функции статус ошибки.
• Ага, у нас не запятая. Если это закрывающаяся скобка, то все хорошо, выходим из функции.
• Иначе вернем статус ошибки.

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

Рассмотрим теперь другой случай, когда выполняется не более, чем 1 итерация цикла:

static bool checkMachOAndArchFlags(....) {
  ....
  unsigned i;
  for (i = 0; i < ArchFlags.size(); ++i) {
    if (ArchFlags[i] == T.getArchName())
      ArchFound = true;
    break;
  }
  ....
}

Предупреждение PVS-Studio: V612 An unconditional 'break' within a loop. MachODump.cpp 1206

Обратите внимание на оператор break. Он прервёт цикл сразу после первой итерации. Мне кажется, оператор break должен относиться к условию, и тогда корректный код станет выглядеть так:

for (i = 0; i < ArchFlags.size(); ++i) {
  if (ArchFlags[i] == T.getArchName())
  {
    ArchFound = true;
    break;
  }
}

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

• V612 An unconditional 'return' within a loop. R600OptimizeVectorRegisters.cpp 54
• V612 An unconditional 'break' within a loop. llvm-size.cpp 525

Перепутан оператор || и &&

static bool containsNoDependence(CharMatrix &DepMatrix,
                                 unsigned Row,
                                 unsigned Column) {
  for (unsigned i = 0; i < Column; ++i) {
    if (DepMatrix[Row][i] != '=' || DepMatrix[Row][i] != 'S' ||
        DepMatrix[Row][i] != 'I')
      return false;
  }
  return true;
}

Предупреждение PVS-Studio: V547 Expression is always true. Probably the '&&' operator should be used here. LoopInterchange.cpp 208

Выражение не имеет смысла. Я упрощу код, чтобы выделить суть ошибки:

if (X != '=' || X != 'S' || X != 'I')

Переменная X всегда будет чему-то не равна. В результате условие всегда истинно. Скорее всего, вместо операторов »||» следовало использовать операторы »&&», тогда выражение приобретает смысл.

Функция возвращает ссылку на локальный объект

SingleLinkedListIterator &operator++(int) {
  SingleLinkedListIterator res = *this;
  ++*this;
  return res;
}

Предупреждение PVS-Studio: V558 Function returns the reference to temporary local object: res. LiveInterval.h 679

Функция представляет традиционную реализацию постфиксного инкремента:

• Текущее состояние сохраняется во временном объекте;
• Изменяется текущее состояние объекта;
• Возвращается старое состояние объекта.

Ошибка в том, что функция возвращает ссылку. Эта ссылка не валидна, так как при выходе из функции временный объект res будет разрушен.

Чтобы исправить ситуацию, нужно возвращать значение, а не ссылку:

SingleLinkedListIterator operator++(int) { .... }

Повторное присваивание

Приведу функцию целиком, чтобы никто не подумал, что перед повторным присваиванием переменная ZeroDirective как-то используется.

HexagonMCAsmInfo::HexagonMCAsmInfo(const Triple &TT) {
  Data16bitsDirective = "\t.half\t";
  Data32bitsDirective = "\t.word\t";
  Data64bitsDirective = nullptr;
  ZeroDirective = "\t.skip\t";                            // <=
  CommentString = "//";

  LCOMMDirectiveAlignmentType = LCOMM::ByteAlignment;
  InlineAsmStart = "# InlineAsm Start";
  InlineAsmEnd = "# InlineAsm End";
  ZeroDirective = "\t.space\t";                           // <=
  AscizDirective = "\t.string\t";

  SupportsDebugInformation = true;
  MinInstAlignment = 4;
  UsesELFSectionDirectiveForBSS  = true;
  ExceptionsType = ExceptionHandling::DwarfCFI;
}

Предупреждение PVS-Studio: V519 The 'ZeroDirective' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 25, 31. HexagonMCAsmInfo.cpp 31

Переменная ZeroDirective представляет собой простой указатель типа const char *. В начале он указывает на строку »\t.skip\t», а чуть ниже ему назначают адрес строки »\t.space\t». Это странно и не имеет смысла. Высока вероятность, что одно из присваиваний должно изменять совсем другую переменную.

Рассмотрим ещё один случай повторного присваивания.

template 
void GNUStyle::printFileHeaders(const ELFO *Obj) {
  ....
  Str = printEnum(e->e_ident[ELF::EI_OSABI], makeArrayRef(ElfOSABI));
  printFields(OS, "OS/ABI:", Str);
  Str = "0x" + to_hexString(e->e_version);                  // <=
  Str = to_hexString(e->e_ident[ELF::EI_ABIVERSION]);       // <=
  printFields(OS, "ABI Version:", Str);
  Str = printEnum(e->e_type, makeArrayRef(ElfObjectFileType));
  printFields(OS, "Type:", Str);
  ....
}

Предупреждение PVS-Studio: V519 The 'Str' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2407, 2408. ELFDumper.cpp 2408

По всей видимости мы имеем дело с опечаткой. Вместо повторного присваивания надо было конкатенировать две строки с помощью оператора +=. Тогда корректный код должен выглядеть так:

Str = "0x" + to_hexString(e->e_version);
Str += to_hexString(e->e_ident[ELF::EI_ABIVERSION]);

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

• V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 55, 57. coff2yaml.cpp 57
• V519 The 'O' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 394, 395. llvm-pdbdump.cpp 395
• V519 The 'servAddr.sin_family' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 63, 64. server.cpp 64

Подозрительная работа с умными указателями

Expected>
PDBFileBuilder::build(
  std::unique_ptr PdbFileBuffer)
{
  ....
  auto File = llvm::make_unique(
    std::move(PdbFileBuffer), Allocator);

  File->ContainerLayout = *ExpectedLayout;

  if (Info) {
    auto ExpectedInfo = Info->build(*File, *PdbFileBuffer);
  ....
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'PdbFileBuffer' might take place. PDBFileBuilder.cpp 106

Код мне не понятен, так как, например, я не изучал, что такое llvm: make_unique и как вообще всё это работает. Тем не менее, анализатор и меня настораживает то, что на первый взгляд владение объектом от умного указателя PdbFileBuffer переходит к File. После чего происходит разыменование умного указателя PdbFileBuffer, который, по идее, в этот момент уже содержит внутри себя nullptr. То есть настораживает следующее:

.... llvm::make_unique(::move(PdbFileBuffer), Allocator);
....
.... Info->build(*File, *PdbFileBuffer);

Если это ошибка, то её следует поправить ещё в 3 местах в этом же файле:

• V522 Dereferencing of the null pointer 'PdbFileBuffer' might take place. PDBFileBuilder.cpp 113
• V522 Dereferencing of the null pointer 'PdbFileBuffer' might take place. PDBFileBuilder.cpp 120
• V522 Dereferencing of the null pointer 'PdbFileBuffer' might take place. PDBFileBuilder.cpp 127

Опечатка в условии

static bool areExclusiveRanges(BinaryOperatorKind OpcodeLHS,
                               const APSInt &ValueLHS,
                               BinaryOperatorKind OpcodeRHS,
                               const APSInt &ValueRHS) {
  ....
  // Handle cases where the constants are different.
  if ((OpcodeLHS == BO_EQ ||
       OpcodeLHS == BO_LE ||                 // <=
       OpcodeLHS == BO_LE)                   // <=
      &&
      (OpcodeRHS == BO_EQ ||
       OpcodeRHS == BO_GT ||
       OpcodeRHS == BO_GE))
    return true;
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'OpcodeLHS == BO_LE' to the left and to the right of the '||' operator. RedundantExpressionCheck.cpp 174

Это классическая опечатка. Переменная OpcodeLHS дважды сравнивается с константой BO_LE. Как мне кажется, одну из констант BO_LE следует заменить на BO_LT. Как видите имена констант схожи между собой и несложно спутать.

Следующий пример демонстрирует, как статический анализ дополняет другие методологии написания качественного кода. Рассмотрим ошибочный код:

std::pair
llvm::createSanitizerCtorAndInitFunctions(
    ....
    ArrayRef InitArgTypes, ArrayRef InitArgs,
    ....)
{
  assert(!InitName.empty() && "Expected init function name");
  assert(InitArgTypes.size() == InitArgTypes.size() &&
    "Sanitizer's init function expects "
    "different number of arguments");
  ....

}

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'InitArgTypes.size ()' to the left and to the right of the '==' operator. ModuleUtils.cpp 107

Одним из хороших способов повысить надежность кода является использование макросов assert (). Этот и аналогичные макросы помогают выявить многие ошибки на этапе разработки и отладки программы. Однако не буду вдаваться в подробные описания пользы, приносимых такими макросами, так как это выходит за рамки статьи.

Нам важно то, что в функции createSanitizerCtorAndInitFunctions () используются макросы assert () для проверки корректности входных значений. Вот только из-за опечатки второй assert () бесполезен.

К счастью, нам помогает статический анализатор, который замечает, что размер массива сравнивается сам с собой. В результате мы можем исправить проверку, а правильное условие в assert () со временем может помочь предотвратить какую-то другую ошибку.

По всей видимости, в условии должны сравниваться размеры массивов InitArgTypes и InitArgs:

assert(InitArgTypes.size() == InitArgs.size() &&
  "Sanitizer's init function expects "
  "different number of arguments");

Путаница между release () и reset ()

В классе std: unique_ptr есть две созвучные функции: release и reset. Как показывают мои наблюдения, иногда их путают. Видимо это произошло и здесь:

std::unique_ptr takeClient()
  { return std::move(Owner); }

VerifyDiagnosticConsumer::~VerifyDiagnosticConsumer() {
  ....
  SrcManager = nullptr;
  CheckDiagnostics();
  Diags.takeClient().release();
}

Предупреждение PVS-Studio: V530 The return value of function 'release' is required to be utilized. VerifyDiagnosticConsumer.cpp 46

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

Избыточные условия

bool ARMDAGToDAGISel::tryT1IndexedLoad(SDNode *N) {
  LoadSDNode *LD = cast(N);
  EVT LoadedVT = LD->getMemoryVT();
  ISD::MemIndexedMode AM = LD->getAddressingMode();
  if (AM == ISD::UNINDEXED ||
      LD->getExtensionType() != ISD::NON_EXTLOAD ||
      AM != ISD::POST_INC ||
      LoadedVT.getSimpleVT().SimpleTy != MVT::i32)
    return false;
  ....
}

Предупреждение PVS-Studio: V590 Consider inspecting this expression. The expression is excessive or contains a misprint. ARMISelDAGToDAG.cpp 1565

Условие длинное, поэтому я выделю самое главное:

AM == ISD::UNINDEXED || AM != ISD::POST_INC

Это условие избыточно и его можно упросить до:

AM != ISD::POST_INC

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

• V590 Consider inspecting this expression. The expression is excessive or contains a misprint. ASTReader.cpp 4178
• V590 Consider inspecting this expression. The expression is excessive or contains a misprint. BracesAroundStatementsCheck.cpp 46

Мои любимые предупреждения V595

Указатели в C и C++ — бесконечная головная боль программистов. Проверяешь их на ноль, проверяешь, а где-то — раз! — и опять разыменование нулевого указателя. Диагностика V595 выявляет ситуации, когда проверка указателя на равенство нулю происходит слишком поздно. До этой проверки указатель уже успевают использовать. Это одна из самых типовых ошибок, находимых нами в коде разнообразнейших приложений (доказательство). Впрочем, в защиту C/C++ скажу, что в C# ситуация не намного лучше. От того, что указатели в C# назвали ссылками, такие ошибки не пропали (доказательство).

Вернемся к коду LLVM и рассмотрим простой вариант ошибки:

bool PPCDarwinAsmPrinter::doFinalization(Module &M) {
  ....
  MachineModuleInfoMachO &MMIMacho =
      MMI->getObjFileInfo();

  if (MAI->doesSupportExceptionHandling() && MMI) {
  ....
}

Предупреждение PVS-Studio: V595 The 'MMI' pointer was utilized before it was verified against nullptr. Check lines: 1357, 1359. PPCAsmPrinter.cpp 1357

Случай простой и всё сразу видно. Проверка (… && MMI) говорит нам, что указатель MMI может быть равен нулю. Если это так, поток выполнения программы не доберётся до этой проверки. Он будет прерван раньше из-за разыменования нулевого указателя.

Рассмотрим ещё один фрагмент кода:

void Sema::CodeCompleteObjCProtocolReferences(
  ArrayRef Protocols)
{
  ResultBuilder 
    Results(*this, CodeCompleter->getAllocator(),
            CodeCompleter->getCodeCompletionTUInfo(),
            CodeCompletionContext::CCC_ObjCProtocolName);
  
  if (CodeCompleter && CodeCompleter->includeGlobals()) {
    Results.EnterNewScope();
  ....
}

Предупреждение PVS-Studio: V595 The 'CodeCompleter' pointer was utilized before it was verified against nullptr. Check lines: 5952, 5955. SemaCodeComplete.cpp 5952

Указатель CodeCompleter сначала разыменовывается, а уже ниже располагается проверка на равенства этого указателя нулю. Такой же код ещё трижды встречается в этом же файле:

• V595 The 'CodeCompleter' pointer was utilized before it was verified against nullptr. Check lines: 5980, 5983. SemaCodeComplete.cpp 5980
• V595 The 'CodeCompleter' pointer was utilized before it was verified against nullptr. Check lines: 7455, 7458. SemaCodeComplete.cpp 7455
• V595 The 'CodeCompleter' pointer was utilized before it was verified against nullptr. Check lines: 7483, 7486. SemaCodeComplete.cpp 7483

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

• V595 The 'Receiver' pointer was utilized before it was verified against nullptr. Check lines: 2543, 2560. SemaExprObjC.cpp 2543
• V595 The 'S' pointer was utilized before it was verified against nullptr. Check lines: 1267, 1296. SemaLookup.cpp 1267
• V595 The 'TargetDecl' pointer was utilized before it was verified against nullptr. Check lines: 4037, 4046. CGExpr.cpp 4037
• V595 The 'CurrentToken' pointer was utilized before it was verified against nullptr. Check lines: 705, 708. TokenAnnotator.cpp 705
• V595 The 'FT' pointer was utilized before it was verified against nullptr. Check lines: 540, 554. Expr.cpp 540
• V595 The 'II' pointer was utilized before it was verified against nullptr. Check lines: 448, 450. IdentifierTable.cpp 448
• V595 The 'MF' pointer was utilized before it was verified against nullptr. Check lines: 268, 274. X86RegisterInfo.cpp 268
• V595 The 'External' pointer was utilized before it was verified against nullptr. Check lines: 40, 45. HeaderSearch.cpp 40
• V595 The 'TLI' pointer was utilized before it was verified against nullptr. Check lines: 4239, 4244. CodeGenPrepare.cpp 4239
• V595 The 'SU→getNode ()' pointer was utilized before it was verified against nullptr. Check lines: 292, 297. ResourcePriorityQueue.cpp 292
• V595 The 'BO0' pointer was utilized before it was verified against nullptr. Check lines: 2835, 2861. InstCombineCompares.cpp 2835
• V595 The 'Ret' pointer was utilized before it was verified against nullptr. Check lines: 2090, 2092. ObjCARCOpts.cpp 2090

Странный код

Прошу прощения, что привожу тяжелый для чтения фрагмент кода. Потерпите, до конца статьи осталось немного.

static bool print_class_ro64_t(....) {
  ....
  const char *r;
  uint32_t offset, xoffset, left;
  ....
  r = get_pointer_64(p, offset, left, S, info);
  if (r == nullptr || left < sizeof(struct class_ro64_t))
    return false;
  memset(&cro, '\0', sizeof(struct class_ro64_t));
  if (left < sizeof(struct class_ro64_t)) {
    memcpy(&cro, r, left);
    outs() << "   (class_ro_t entends past the .......)\n";
  } else
    memcpy(&cro, r, sizeof(struct class_ro64_t));
  ....
}

Предупреждение PVS-Studio: V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 4410, 4413. MachODump.cpp 4413

Обратите внимание на проверку:

if (.... || left < sizeof(struct class_ro64_t))
  return false;

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

if (left < sizeof(struct class_ro64_t)) {
  memcpy(&cro, r, left);
  outs() << "   (class_ro_t entends past the .......)\n";
} else
  memcpy(&cro, r, sizeof(struct class_ro64_t));

Условие всегда ложно, а, следовательно, всегда выполняется else-ветвь. Это очень странно. Возможно, программа содержит логическую ошибку, или мы имеем дело с кокой-то опечаткой.

Заодно следует проверить вот это место:

• V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 4612, 4615. MachODump.cpp 4615

Разная мелочь

Внутри шаблонного класса RPC объявлен класс SequenceNumberManager. В нём есть вот такой перемещающий оператор присваивания (move assignment operator):

SequenceNumberManager &operator=(SequenceNumberManager &&Other) {
  NextSequenceNumber = std::move(Other.NextSequenceNumber);
  FreeSequenceNumbers = std::move(Other.FreeSequenceNumbers);
}

Предупреждение PVS-Studio: V591 Non-void function should return a value. RPCUtils.h 719

Как видите в конце забыли написать return:

return *this;

На самом деле в этом нет ничего страшного. Компиляторы, как правило, никак не работают с телами функций шаблонных классов, если эти функции не используются. Здесь, видимо, именно такой случай. Хотя я не проверял, но я уверен: если вызвать такой оператор перемещения, компилятор выдаст ошибку компиляции или громкий warning. Так что ничего страшного здесь нет, но решил указать на эту недоработку.

Встретилось несколько странных участков кода, где значение указателя, который вернул оператор new, проверяется на равенство нулю. Этот код не имеет смысла, так как если память не удастся выделить, должно быть сгенерировано исключение std: bad_alloc. Вот одно из таких мест:

LLVMDisasmContextRef LLVMCreateDisasmCPUFeatures(....) {
  ....
  // Set up the MCContext for creating symbols and MCExpr's.
  MCContext *Ctx = new MCContext(MAI, MRI, nullptr);
  if (!Ctx)
    return nullptr;
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the 'Ctx' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. Disassembler.cpp 76

И ещё 2 предупреждения:

• V668 There is no sense in testing the 'DC' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. Disassembler.cpp 103
• V668 There is no sense in testing the 'JITCodeEntry' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. GDBRegistrationListener.cpp 180

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

Заключение

Как видите, предупреждения компиляторов — это хорошо, но недостаточно. Специализированные инструменты статического анализа, такие как PVS-Studio, всегда будут опережать компиляторы в диагностических возможностях и гибкости настройки при работе с предупреждениями. Собственно, на этом разработчики анализаторов и зарабатывают свои деньги.

Ещё важно отметить, что основной эффект применения методологии статического анализа достигается при регулярном использовании статических анализаторов кода. Многие ошибки будут выявлены на самом раннем этапе, и их не потребуется отлаживать или упрашивать пользователя подробно описать последовательность действий, приводящих к падению программы. Здесь полная аналогия с предупреждениями компилятора (собственно, это те же самые предупреждения, но более интеллектуальные). Вы ведь смотрите предупреждения компилятора постоянно, а не раз в месяц?!

Приглашаем скачать и попробовать PVS-Studio на коде своего проекта.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey karpov. Finding bugs in the code of LLVM project with the help of PVS-Studio.

Комментарии (0)