PVS-Studio vs Clang

• Ошибка Copy-Paste N1
• Ошибка Copy-Paste N2
• Ошибка Copy-Paste N3
• Ошибка Copy-Paste N4
• Ошибка Copy-Paste N5
• Вот, например, классическая ошибка в switch
• Бестолковое действие N1, которое может быть ошибкой
• Бестолковое действие N2, которое точно не является ошибкой
• Бестолковое действие N3, которое я затрудняюсь описать
• Есть код, который просто потенциально опасен
• Заключение
• P.S.
• Дополнительные ссылки

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

PVS-Studio сейчас использует внешний препроцессор Microsoft Visual C++, что является существенным недостатком. Препроцессор Visual C++ крайне медленный и имеет ошибки, которые мы не можем исправить. Да, пусть вас не удивляет, что препроцессор работает из вон рук плохо, но это не мешает быстро и правильно компилировать файлы в Visual Studio. Как именно устроен cl.exe я не знаю, но по косвенным уликам могу предположить, что имеется два совершенно разных алгоритма препроцессирования, используемых для компиляции и для создания*.i файлов. Видимо, так удобнее в плане архитектуры компилятора.

В последнее время мы активно занялись поиском альтернативного решения для препроцессирования файлов. И по все видимости, наш выбор остановится на препроцессоре, реализованном в Clang. Предварительные замеры показывают, что он работает в несколько раз быстрее cl.exe, что очень приятно и полезно.

Clang — это новый компилятор для C-подобных языков (C, C++, Objective-C ,Objective-C++, в процессе поддержка C++11). Разработка спонсируется корпорацией Apple. Одной из сильных сторон компилятора Clang является большое количество правил статического анализа кода. Практически, Clang уже используется Apple как инструмент статического анализа.

Clang изначально спроектирован для максимального сохранения информации в ходе процесса компиляции [1]. Эта особенность позволяет Clang создавать развернутые контекстно-ориентированные сообщения об ошибках, понятные как для программистов, так и для сред разработки. Модульный дизайн компилятора позволяет использовать его в составе среды разработки для подсветки синтаксиса и рефакторинга.

Так что, по хорошему, PVS-Studio, возможно, стоило бы основывать именно на Clang, а не на VivaCore [2]. Но теперь уже поздно и всё не так однозначно. В этом случае мы бы слишком сильно зависели от возможностей сторонней библиотеки. Да и поддерживать Microsoft Specific в проекте Clang не спешат.

Однако, мы отвлеклись. Наиболее интересно проверить один анализатор кода другим анализатором кода. Мы это и сделали, раз всё равно уже начали изучать проект Clang.

К сожалению, полноценное сравнение сделать невозможно. Было бы замечательно проверить Clang с помощью PVS-Studio и наоборот. А потом посчитать количество найденных ошибок на одну тысячу строк. Беда в том, что мы можем проверить Clang, а он нас нет. В Clang поддержка MSVC экспериментальная. Плюс дело осложняется тем, что в PVS-Studio уже используются возможности C++11. Простая попытка скомпилировать приводит к ошибкам вида 'Эти расширения языка ещё не поддерживаются'.

Придется ограничиться тем, что удалось найти PVS-Studio в коде Clang. Естественно то, что будет описано в статье, это не все ошибки, которые были найдены. Clang это около 50 мегабайт исходного кода. Я уведомлю разработчиков о найденных дефектах и, если им будет интересно, то они сами смогут проверить свой проект и изучить весь список потенциальных ошибок. Впрочем, они про нас слышали и обсуждали некоторые наши диагностики.

Посмотрим, что мы нашли интересненького. Практически все найденные ошибки, являются ошибками Copy-Paste.

Ошибка Copy-Paste N1

static SDValue PerformSELECTCombine(...)
{
  ...
  SDValue LHS = N->getOperand(1);
  SDValue RHS = N->getOperand(2);

  ...
  if (!UnsafeFPMath &&
      !DAG.isKnownNeverZero(LHS) && !DAG.isKnownNeverZero(RHS))
  ...
  if (!UnsafeFPMath &&
      !DAG.isKnownNeverZero(LHS) && !DAG.isKnownNeverZero(LHS))
  ...
}

Диагностика PVS-Studio: V501 There are identical sub-expressions '!DAG.isKnownNeverZero (LHS)' to the left and to the right of the '&&' operator. LLVMX86CodeGen x86isellowering.cpp 11635

