Знаходимо помилки в коді проекту LLVM з допомогою аналізатора PVS-Studio

PVS-Studio vs LLVMБлизько двох місяців тому я написав статтю про перевірки компілятора 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 зовсім не складно, і ви можете, не відкладаючи, спробувати перевірити власний проект.

Пінгвін на єдинорога PVS-Studio

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. У ньому зберігається список викликів компілятора в наступному вигляді:
[
{
"fs": "/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
};

Це, якщо так можна сказати, «класичне перерахування». Кожному імені в перерахуванні присвоюється цілочисельне значення, яке відповідає певним місцем в порядку значень у перерахуванні:
  • 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<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]);

Є ще кілька фрагментів коду, де відбувається повторне призначення. На мій погляд, ці повторні присвоювання не несуть у собі ніякої небезпеки, тому я просто перерахую відповідні попередження:
  • 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<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 місцях в цьому ж файлі:
  • 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<Function *, Function *>
llvm::createSanitizerCtorAndInitFunctions(
....
ArrayRef<Type *> InitArgTypes, ArrayRef<Value *> InitArgs,
....)
{
assert(!InitName.empty() && "Expected init function name");
assert(InitArgTypes.size() == InitArgTypes.size() &&
"Sanitizer's function init 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 function init expects "
"different number of arguments");

Плутанина між release() і reset()
У класі 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 попередження аналізатора:
  • 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<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 спочатку разыменовывается, а вже нижче розташовується перевірка на рівності цього покажчика нулю. Такий же код ще тричі зустрічається в цьому ж файлі:
  • 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 the past .......)\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 the past .......)\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 symbols for creating and MCExpr's.
MCContext *Ctx = new MCContext(MAI, MRI, nullptr);
if (!Ctx)
return nullptr;
....
}

Попередження PVS-Studio: V668 There is no sense testing in 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 testing in 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 testing in 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.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.