Статический анализатор PVS-Studio способен находить ошибки даже в таком качественном и протестированном проекте, как LLVM. Чтобы это не было пустыми словами, мы время от времени перепроверяем проект и публикуем такие заметки, как эта.
LLVM — проект программной инфраструктуры для создания компиляторов и сопутствующих им утилит. Состоит из набора компиляторов языков высокого уровня, системы оптимизации, интерпретации и компиляции в машинный код. Написан на современной версии языка C++. Проект используется огромным количеством пользователей и хорошо протестирован.
Время от времени мы проверяем этот проект, чтобы продемонстрировать возможности PVS-Studio. Предыдущие публикации:
Эти статьи хорошо демонстрируют пользу методологии статического анализа кода. Однако это не имеет ничего общего с повышением качества и надёжности программного проекта. Статические анализаторы должны использоваться регулярно в процессе разработки, чтобы помогать выявлять ошибки на ранних этапах.
Собственно, LLVM, если я правильно понимаю, уже регулярно проверяется с помощью Coverity Scan Static Analysis и Clang Static Analyzer. Другое дело, что хорошо бы ещё и PVS-Studio добавить :)
При написании статьи хотелось показать интересные ошибки, но не ставилась цель найти их как можно больше. Изучать предупреждения, выданные на незнакомый код, тяжело, поэтому я решил остановиться, выписав 19 ошибок. Решил, что это подходящее число для 19 версии проекта. Естественно, обнаружить можно гораздо больше ошибок.
Для начала взглянем на прототип функции isStoreUsed. Обратите внимание, что последний аргумент является опциональным.
bool isStoreUsed(const FrameIndexEntry &StoreFIE, ExprIterator Candidates,
bool IncludeLocalAccesses = true) const;Есть места, где эта функция используется правильно:
if (SRU.isStoreUsed(*FIEX,
Prev ? SRU.expr_begin(*Prev) : SRU.expr_begin(BB),
/*IncludeLocalAccesses=*/false)) {Но в одном месте рука программиста дрогнула, и скобка оказалась не там, где надо. Что ж, никто не застрахован от таких ошибок.
void CalleeSavedAnalysis::analyzeSaves() {
....
// If this stack position is accessed in another function, we are
// probably dealing with a parameter passed in a stack -- do not mess
// with it
if (SRU.isStoreUsed(*FIE,
Prev ? SRU.expr_begin(*Prev) : SRU.expr_begin(BB)),
/*IncludeLocalAccesses=*/false) {
BlacklistedRegs.set(FIE->RegOrImm);
CalleeSaved.reset(FIE->RegOrImm);
Prev = &Inst;
continue;
}
....
}Анализатор PVS-Studio выдаёт на это сразу два предупреждения:
Красивая ошибка. Код компилируется по той причине, что последний аргумент функции опционален. По сути, он эквивалентен этому:
SRU.isStoreUsed(*FIE,
Prev ? SRU.expr_begin(*Prev) : SRU.expr_begin(BB));
if (/*IncludeLocalAccesses=*/false) {Заметить такую ошибку на обзоре кода не так просто. Анализатор PVS-Studio может быть хорошим помощником в высматривании подобных опечаток.
FailureOr<ConstantOrScalableBound>
ScalableValueBoundsConstraintSet::computeScalableBound(....)
{
....
AffineMap bound = [&] {
if (boundType == BoundType::EQ && !invalidBound(lowerBound) &&
lowerBound[0] == lowerBound[0]) { // <=
return lowerBound[0];
} else if (boundType == BoundType::LB && !invalidBound(lowerBound)) {
return lowerBound[0];
} else if (boundType == BoundType::UB && !invalidBound(upperBound)) {
return upperBound[0];
}
return AffineMap{};
}();
....
}Предупреждение PVS-Studio:
V501 There are identical sub-expressions to the left and to the right of the '==' operator: lowerBound[0] == lowerBound[0] ScalableValueBoundsConstraintSet.cpp 110
Простая опечатка при работе со схожими именами массивов. Даже не знаю, что ещё про неё написать, так что перейдём к следующей.
Для тех, кто не читал, загляните в "Ноль, один, два, Фредди заберёт тебя".
Value buildTernaryFn(TernaryFn ternaryFn, Value arg0, Value arg1,
Value arg2) {
bool headBool =
isInteger(arg0) && arg0.getType().getIntOrFloatBitWidth() == 1;
bool tailFloatingPoint =
isFloatingPoint(arg0) && isFloatingPoint(arg1) && isFloatingPoint(arg2);
bool tailInteger = isInteger(arg0) && isInteger(arg1) && isInteger(arg1);
OpBuilder::InsertionGuard g(builder);
builder.setInsertionPointToEnd(&block);
....
}Предупреждение PVS-Studio:
V501 There are identical sub-expressions 'isInteger(arg1)' to the left and to the right of the '&&' operator. LinalgOps.cpp 502
В последнем вызове функции повторно используется arg1:
bool tailInteger = isInteger(arg0) && isInteger(arg1) && isInteger(arg1);Код читается тяжело, что и является причиной опечатки. Возможно, ошибку удастся избежать, если написать код более "разрежено" и выровнено:
bool tailFloatingPoint = isFloatingPoint(arg0)
&& isFloatingPoint(arg1)
&& isFloatingPoint(arg2);
bool tailInteger = isInteger(arg0)
&& isInteger(arg1)
&& isInteger(arg1);В случае табличного форматирования ошибку будет проще заметить во время обзора кода.
StringRef ARMAsmParser::splitMnemonic(StringRef Mnemonic, ....) {
....
if (isMnemonicVPTPredicable(Mnemonic, ExtraToken) && Mnemonic != "vmovlt" &&
Mnemonic != "vshllt" && Mnemonic != "vrshrnt" && Mnemonic != "vshrnt" &&
Mnemonic != "vqrshrunt" && Mnemonic != "vqshrunt" &&
Mnemonic != "vqrshrnt" && Mnemonic != "vqshrnt" && Mnemonic != "vmullt" &&
Mnemonic != "vqmovnt" /*** <= ***/ && Mnemonic != "vqmovunt" &&
Mnemonic != "vqmovnt" /*** <= ***/ && Mnemonic != "vmovnt" &&
Mnemonic != "vqdmullt" && Mnemonic != "vpnot" &&
Mnemonic != "vcvtt" && Mnemonic != "vcvt") {
....
}Предупреждение PVS-Studio:
V501 There are identical sub-expressions 'Mnemonic != "vqmovnt"' to the left and to the right of the '&&' operator. ARMAsmParser.cpp 6633
template <typename T>
LIBC_NAMESPACE::cpp::enable_if_t<LIBC_NAMESPACE::cpp::is_floating_point_v<T>,
bool>
ValuesEqual(T x1, T x2) {
LIBC_NAMESPACE::fputil::FPBits<T> bits1(x1);
LIBC_NAMESPACE::fputil::FPBits<T> bits2(x2);
// If either is NaN, we want both to be NaN.
if (bits1.is_nan() || bits2.is_nan())
return bits2.is_nan() && bits2.is_nan(); // <=
// For all other values, we want the values to be bitwise equal.
return bits1.uintval() == bits2.uintval();
}Предупреждение PVS-Studio:
V501 There are identical sub-expressions to the left and to the right of the '&&' operator: bits2.is_nan() && bits2.is_nan() Compare.h 23
bool OpenACCClauseWithVarList::classof(const OpenACCClause *C) {
return OpenACCPrivateClause::classof(C) ||
OpenACCFirstPrivateClause::classof(C) ||
OpenACCDevicePtrClause::classof(C) || // <=
OpenACCDevicePtrClause::classof(C) || // <=
OpenACCAttachClause::classof(C) || OpenACCNoCreateClause::classof(C) ||
OpenACCPresentClause::classof(C) || OpenACCCopyClause::classof(C) ||
OpenACCCopyInClause::classof(C) || OpenACCCopyOutClause::classof(C) ||
OpenACCReductionClause::classof(C) || OpenACCCreateClause::classof(C);
}Предупреждение PVS-Studio:
V501 There are identical sub-expressions 'OpenACCDevicePtrClause::classof(C)' to the left and to the right of the '||' operator. OpenACCClause.cpp 31
Вроде всё просто, когда эти ошибки уже найдены. Но при этом до анализатора их никто не заметил. Любите статический анализ кода!
isl_size isl_local_space_var_offset(....)
{
isl_space *space;
space = isl_local_space_peek_space(ls);
if (space < 0)
return isl_size_error;
....
}Предупреждение PVS-Studio:
V503 This is a nonsensical comparison: pointer < 0. isl_local_space.c 257
Проверка указателя на то, что он меньше 0, не имеет смысла. Поведение такой операции не уточнено. Здесь должно быть или сравнение на равенство nullptr, или вообще другая проверка.
static Expected<uint64_t>
GeneratePerfEventConfigValue(bool enable_tsc,
std::optional<uint64_t> psb_period) {
uint64_t config = 0;
....
if (Expected<uint32_t> offset = ReadIntelPTConfigFile(
kTSCBitOffsetFile, IntelPTConfigFileType::BitOffset))
config |= 1 << *offset;
....
}Предупреждение PVS-Studio:
V629 Consider inspecting the '1 << * offset' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. IntelPTSingleBufferTrace.cpp 153
Анализатор выдал ряд предупреждений V629 на выражения, где сдвигается значение типа int, а результат используется совместно с 64-битными значениями. Приведённый выше фрагмент кода, как и другие места, на которые указал анализатор, необязательно ошибочны, но очень подозрительны.
Маска формируется в 64-битной переменной config. Но при этом сдвигается литерал 1, который имеет тип int. Следовательно, не удастся корректно выставить старшие биты в config, если это понадобится. Возможно, этот код работает, так как старшие биты пока выставлять не нужно, но в любом случае правильнее будет написать так:
config |= (uint64_t)(1) << *offset;
// или так
config |= 1ULL << *offset;std::unique_ptr<OptionDefinition> m_options_definition_up;
Status SetOptionsFromArray(StructuredData::Dictionary &options) {
Status error;
m_num_options = options.GetSize();
m_options_definition_up.reset(new OptionDefinition[m_num_options]);
....
}Предупреждение PVS-Studio:
V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. CommandObjectCommands.cpp 1384
Умный указатель m_options_definition_up ориентирован на хранение одиночного элемента, а туда помещается указатель на массив. Как результат — неопределённое поведение в момент разрушения объекта. Подробнее здесь "Почему в С++ массивы нужно удалять через delete[]".
Чтобы исправить баг, достаточно добавить [] при объявлении умного указателя:
std::unique_ptr<OptionDefinition []> m_options_definition_up;std::vector<lldb::addr_t> m_indexed_isa_cache;
bool AppleObjCRuntimeV2::NonPointerISACache::EvaluateNonPointerISA(
ObjCISA isa, ObjCISA &ret_isa) {
....
uintptr_t index = (isa & m_objc_debug_indexed_isa_index_mask) >>
m_objc_debug_indexed_isa_index_shift;
....
// If the index is still out of range then this isn't a pointer.
if (index > m_indexed_isa_cache.size())
return false;
LLDB_LOGF(log, "AOCRT::NPI Evaluate(ret_isa = 0x%" PRIx64 ")",
(uint64_t)m_indexed_isa_cache[index]);
ret_isa = m_indexed_isa_cache[index];
....
}Предупреждения PVS-Studio:
Классический антипаттерн: использование неправильного оператора для проверки, что индекс не выходит за границу массива. Выполнение функции прерывается, если индекс больше размера массива:
if (index > m_indexed_isa_cache.size())
return false;Это неправильно, так как элементы массива нумеруются с нуля. Если окажется, что index равен m_indexed_isa_cache.size(), то произойдёт выход за границу массива (Off-by-one Error).
Правильно писать >=:
if (index >= m_indexed_isa_cache.size())
return false;Интересный момент. Поскольку я часто встречаю этот антипаттерн, то уже привык к нему и однажды чуть не просмотрел из-за этого интересную ошибку в проекте DPDK. Вернее, из-запылённости взгляда я подумал, что это как раз такой случай. На самом деле всё оказалось куда необычнее, чем обычно – "Самая красивая ошибка, которую я нашёл с помощью PVS-Studio в 2024 году".
template <typename Key, typename ElementLattice> class MapLattice {
using Container = llvm::DenseMap<Key, ElementLattice>;
Container C;
public:
....
explicit MapLattice(Container C) { C = std::move(C); }
....
}Предупреждение PVS-Studio:
V570 The 'C' variable is assigned to itself. MapLattice.h 52
Получилось неудачно, что поле класса и параметр функции имеют одинаковые имена. Как результат, конструктор не записывает в контейнер нужные данные. Варианты исправления:
explicit MapLattice(Container C) { this->C = std::move(C); }
// Или использовать другое имя переменной
explicit MapLattice(Container src) { C = std::move(src); }
// Ещё изящнее воспользоваться списком инициализации конструктора
explicit MapLattice(Container C) : C { std::move(C) } {};P.S. Коллега, читая черновик, указал, что, оказывается, в предыдущей статье мы уже описывали эту ошибку. Но она по-прежнему на месте. Жаль, но правки по мотивам наших проверок бывают небыстрыми. Иногда совсем небыстрыми :)
std::unique_ptr<Module> parseMIR(....) {
....
MachineModuleInfoWrapperPass *MMIWP =
new MachineModuleInfoWrapperPass(&TM);
if (MIR->parseMachineFunctions(*M, MMIWP->getMMI()))
return nullptr;
PM.add(MMIWP);
return M;
}Предупреждение PVS-Studio:
V773 The function was exited without releasing the 'MMIWP' pointer. A memory leak is possible. LiveIntervalTest.cpp 74
Если функция parseMachineFunctions вернёт статус ошибки, то при выходе из функции parseMIR не будет вызван delete для указателя MMIWP.
Аналогичный случай:
Value *LLParser::PerFunctionState::getVal(const std::string &Name, Type *Ty,
LocTy Loc) {
....
Value *FwdVal;
if (Ty->isLabelTy()) {
FwdVal = BasicBlock::Create(F.getContext(), Name, &F);
} else {
FwdVal = new Argument(Ty, Name);
}
if (FwdVal->getName() != Name) {
P.error(Loc, "name is too long which can result in name collisions, "
"consider making the name shorter or "
"increasing -non-global-value-max-name-size");
return nullptr;
}
ForwardRefVals[Name] = std::make_pair(FwdVal, Loc);
return FwdVal;
}Предупреждение PVS-Studio:
V773 The function was exited without releasing the 'FwdVal' pointer. A memory leak is possible. LLParser.cpp 3625
Вообще, странно видеть в LLVM ручное управление памятью. Такой код прямо-таки притягивает к себе ошибки...
Error LinuxKernelRewriter::readORCTables() {
....
MCInst *Inst = BF->getInstructionAtOffset(Offset);
if (!Inst) {
Inst = BF->getInstructionContainingOffset(Offset);
if (Inst || BC.MIB->hasAnnotation(*Inst, "AltInst")) // <=
continue;
return createStringError(
errc::executable_format_error,
"no instruction at address 0x%" PRIx64 " in .orc_unwind_ip", IP);
}
....
}Предупреждение PVS-Studio:
V522 Dereferencing of the null pointer 'Inst' might take place. LinuxKernelRewriter.cpp 583
Уже показывал эту ошибку в своём Telegram-канале (кстати, подписывайтесь :). Если дошло до вычисления правого операнда ИЛИ, то указатель BF равен nullptr, и он смело разыменовывается. Проверка обретает смысл, если заменить || на &&:
if (Inst && BC.MIB->hasAnnotation(*Inst, "AltInst"))
continue;В заголовочных файлах встречаются достаточно тяжеловесные объекты, например, такие:
static constexpr llvm::StringLiteral kNoRegister("%noreg");Предупреждение PVS-Studio:
V1043 A global object variable 'kNoRegister' is declared in the header. Multiple copies of it will be created in all translation units that include this header file. LlvmState.h 28
static const std::vector<int64_t> InstructionsMappingShape{
1, NumberOfInterferences, ModelMaxSupportedInstructionCount};Предупреждение PVS-Studio:
V1043 A global object variable 'InstructionsMappingShape' is declared in the header. Multiple copies of it will be created in all translation units that include this header file. MLRegAllocEvictAdvisor.h 82
За счёт static линковка пройдёт без ошибки, так как в каждой единице будет свой уникальный объект. Проблема в раздувании скомпилированного кода. Эти переменные будут независимо присутствовать В КАЖДОМ скомпилированном .cpp файле, куда включены эти заголовочные файлы.
Подробнее про это рассказывается в главе "Вредный совет N34. Везде нужен некий константный экземпляр класса? Для удобства объявите его в заголовочном файле" из книги про антипаттерны.
Конечно, это не совсем баг, так как код будет работать. Однако static однозначно лучше заменить на inline.
TaskData *Parent{nullptr};
TaskData *Init(TaskData *parent, int taskType) {
TaskType = taskType;
Parent = parent;
Team = Parent->Team; // <= (1)
BarrierIndex = Parent->BarrierIndex; // <= (2)
if (Parent != nullptr) { // <= (3)
Parent->RefCount++;
....
}Предупреждение PVS-Studio:
V595 The 'Parent' pointer was utilized before it was verified against nullptr. Check lines: 543, 545. ompt-tsan.cpp 543
Указатель Parent разыменовывается в (1) и (2), хотя проверка (3) подсказывает нам и анализатору, что он может быть нулевым.
Теперь случай немного сложнее, когда взаимодействуют две функции.
ScheduleDAGSDNodes *createDefaultScheduler(SelectionDAGISel *IS,
CodeGenOptLevel OptLevel) {
const TargetLowering *TLI = IS->TLI;
const TargetSubtargetInfo &ST = IS->MF->getSubtarget();
....
}Обратите внимание, что в показанной функции указатель IS разыменовывается без предварительной проверки. А теперь посмотрим, как эта функция вызывается.
struct ForceCodegenLinking {
ForceCodegenLinking() {
....
(void)llvm::createDefaultScheduler(nullptr,
llvm::CodeGenOptLevel::Default);
....
}
};Предупреждение PVS-Studio:
V522 Dereferencing of the null pointer might take place. The null pointer is passed into 'createDefaultScheduler' function. Inspect the first argument. Check lines: 'SelectionDAGISel.cpp:285', 'LinkAllCodegenComponents.h:48'. SelectionDAGISel.cpp 285
P.S. Оказывается, эту ошибку мы тоже уже описывали (фрагмент N3).
Закончим интересным случаем, когда анализатор ошибся в диагностировании происходящего, но всё равно навёл на реальную ошибку.
DiagnosedSilenceableFailure
transform::ConvertToLoopsOp::apply(transform::TransformRewriter &rewriter,
transform::TransformResults &results,
transform::TransformState &state) {
SmallVector<Operation *> loops;
for (Operation *target : state.getPayloadOps(getTarget())) {
auto tilingOp = dyn_cast<TilingInterface>(*target);
if (!target) {
DiagnosedSilenceableFailure diag =
emitSilenceableError()
<< "expected the payload to implement TilingInterface";
diag.attachNote(target->getLoc()) << "payload op";
return diag;
}
....
}Анализатор выдал аж два предупреждения, но не по делу:
Выделю суть:
auto tilingOp = dyn_cast<TilingInterface>(*target);
if (!target)
Failure(target->getLoc());Анализатор переживает, что разыменовывается нулевой указатель target. На самом деле этот указатель никогда не будет нулевым, так что условие никогда не выполнится.
Ошибка в том, что проверяется не тот указатель. Правильный вариант кода:
auto tilingOp = dyn_cast<TilingInterface>(*target);
if (!tilingOp)
Failure(target->getLoc());Приходит ненулевой указатель target. Происходит попытка его динамически привести к другому типу. Если это не удалось, то указатель tilingOp окажется равен nullptr. В этом случае ненулевой указатель target будет использован при формировании некоего сообщения о сбое.
Что хотелось сказать этой статьёй?
Здесь вы можете скачать и попробовать триальную полнофункциональную версию PVS-Studio. Для открытых проектов существует вариант бесплатного лицензирования.
Помимо статей, мы регулярно выкладываем записи докладов и вебинаров. Возможно, вас они заинтересуют:
Если понравится, не забывайте время от времени заглядывать к нам на сайт — будут разбираться и другие интересные темы.