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

Назначение статьи

Иногда программисты задаются вопросом, а есть ли смысл пробовать какие-то дополнительные инструменты контроля качества кода, помимо тех, которые предоставляют компиляторы/среды разработки. Есть ли преимущества у таких инструментов перед "условным -Wall"? Стоит ли внедрять в процесс разработки дополнительный инструмент, такой как PVS-Studio?

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

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

Однако это теоретический ответ. А как дела обстоят на практике?

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

• 2021 — LLVM 13.0;
• 2020 — GCC 10;
• 2020 — LLVM 11;
• 2019 — Roslyn;
• 2019 — LLVM 8.

Проверку LLVM 14 мы пропустили, но решили, что стоит написать заметку, посвященную проверке LLVM 15.

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

При разработке проекта LLVM для предотвращения ошибок используются диагностические возможности компилятора Clang. LLVM проверяется с помощью Clang Static Analyzer и clang-tidy. Более того, проект LLVM дополнительно проверяется с помощью проприетарного анализатора Coverity.

Найти даже несколько ошибок в LLVM с помощью статического анализатора — это уже большое достижение. И анализатор PVS-Studio на это способен!

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

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

Это не является правильным сценарием работы с анализатором. Надо понимать, что моей целью было только написать эту статью :). И с этой задачей я успешно справился. О том, как правильно начать использовать анализатор, я писал в статье "Как внедрить статический анализатор кода в legacy проект и не демотивировать команду".

Итак в отчёте встретились ложно-положительные срабатывания. С этими всё понятно, любой статический анализатор, к сожалению, их выдаёт. Интересен другой момент. На проекте LLVM я увидел очень большое количество предупреждений, которые нельзя назвать ложными. Анализатор прав. Но и полезными эти сообщения назвать сложно. Давайте чуть подробнее остановимся на этой теме.

Очень много неоднозначных предупреждений связано с условиями, которые всегда истинны или ложны. Из-за этого я даже не захотел изучать предупреждения V547 — Expression is always true/false. Это утомительно и неинтересно. Давайте на паре примеров посмотрим, что я имею в виду.

int L = -1, V = -1, T = -1;
if (L != -1 && V != -1 && T != -1 && Name.empty()) {  // <=
  if (!Strict) {
    Buffer.append("HANGUL SYLLABLE ");
    if (L != -1)                                      // <=
      Buffer.append(HangulSyllables[L][0]);
    if (V != -1)                                      // <=
      Buffer.append(HangulSyllables[V][1]);
    if (T != -1)                                      // <=
      Buffer.append(HangulSyllables[T][2]);
  }

Здесь анализатор выдаёт сразу 3 предупреждения:

• V547 [CWE-571] Expression 'L != — 1' is always true. UnicodeNameToCodepoint.cpp 306
• V547 [CWE-571] Expression 'V != — 1' is always true. UnicodeNameToCodepoint.cpp 308
• V547 [CWE-571] Expression 'T != — 1' is always true. UnicodeNameToCodepoint.cpp 310

И действительно, переменные L, V, T проверяются дважды. Повторные проверки избыточны, но и ошибкой это назвать нельзя.

Другой пример:

while (top_reader_sp) {
  if (!top_reader_sp)
    break;
  top_reader_sp->Run();

Предупреждение: V547 [CWE-570] Expression '!top_reader_sp' is always false. Debugger.cpp 1042

Условный оператор действительно не имеет смысла. Это похоже на последствия множественных правок и рефакторинга. Код менялся, менялся и в результате стал вот таким. Да, это можно назвать атавизмом, но на ошибку всё равно не тянет.

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

42 ошибки в коде LLVM

Почему 42? Потому, что "42 — ответ на главный вопрос жизни, вселенной и всего такого" :). Выписывать больше примеров ошибок мне было утомительно и неинтересно. Да и всё равно читать здоровый отчёт будет никому неинтересно. Ну разве что разработчикам LLVM… Но им будет полезней проверить проект, настроить анализатор PVS-Studio и самостоятельно изучить предупреждения.

Фрагмент N1

Классическая ошибка, похожая на множество тех, что я рассматривал в статье "Зло живёт в функциях сравнения". Конечно, перед нами не совсем функция сравнения. Однако суть в том же. Есть два объекта, над которыми выполняется ряд проверок. Эта функция скучна, и её не интересно внимательно проверять на code review. И именно поэтому в ней есть эта ошибка, которую, к счастью, может заметить статический анализатор кода. Анализатор никогда не устаёт и не ленится :).

