19 ошибок в LLVM 1920.11.2024 12:01

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

Предыдущие проверки LLVM

LLVM — проект программной инфраструктуры для создания компиляторов и сопутствующих им утилит. Состоит из набора компиляторов языков высокого уровня, системы оптимизации, интерпретации и компиляции в машинный код. Написан на современной версии языка C++. Проект используется огромным количеством пользователей и хорошо протестирован.

Время от времени мы проверяем этот проект, чтобы продемонстрировать возможности PVS-Studio. Предыдущие публикации:

1. Август 2011;
2. Август 2012;
3. Октябрь 2016;
4. Апрель 2019;
5. Октябрь 2020;
6. Октябрь 2021;
7. Октябрь 2022;
8. Май 2024: часть 1, часть 2.

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

Собственно, LLVM, если я правильно понимаю, уже регулярно проверяется с помощью Coverity Scan Static Analysis и Clang Static Analyzer. Другое дело, что хорошо бы ещё и PVS-Studio добавить :)

Найденные баги

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

Баг 1. Не там поставлена закрывающаяся скобка

Для начала взглянем на прототип функции 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 выдаёт на это сразу два предупреждения:

• V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. ShrinkWrapping.cpp 80
• V639 Consider inspecting the expression for 'isStoreUsed' function call. It is possible that one of the closing ')' parentheses was positioned incorrectly. ShrinkWrapping.cpp 80

Красивая ошибка. Код компилируется по той причине, что последний аргумент функции опционален. По сути, он эквивалентен этому:

SRU.isStoreUsed(*FIE,
                Prev ? SRU.expr_begin(*Prev) : SRU.expr_begin(BB));
if (/*IncludeLocalAccesses=*/false) {

Заметить такую ошибку на обзоре кода не так просто. Анализатор PVS-Studio может быть хорошим помощником в высматривании подобных опечаток.

Баг 2. Путаница в именах lowerBound/upperBound

FailureOr
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

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

Баг 3. 0, 1, 2

Для тех, кто не читал, загляните в «Ноль, один, два, Фредди заберёт тебя».

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);

В случае табличного форматирования ошибку будет проще заметить во время обзора кода.

Баг 4–6. И ещё такие же опечаточки

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 
LIBC_NAMESPACE::cpp::enable_if_t,
                                 bool>
