Около двух месяцев назад я написал статью о проверке компилятора GCC с помощью анализатора PVS-Studio. Идея статьи была следующая: предупреждения GCC - это хорошо, но недостаточно. Надо использовать специализированные инструменты анализа кода, например, PVS-Studio. В качестве подтверждения я показал ошибки, которые PVS-Studio смог найти в коде GCC. Ряд читателей заметили, что качество кода GCC и его диагностики так себе, в то время как компилятор Clang современен, качественен, свеж и молод. В общем Clang - это ого-го! Что ж, значит пришло время мне проверить с помощью PVS-Studio проект LLVM.
Думаю, мало тех, кто не знает, что такое 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 для Linux, и мне удавалась работать над статьёй только урывками.
Всё, лирика закончилась, перейду к самому интересному. Рассмотрим подозрительные места в коде LLVM, на которые указал мне PVS-Studio.
В коде имеется вот такое перечисление:
enum Type {
ST_Unknown, // Type not specified
ST_Data,
ST_Debug,
ST_File,
ST_Function,
ST_Other
};Это, если так можно сказать, "классическое перечисление". Каждому имени в перечислении присваивается целочисленное значение, которое соответствует определенному месту в порядке значений в перечислении:
Ещё раз подчеркну, что это просто перечисление, а не набор масок. Если бы эти константы можно было сочетать между собой, то они бы являлись степенью числа 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;
}
}Есть ещё два аналогичных места, но, чтобы статья не получилась слишком большой, приведу только предупреждения анализатора:
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<T> &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<T> 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 <class ELFT>
void GNUStyle<ELFT>::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]);Есть еще несколько фрагментов кода, где происходит повторное присваивание. На мой взгляд, эти повторные присваивания не несут в себе никакой опасности, поэтому я просто перечислю соответствующие предупреждения:
Expected<std::unique_ptr<PDBFile>>
PDBFileBuilder::build(
std::unique_ptr<msf::WritableStream> PdbFileBuffer)
{
....
auto File = llvm::make_unique<PDBFile>(
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<PDBFile>(::move(PdbFileBuffer), Allocator);
....
.... Info->build(*File, *PdbFileBuffer);Если это ошибка, то её следует поправить ещё в 3 местах в этом же файле:
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<Function *, Function *>
llvm::createSanitizerCtorAndInitFunctions(
....
ArrayRef<Type *> InitArgTypes, ArrayRef<Value *> 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");В классе std::unique_ptr есть две созвучные функции: release и reset. Как показывают мои наблюдения, иногда их путают. Видимо это произошло и здесь:
std::unique_ptr<DiagnosticConsumer> 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<LoadSDNode>(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 предупреждения анализатора:
Указатели в C и C++ - бесконечная головная боль программистов. Проверяешь их на ноль, проверяешь, а где-то - раз! - и опять разыменование нулевого указателя. Диагностика V595 выявляет ситуации, когда проверка указателя на равенство нулю происходит слишком поздно. До этой проверки указатель уже успевают использовать. Это одна из самых типовых ошибок, находимых нами в коде разнообразнейших приложений (доказательство). Впрочем, в защиту C/C++ скажу, что в C# ситуация не намного лучше. От того, что указатели в C# назвали ссылками, такие ошибки не пропали (доказательство).
Вернемся к коду LLVM и рассмотрим простой вариант ошибки:
bool PPCDarwinAsmPrinter::doFinalization(Module &M) {
....
MachineModuleInfoMachO &MMIMacho =
MMI->getObjFileInfo<MachineModuleInfoMachO>();
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<IdentifierLocPair> 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 сначала разыменовывается, а уже ниже располагается проверка на равенства этого указателя нулю. Такой же код ещё трижды встречается в этом же файле:
Это были простые случаи, но встречается и более запутанный код, где я сходу не могу сказать насколько он опасен. Поэтому предлагаю разработчикам самостоятельно проверить следующие участки кода LLVM:
Прошу прощения, что привожу тяжелый для чтения фрагмент кода. Потерпите, до конца статьи осталось немного.
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-ветвь. Это очень странно. Возможно, программа содержит логическую ошибку, или мы имеем дело с какой-то опечаткой.
Заодно следует проверить вот это место:
Внутри шаблонного класса 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 предупреждения:
Эту участки кода не выглядят опасными, поэтому я решил описать их в разделе несущественных предупреждений. Скорее всего, все эти три проверки можно просто удалить.
Как видите, предупреждения компиляторов — это хорошо, но недостаточно. Специализированные инструменты статического анализа, такие как PVS-Studio, всегда будут опережать компиляторы в диагностических возможностях и гибкости настройки при работе с предупреждениями. Собственно, на этом разработчики анализаторов и зарабатывают свои деньги.
Ещё важно отметить, что основной эффект применения методологии статического анализа достигается при регулярном использовании статических анализаторов кода. Многие ошибки будут выявлены на самом раннем этапе, и их не потребуется отлаживать или упрашивать пользователя подробно описать последовательность действий, приводящих к падению программы. Здесь полная аналогия с предупреждениями компилятора (собственно, это те же самые предупреждения, но более интеллектуальные). Вы ведь смотрите предупреждения компилятора постоянно, а не раз в месяц?!
Приглашаем скачать и попробовать PVS-Studio на коде своего проекта.