В начале код одновременно работает с LHS и с RHS, а потом только с LHS. Причиной тому, видимо, стала описка или скопированный фрагмент строки. Как я понимаю, во втором случае также должна присутствовать переменная RHS.

Ошибка Copy-Paste N2

MapTy PerPtrTopDown;
MapTy PerPtrBottomUp;

void clearBottomUpPointers() {
  PerPtrTopDown.clear();
}

void clearTopDownPointers() {
  PerPtrTopDown.clear();
}

Диагностика PVS-Studio: V524 It is odd that the body of 'clearTopDownPointers' function is fully equivalent to the body of 'clearBottomUpPointers' function (ObjCARC.cpp, line 1318). LLVMScalarOpts objcarc.cpp 1322

Классический Copy-Paste. Функция была скопирована. Было изменено её имя, но не само тело. Правильный вариант:

void clearBottomUpPointers() {
  PerPtrBottomUp.clear();
}

Ошибка Copy-Paste N3

static Value *SimplifyICmpInst(...) {
  ...
  case Instruction::Shl: {
    bool NUW = LBO->hasNoUnsignedWrap() && LBO->hasNoUnsignedWrap();
    bool NSW = LBO->hasNoSignedWrap() && RBO->hasNoSignedWrap();
  ...
}

Диагностика PVS-Studio: V501 There are identical sub-expressions 'LBO->hasNoUnsignedWrap ()' to the left and to the right of the '&&' operator. LLVMAnalysis instructionsimplify.cpp 1891

По всей видимости, хотели написать так:

bool NUW = LBO->hasNoUnsignedWrap() && RBO->hasNoUnsignedWrap();

Ошибка Copy-Paste N4

Sema::DeduceTemplateArguments(...)
{
  ...
  if ((P->isPointerType() && A->isPointerType()) ||
      (P->isMemberPointerType() && P->isMemberPointerType()))
  ...
}

Диагностика PVS-Studio. V501 There are identical sub-expressions 'P->isMemberPointerType ()' to the left and to the right of the '&&' operator. clangSema sematemplatededuction.cpp 3240

Это просто случай, в отличие от пятого примера. Понятно, что хотели написать так:

(P->isMemberPointerType() && A->isMemberPointerType())

Ошибка Copy-Paste N5

static bool CollectBSwapParts(...) {
  ...
  // 2) The input and ultimate destinations must line up:
  // if byte 3 of an i32 is demanded, it needs to go into byte 0
  // of the result. This means that the byte needs to be shifted
  // until it lands in the right byte bucket. The shift amount
  // depends on the position: if the byte is coming from the
  // high part of the value (e.g. byte 3) then it must be shifted
  // right. If from the low part, it must be shifted left.
  unsigned DestByteNo = InputByteNo + OverallLeftShift;
  if (InputByteNo < ByteValues.size()/2) {
    if (ByteValues.size()-1-DestByteNo != InputByteNo)
      return true;
  } else {
    if (ByteValues.size()-1-DestByteNo != InputByteNo)
      return true;
  }
  ...
}

Диагностика PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. LLVMInstCombine instcombineandorxor.cpp 1387

А это пример сложной ситуации. Я даже не уверен до конца ошибка здесь или нет. Комментарий мне тоже не помогает. Здесь анализировать код должны создатели. Но всё-таки я думаю, что здесь пример ошибки при Copy-Paste.

Думаю, про Copy-Paste пока достаточно, ведь есть некоторое количество и других типов ошибок.

Вот, например, классическая ошибка в switch

void llvm::EmitAnyX86InstComments(...) {
  ...
  case X86::VPERMILPSri:
    DecodeVPERMILPSMask(4, MI->getOperand(2).getImm(),
                        ShuffleMask);
    Src1Name = getRegName(MI->getOperand(0).getReg());
  case X86::VPERMILPSYri:
    DecodeVPERMILPSMask(8, MI->getOperand(2).getImm(),
                        ShuffleMask);
    Src1Name = getRegName(MI->getOperand(0).getReg());
    break;
  ... 
}

Диагностика PVS-Studio: V519 The 'Src1Name' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 211, 215. LLVMX86AsmPrinter x86instcomments.cpp 215

В большом коде просто неизбежны подобные ошибки. Уж очень опасен оператор switch. Можно сколько угодно хорошо знать, как он работает, но всё равно забыть этот проклятый 'break'.

Есть ряд если и не ошибок, то явно бестолковых или подозрительных действий.

Бестолковое действие N1, которое может быть ошибкой

AsmToken AsmLexer::LexLineComment() {
  // FIXME: This is broken if we happen to a comment at
  // the end of a file, which was .included, and which
  // doesn't end with a newline.
  int CurChar = getNextChar();
  while (CurChar != '\n' && CurChar != '\n' && CurChar != EOF)
    CurChar = getNextChar();
  ...
}

Диагностика PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: CurChar != '\n' && CurChar != '\n' LLVMMCParser asmlexer.cpp 149

Скорее всего, вторая проверка CurChar != '\n' здесь лишняя. Но, возможно, это ошибка и должно быть написано:

while (CurChar != '\n' && CurChar != '\r' && CurChar != EOF)

Бестолковое действие N2, которое точно не является ошибкой

std::string ASTContext::getObjCEncodingForBlock(...) const {
  ...
  ParmOffset = PtrSize;
  // Argument types.
  ParmOffset = PtrSize;
  ...
}

Диагностика PVS-Studio: V519 The 'ParmOffset' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3953, 3956. clangAST astcontext.cpp 3956

Бестолковое действие N3, которое я затрудняюсь описать

static unsigned getTypeOfMaskedICmp(...)
{
  ...
  result |= (icmp_eq ? (FoldMskICmp_Mask_AllZeroes |
                        FoldMskICmp_Mask_AllZeroes |
                        FoldMskICmp_AMask_Mixed |
                        FoldMskICmp_BMask_Mixed)
                     : (FoldMskICmp_Mask_NotAllZeroes |
                        FoldMskICmp_Mask_NotAllZeroes |
                        FoldMskICmp_AMask_NotMixed |
                        FoldMskICmp_BMask_NotMixed));
  ...
}

Диагностика PVS-Studio:

V501 There are identical sub-expressions 'FoldMskICmp_Mask_AllZeroes' to the left and to the right of the '|' operator. LLVMInstCombine instcombineandorxor.cpp 505

V501 There are identical sub-expressions 'FoldMskICmp_Mask_NotAllZeroes' to the left and to the right of the '|' operator. LLVMInstCombine instcombineandorxor.cpp 509

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

Есть код, который просто потенциально опасен

Этот код будет работать до тех пор, пока два enum имеют схожую структуру.

enum LegalizeAction {
  Legal,
  Promote,
  Expand,
  Custom
};

enum LegalizeTypeAction {
  TypeLegal,
  TypePromoteInteger,
  TypeExpandInteger,
  ...
};

LegalizeTypeAction getTypeAction(...) const;

EVT getTypeToExpandTo(LLVMContext &Context, EVT VT) const {
  ...
  switch (getTypeAction(Context, VT)) {
  case Legal:
    return VT;
  case Expand:
  ...
}

Диагностика PVS-Studio:

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. LLVMAsmPrinter targetlowering.h 268

V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. LLVMAsmPrinter targetlowering.h 270

Имея TypeLegal созвучно Legal, а имя TypeExpandInteger созвучно Expand. Это и стало причиной опечатки. Код работает только потому, что повезло и значения этих имен совпадают.

Заключение

Страшно, когда в компиляторе ошибки находятся? :)

P.S.

Кажется, я поспешил хвалить Clang. Только что наткнулись на ситуацию, когда при препроцессировании он портит код. Имеем вот такой фрагмент в atlcore.h:

ATLASSUME(p != NULL); // Too .... here
if (*p == '\0')
  return const_cast<_CharType*>(p+1);

Препроцессор Clang превращает это в:

do { ((void)0);
#pragma warning(push)
#pragma warning(disable : 4548)
 do {__noop(!!(p != 0));} while((0,0)
#pragma warning(pop)
 ); } while(0); // Too .... here if (*p == '\0')
  return const_cast<_CharType*>(p+1);

Он разместил оператор 'if' после комментария и получилось, что "if (*p == '\0')" теперь тоже комментарий. В результате имеем некорректный код. Эх, нет счастья в жизни программистов.

Дополнительные ссылки

• Wikipedia. Clang. https://ru.wikipedia.org/wiki/Clang
• Библиотека VivaCore. http://www.viva64.com/ru/vivacore-library/