ValuesEqual(T x1, T x2) {
  LIBC_NAMESPACE::fputil::FPBits bits1(x1);
  LIBC_NAMESPACE::fputil::FPBits 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©' to the left and to the right of the '||' operator. OpenACCClause.cpp 31

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

Баг 7. Бессмысленная проверка указателя

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, или вообще другая проверка.

Баг 8. Опасный сдвиг литерала 1

static Expected
GeneratePerfEventConfigValue(bool enable_tsc,
                             std::optional psb_period) {
  uint64_t config = 0;
  ....
  if (Expected 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;

Баг 9. Неправильное использование unique_ptr

std::unique_ptr 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 m_options_definition_up;

Баг 10. Неправильная проверка индекса

std::vector 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:

• V557 Array overrun is possible. The 'index' index is pointing beyond array bound. AppleObjCRuntimeV2.cpp 3284
• V557 Array overrun is possible. The 'index' index is pointing beyond array bound. AppleObjCRuntimeV2.cpp 3287

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

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 году».

Баг 11. Переменная присваивается сама себе

template  class MapLattice {
  using Container = llvm::DenseMap;
  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. Коллега, читая черновик, указал, что, оказывается, в предыдущей статье мы уже описывали эту ошибку. Но она по-прежнему на месте. Жаль, но правки по мотивам наших проверок бывают небыстрыми. Иногда совсем небыстрыми :)

Баги 12–13. Потенциальные утечки памяти

std::unique_ptr 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 ручное управление памятью. Такой код прямо-таки притягивает к себе ошибки…

Баг 14. Кажется, перепутали || и &&

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;

Баги 15–16. Раздувание кода

В заголовочных файлах встречаются достаточно тяжеловесные объекты, например, такие:

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 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.

Баги 17–18. Потенциальное разыменование нулевых указателей

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).

Баг 19. Косвенное обнаружение ошибки

Закончим интересным случаем, когда анализатор ошибся в диагностировании происходящего, но всё равно навёл на реальную ошибку.

DiagnosedSilenceableFailure
transform::ConvertToLoopsOp::apply(transform::TransformRewriter &rewriter,
                                   transform::TransformResults &results,
                                   transform::TransformState &state) {
  SmallVector loops;
  for (Operation *target : state.getPayloadOps(getTarget())) {
    auto tilingOp = dyn_cast(*target);
    if (!target) {
      DiagnosedSilenceableFailure diag =
          emitSilenceableError()
          << "expected the payload to implement TilingInterface";
      diag.attachNote(target->getLoc()) << "payload op";
      return diag;
    }
  ....
}

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

• V522 Dereferencing of the null pointer 'target' might take place. LinalgTransformOps.cpp 2219
• V595 The 'target' pointer was utilized before it was verified against nullptr. Check lines: 2214, 2215. LinalgTransformOps.cpp 2214

Выделю суть:

auto tilingOp = dyn_cast(*target);
if (!target)
  Failure(target->getLoc());

Анализатор переживает, что разыменовывается нулевой указатель target. На самом деле этот указатель никогда не будет нулевым, так что условие никогда не выполнится.

Ошибка в том, что проверяется не тот указатель. Правильный вариант кода:

auto tilingOp = dyn_cast(*target);
if (!tilingOp)
  Failure(target->getLoc());

Приходит ненулевой указатель target. Происходит попытка его динамически привести к другому типу. Если это не удалось, то указатель tilingOp окажется равен nullptr. В этом случае ненулевой указатель target будет использован при формировании некоего сообщения о сбое.

Выводы

Что хотелось сказать этой статьёй?

1. Никто не застрахован от ошибок, даже такте высококвалифицированные разработчики, как контрибьюторы в проект LLVM.
2. PVS-Studio способен находить ошибки даже в компиляторах и способен значительно улучшить качество и надёжность кода.
3. Статические анализаторы нужно использовать регулярно, а не время от времени. Аналогия — просмотр предупреждений компилятора.

Здесь вы можете скачать и попробовать триальную полнофункциональную версию PVS-Studio. Для открытых проектов существует вариант бесплатного лицензирования.

P.S. Наши доклады

Помимо статей, мы регулярно выкладываем записи докладов и вебинаров. Возможно, вас они заинтересуют:

• Олег Лысый. Парсим С++. В этом вебинаре мы обсудили грамматические конструкции в С++, и как они работают. Мы поговорили о разных видах парсеров и о том, почему С++ сложно парсить. Мы также поделились некоторыми хитростями, позволяющими избежать чрезмерного замедления.
• Филипп Хандельянц. Разрушители мифов: С++ уже не торт. Язык программирования С++ существует уже более 40 лет. За это время появилась целая куча мифов и небылиц о нём. Мы будем вместе опровергать или подтверждать их. Действительно ли С++ непрост для новичков? С++ устарел и больше никому не нужен?
• Юрий Минаев. Семантика для кремниевых мозгов. Семантика языка С++ с точки зрения статического анализатора или фронтенда компилятора.
• Андрей Карпов, Юрий Минаев. Статический анализ кода / Виды анализа и диагностики. Как устроен статический анализ и в чём главные отличия от линтера и фронтенда компилятора? Почему PVS-Studio не использует LLVM? Для каких диагностик важен Data-Flow анализ и насколько тяжело пока обходиться без CFG? Что нужно знать и уметь, чтобы попасть на работу в PVS-Studio? Что регулирует ГОСТ на статический анализ и правда ли он помогает в построении процессов?

Если понравится, не забывайте время от времени заглядывать к нам на сайт — будут разбираться и другие интересные темы.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. 19 errors in LLVM 19.

© Habrahabr.ru