static bool isCopyCompatibleType(LLT SrcTy, LLT DstTy) {
  if (SrcTy == DstTy)
    return true;

  if (SrcTy.getSizeInBits() != DstTy.getSizeInBits())
    return false;

  SrcTy = SrcTy.getScalarType();
  DstTy = DstTy.getScalarType();

  return (SrcTy.isPointer() && DstTy.isScalar()) ||
         (DstTy.isScalar() && SrcTy.isPointer());
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions '(SrcTy.isPointer() && DstTy.isScalar())' to the left and to the right of the '||' operator. CallLowering.cpp 1198

Ошибка здесь:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(DstTy.isScalar()  && SrcTy.isPointer())

Программист одновременно поменял во второй строчке Src на Dst и Pointer на Scalar. В результате получается, что два раза проверяется одно и то же! Код выше эквивалентен:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isPointer() && DstTy.isScalar())

Правильный вариант:

(SrcTy.isPointer() && DstTy.isScalar()) ||
(SrcTy.isScalar()  && DstTy.isPointer())

Фрагмент N2

И сразу ещё одна родственная ошибка.

static ValueKnowledge meet(const ValueKnowledge &lhs,
                           const ValueKnowledge &rhs) {
  ValueKnowledge result = getPessimisticValueState();
  result.hasError = true;

  if (!rhs || !rhs || lhs.dtype != rhs.dtype)   // <=
    return result;

  result.hasError = false;
  result.dtype = lhs.dtype;
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the '||' operator: !rhs ||!rhs ShapeUtils.h 141

Дважды проверяется объект rhs, а lhs не проверяется. Обыкновенная спешка при написании кода с отсутствием последующей внимательной проверки на этапе code review.

Фрагмент N3

std::optional<parser::MessageFixedText> Scope::SetImportKind(ImportKind kind) {
  ....
  } else if (kind != *importKind_ &&
      (kind != ImportKind::Only || kind != ImportKind::Only)) {
    return 
      "Every IMPORT must have ONLY specifier if one of them does"_err_en_US;
  } else {
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions 'kind != ImportKind::Only' to the left and to the right of the '||' operator. scope.cpp 275

Нет смысла два раза выполнять одну и ту же проверку. Перед нами явная опечатка. Должна быть использована ещё какая-то именованная константа, помимо ImportKind::Only.

Примечание. Если бы выражение было оформлено в виде "столбика", а не в одну строчку, то, возможно, ошибку было бы легче заметить. См. главу 13. Выравнивайте однотипный код "таблицей" :) Я бы писал этот код так:

} else if (kind != *importKind_ &&
    (   kind != ImportKind::Only
     || kind != ImportKind::Only)) {
  return ....;

Фрагмент N4

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

bool Merger::maybeZero(unsigned e) const {
  if (tensorExps[e].kind == kInvariant) {
    if (auto c = tensorExps[e].val.getDefiningOp<complex::ConstantOp>()) {
      ArrayAttr arrayAttr = c.getValue();
      return arrayAttr[0].cast<FloatAttr>().getValue().isZero() &&
             arrayAttr[0].cast<FloatAttr>().getValue().isZero();
    }
    if (auto c = tensorExps[e].val.getDefiningOp<arith::ConstantIntOp>())
      return c.value() == 0;
    if (auto c = tensorExps[e].val.getDefiningOp<arith::ConstantFloatOp>())
      return c.value().isZero();
  }
  return true;
}

Предупреждение PVS-Studio: V501 [CWE-571] There are identical sub-expressions 'arrayAttr[0].cast < FloatAttr > ().getValue().isZero()' to the left and to the right of the '&&' operator. Merger.cpp 812

Подозрительно это место:

return arrayAttr[0].cast<FloatAttr>().getValue().isZero() &&
       arrayAttr[0].cast<FloatAttr>().getValue().isZero();

Фрагмент N5

И последняя ошибка, выявленная диагностикой V501.

template <int KIND>
Expr<Type<TypeCategory::Logical, KIND>> FoldIntrinsicFunction(....)
{
  ....
  } else if (name == "__builtin_ieee_support_datatype" ||
      name == "__builtin_ieee_support_denormal" ||
      name == "__builtin_ieee_support_divide" ||     // <=
      name == "__builtin_ieee_support_divide" ||     // <=
      name == "__builtin_ieee_support_inf" ||
      name == "__builtin_ieee_support_io" ||
      name == "__builtin_ieee_support_nan" ||
      name == "__builtin_ieee_support_sqrt" ||
      name == "__builtin_ieee_support_standard" ||
      name == "__builtin_ieee_support_subnormal" ||
      name == "__builtin_ieee_support_underflow_control") {
    return Expr<T>{true};
  }
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions 'name == "__builtin_ieee_support_divide"' to the left and to the right of the '||' operator. fold-logical.cpp 218

Я пометил комментарием две одинаковые проверки. Скорее всего, одна из них просто лишняя. Я думаю, что перед нами не настоящая ошибка, а избыточный код. Однако есть небольшая вероятность, что здесь забыли сравнить name с какой-то другой строковой константой.

Фрагмент N6

Для начала обратите внимание, что bmap — это указатель:

struct isl_coalesce_info {
  isl_basic_map *bmap;
  ....
};

А теперь ошибочный код, выполняющий бессмысленную проверку этого члена класса:

static isl_stat tab_insert_divs(struct isl_coalesce_info *info, ....)
{
  ....
  if (any) {
    if (isl_tab_rollback(info->tab, snap) < 0)
      return isl_stat_error;
    info->bmap = isl_basic_map_cow(info->bmap);
    info->bmap = isl_basic_map_free_inequality(info->bmap, 2 * n);

    if (info->bmap < 0)                 // <=
      return isl_stat_error;

    return fix_constant_divs(info, n, expanded);
  }
  ....
}

Предупреждение PVS-Studio: V503 [CWE-697, CERT-EXP08-C] This is a nonsensical comparison: pointer < 0. isl_coalesce.c 3181

Нет практического смысла выполнять проверку "указатель < 0". Скорее всего, перед нами опечатка или код не дописан. Например, с 0 должен сравниваться какой-то из членов структуры isl_basic_map.

Фрагмент N7

Встретилась достаточно редкая и интересная ошибка. По крайней мере, до этого момента в нашей коллекции, было всего два таких бага, найденных нашей командой в открытых проектах.

void FunctionLoweringInfo::ComputePHILiveOutRegInfo(const PHINode *PN) {
  ....
  Register DestReg = It->second;
  if (DestReg == 0)
    return
  assert(Register::isVirtualRegister(DestReg) &&
         "Expected a virtual reg");
  LiveOutRegInfo.grow(DestReg);
  ....
}

Предупреждение PVS-Studio: V504 [CWE-841] It is highly probable that the semicolon ';' is missing after 'return' keyword. FunctionLoweringInfo.cpp 454

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

Кажется, что assert выполняется в том случае, если не выполняется условие DestReg == 0. Но на самом деле, assert выполнится только в том случае, если DestReg == 0.

Нестрашна ошибка только из-за везения. Собственно, assert не оказывает заметного влияния на выполнение программы. А в release-версии он вообще превращается в ничто.

Фрагмент N8, N9

Не изучал историю этого кода, но, скорее всего, это "калька с malloc". Т.е. когда-то здесь использовалась функция malloc, а затем её грубо заменили на new и shared_ptr.

std::shared_ptr<uint8_t>
RenderScriptRuntime::GetAllocationData(....) {
  ....
  const uint32_t size = *alloc->size.get();
  std::shared_ptr<uint8_t> buffer(new uint8_t[size]);
  if (!buffer) {
    LLDB_LOGF(log, "%s - couldn't allocate a %" PRIu32 " byte buffer",
              __FUNCTION__, size);
    return nullptr;
  }
  ....
  return buffer;
}

Предупреждение PVS-Studio: V554 [CWE-762, CERT-MEM51-CPP] Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. RenderScriptRuntime.cpp 2371

Умный указатель std::shared_ptr используется некорректно:

std::shared_ptr<uint8_t> buffer(new uint8_t[size]);

Память будет освобождаться с помощью delete, а не delete [], что приводит к неопределённому поведению. Подробности см. в статье "Почему в С++ массивы нужно удалять через delete[]". Правильный вариант:

std::shared_ptr<uint8_t []> buffer(new uint8_t[size]);

Поясняю, почему я предполагаю, что раньше здесь использовался malloc. Обратите внимание на последующую проверку указателя на равенство nullptr. Она не имеет смысла, так как в случае ошибки выделения памяти будет брошено исключение std::bad_alloc. Проверка и вывод отладочного сообщения — атавизм.

Ещё точно такая же "калька с malloc": V554 [CWE-762, CERT-MEM51-CPP] Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. RenderScriptRuntime.cpp 2698

Фрагмент N10

Ой, я давно не находил столь примечательную и забавную ошибку! Однозначно жемчужина среди опечаток!

OwningMemRef &operator=(const OwningMemRef &&other) {
  freeFunc = other.freeFunc;
  descriptor = other.descriptor;
  other.freeFunc = nullptr;
  memset(0, &other.descriptor, sizeof(other.descriptor));
}

Предупреждение PVS-Studio: V575 [CWE-628, CERT-EXP37-C] The null pointer is passed into 'memset' function. Inspect the first argument. MemRefUtils.h 194

Я описал разнообразные способы, как можно ошибиться, используя функцию memset, в статье "Самая опасная функция в мире С/С++". Но это что-то новенькое!

Перепутан первый и второй аргумент функции. Запись произойдёт по нулевому указателю.

Хотя на самом деле не произойдёт. Этот код просто не скомпилируется, так как в качестве второго фактического аргумента передаётся указатель, а функция ожидает int. Этот код находится в шаблонном классе. И пока функция не используется, она не инстанцируется, и компилятор не замечает, что код некорректен. В случае шаблонов компилятор может не выдавать ошибки на некорректный код, пока он не используется.

В общем ничего страшного, но тем не менее это ошибка, которую лучше поправить заранее.

Фрагмент N11-N17

Самым распространённым среди обнаруживаемых нашей командой в открытых проектах дефектов является разыменование указателя до его проверки. По крайней мере больше всего в базе ошибок выписано случаев именно такого типа. Исходные коды LLVM не исключение.

void LibCallSimplifier::classifyArgUse(....) {
  CallInst *CI = dyn_cast<CallInst>(Val);
  Module *M = CI->getModule();

  if (!CI || CI->use_empty())
    return;
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The 'CI' pointer was utilized before it was verified against nullptr. Check lines: 2515, 2517. SimplifyLibCalls.cpp 2515

Указатель CI в начале разыменовывается при вызове функции: CI->getModule(). И только затем проверяется на равенство nullptr.

Ещё один пример:

void Module::FindFunctions(....) {
  ....
  for (size_t i = 0; i < num_matches; ++i) {
    sc.symbol = symtab->SymbolAtIndex(symbol_indexes[i]);
    SymbolType sym_type = sc.symbol->GetType();
    if (sc.symbol && (sym_type == eSymbolTypeCode ||
                      sym_type == eSymbolTypeResolver))
      sc_list.Append(sc);
  }
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The 'sc.symbol' pointer was utilized before it was verified against nullptr. Check lines: 877, 878. Module.cpp 877

Чтобы сократить выражение, программист решил предварительно сохранить некий тип символа в переменной sym_type:

SymbolType sym_type = sc.symbol->GetType();

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

if (sc.symbol && (sym_type == eSymbolTypeCode ||
                  sym_type == eSymbolTypeResolver))

Далее рассматривать подобные ошибки неинтересно. Более того, я не стал изучать все предупреждения V595.

Фрагмент N18

Optional<SmallVector<ReassociationIndices>>
mlir::getReassociationIndicesForCollapse(ArrayRef<int64_t> sourceShape,
                                         ArrayRef<int64_t> targetShape) {
  unsigned sourceDim = 0;
  ....
  int64_t currTargetShape = targetShape[targetDim];
  while (sourceShape[sourceDim] != ShapedType::kDynamicSize &&
         prodOfCollapsedDims * sourceShape[sourceDim] < currTargetShape &&
         sourceDim < sourceShape.size()) {
    prodOfCollapsedDims *= sourceShape[sourceDim];
    currIndices.push_back(sourceDim++);
  }
  ....
}

Предупреждение PVS-Studio: V781 [CWE-20, CERT-API00-C] The value of the 'sourceDim' index is checked after it was used. Perhaps there is a mistake in program logic. ReshapeOpsUtils.cpp 49

Потенциальный выход за границу массива. Взглянем внимательно на это условие цикла:

sourceShape[sourceDim] != ShapedType::kDynamicSize && .... &&
sourceDim < sourceShape.size()

Проверка индекса выполняется до обращения к элементу массива. Более логичным и безопасным выглядит такой вариант кода:

sourceDim < sourceShape.size() &&
sourceShape[sourceDim] != ShapedType::kDynamicSize && ....

Фрагмент N19-N36

Неожиданно много опасных разыменований указателей встретилось в конструкторах классов. Указатель смело разыменовывается при инициализации членов класса, а затем вдруг проверяется на равенство nullptr в теле конструктора. Пример:

typedef std::shared_ptr<lldb_private::ValueObject> ValueObjectSP;

lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
    LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
    : SyntheticChildrenFrontEnd(*valobj_sp), m_pair_ptr(), m_pair_sp() {
  if (valobj_sp)
    Update();
}

Предупреждение PVS-Studio: V664 [CWE-476, CERT-EXP34-C] The 'valobj_sp' pointer is being dereferenced on the initialization list before it is verified against null inside the body of the constructor function. Check lines: 211, 212. LibCxx.cpp 211

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

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

Нет, на такое допущение закладываться нельзя. Разыменование нулевого указателя — это неопределённое поведение. И невозможно предсказать, как оно проявит себя. Например, компилятор видит, что указатель разыменовывается. Значит, он точно не нулевой. Следовательно, в целях оптимизации проверку "if (valobj_sp)" можно удалить. Подробнее эту тему я рассматривал в статье "Разыменовывание нулевого указателя приводит к неопределённому поведению". И вот здесь тоже про это говорится "What Every C Programmer Should Know About Undefined Behavior #2/3".

Фрагмент N37

uint64_t NullabilityPayload = 0;

static constexpr const unsigned NullabilityKindMask = 0x3;

void addTypeInfo(unsigned index, NullabilityKind kind) {
  ....
  NullabilityPayload &=
    ~(NullabilityKindMask << (index * NullabilityKindSize));
  ....
}

Предупреждение PVS-Studio: V784 [CWE-197, CERT-INT31-C] The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. Types.h 529

Старшие биты в переменной NullabilityPayload будут потеряны. Сейчас поясню, как это произойдёт.

Переменная NullabilityKindMask представлена 32-битным типом unsigned. Сколько не сдвигай биты в этой переменной, результат всё равно будет иметь тип unsigned. Оператор побитового НЕ (~) также не изменит тип выражения.

И только в момент выполнения оператора &= тип unsigned будет расширен до типа uint64_t. Так как тип был беззнаковый, то старшие биты 64-битного типа будут всегда нулевыми. В результате выполнится операция:

NullabilityPayload &= 0x00000000xxxxxxxx;

Что приведёт к сбросу старших битов в NullabilityPayload. Правильный вариант кода:

NullabilityPayload &=
  ~(static_cast<uint64_t>(NullabilityKindMask)
     << (index * NullabilityKindSize));

Другой вариант исправить код — это сразу сделать константу NullabilityKindMask 64-битной:

static constexpr const uint64_t NullabilityKindMask = 0x3;
....
NullabilityPayload &=
  ~(NullabilityKindMask << (index * NullabilityKindSize));

Фрагмент N38

Бывает интересно заметить ошибку в тестах и понять, что он тестирует не совсем то, что задумывалось. Это, кстати, хороший пример, когда статический анализ дополняет юнит-тесты и методологию TDD. Ведь никто не пишет тесты на тесты :). К счастью, статический анализатор помогает находить ошибки не только в основном коде, но и в тестах.

TEST(RegisterContextMinidump, ConvertMinidumpContext_x86_64) {
  MinidumpContext_x86_64 Context;
  ....
  Context.rax = 0x0001020304050607;
  Context.rbx = 0x08090a0b0c0d0e0f;
  ....
  Context.eflags = 0x88898a8b;
  Context.cs = 0x8c8d;
  Context.fs = 0x8e8f;
  Context.gs = 0x9091;
  Context.ss = 0x9293;
  Context.ds = 0x9495;
  Context.ss = 0x9697;
  llvm::ArrayRef<uint8_t> ContextRef(reinterpret_cast<uint8_t *>(&Context),
                                     sizeof(Context));
  ....
}

Предупреждение PVS-Studio: V519 [CWE-563, CERT-MSC13-C] The 'Context.ss' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 110, 112. RegisterContextMinidumpTest.cpp 112

Обратите внимание на вот эти строчки:

Context.ss = 0x9293;
....
Context.ss = 0x9697;

Опечатка. В одном месте, по всей видимости, следовало написать Context.es.

Фрагмент N39, N40

В начале статьи я говорил, что не стал разбираться с некоторыми предупреждениями, такими как V547. Но немного я всё-таки их посмотрел и заметил, например, вот такую ошибку. Её обнаружение возможно благодаря технологии data-flow анализа, который учитывает возможные значения переменных в разных точках программы. Если вам интересно, как вообще работает анализатор, предлагаю вашему вниманию заметку "Технологии статического анализа кода PVS-Studio".

static std::unordered_multimap<char32_t, std::string>
loadDataFiles(const std::string &NamesFile, const std::string &AliasesFile) {
  ....
  auto FirstSemiPos = Line.find(';');
  if (FirstSemiPos == std::string::npos)                   // <=
    continue;
  auto SecondSemiPos = Line.find(';', FirstSemiPos + 1);
  if (FirstSemiPos == std::string::npos)                   // <=
    continue;
  ....
}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'FirstSemiPos == std::string::npos' is always false. UnicodeNameMappingGenerator.cpp 46

Скорее всего, этот код писался с помощью Copy-Paste. Был размножен этот фрагмент:

auto FirstSemiPos = Line.find(';');
if (FirstSemiPos == std::string::npos)
  continue;

Затем поменяли вызов функции find. First заменили на Second, но не везде. В результате строчка

if (FirstSemiPos == std::string::npos)

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

И ещё одна прям точно такая же ошибка:

Status Host::ShellExpandArguments(ProcessLaunchInfo &launch_info) {
  ....
  auto data_sp = StructuredData::ParseJSON(output);
  if (!data_sp) {                                        // <=
    error.SetErrorString("invalid JSON");
    return error;
  }

  auto dict_sp = data_sp->GetAsDictionary();
  if (!data_sp) {                                        // <=
    error.SetErrorString("invalid JSON");
    return error;
  }
  ....
}

Предупреждение PVS-Studio: V547 [CWE-570] Expression '!data_sp' is always false. Host.cpp 248

Фрагмент N41

Для выявления двух предыдущих ошибок использовался анализ потока данных (data-flow анализ). Для следующей ошибки используется ещё более изощрённая техника: символьное выполнение (symbolic execution). Смысл в том, что необязательно знать возможные значения переменных. Чтобы определить, что условие истинно/ложно, можно решить уравнение.

void Sema::adjustMemberFunctionCC(QualType &T, bool IsStatic, bool IsCtorOrDtor,
                                  SourceLocation Loc) {
  ....
  CallingConv CurCC = FT->getCallConv();
  CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);

  if (CurCC == ToCC)
    return;
  ....
  CallingConv DefaultCC = 
    Context.getDefaultCallingConvention(IsVariadic, IsStatic);

  if (CurCC != DefaultCC || DefaultCC == ToCC)
    return;
  ....
}

Предупреждение PVS-Studio: V560 [CWE-570] A part of conditional expression is always false: DefaultCC == ToCC. SemaType.cpp 7856

Давайте разбираться, почему анализатор решил, что правая часть условия всегда ложна. Для удобства уберём всё лишнее и заменим имена:

A = ....;
B = ....;
if (A == B) return;
C = ....;
if (A != C || C == B)

Разберём как работает этот код:

• есть 3 переменные A, B, C, значения которых нам неизвестны;
• но мы знаем, что если A == B, то происходит выход из функции;
• следовательно, если функция продолжает выполняться, то A != B;
• если A != C, то, в силу вычисления по короткой схеме, правое подвыражение не вычисляется;
• если правое подвыражение "C == B" вычисляется, значит A == C;
• если A != B и A == C, то С никак не может быть равно B.

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

if (CurCC != DefaultCC || DefaultCC != ToCC)

Фрагмент N42

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

ObjectFileMachO::MachOCorefileAllImageInfos
ObjectFileMachO::GetCorefileAllImageInfos() {
  ....
  lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);
  ....
  uint32_t imgcount = m_data.GetU32(&offset);
  uint64_t entries_fileoff = m_data.GetU64(&offset);
  offset += 4; // uint32_t entries_size;
  offset += 4; // uint32_t unused;
  offset = entries_fileoff;                  // <=
  ....
}

Предупреждение PVS-Studio: V519 [CWE-563, CERT-MSC13-C] The 'offset' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 6888, 6890. ObjectFileMachO.cpp 6890

Очень странно в начале что-то считать в переменной offset, а потом взять и перетереть её значение. Возможно, здесь должно быть написано так:

offset += 4; // uint32_t entries_size;
offset += 4; // uint32_t unused;
offset += entries_fileoff;

Скачайте и попробуйте PVS-Studio

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Examples of errors that PVS-Studio found in LLVM 15.0.