VPF::PVS-Studio vs Clang - Форум программистов

Thunderbolt

Дата 10.8.2011, 11:23 (ссылка)

(нет голосов)

Загрузка ...

DevRel

Профиль
Группа: Участник
Сообщений: 122
Регистрация: 7.11.2007
Где: Тула

Репутация: 10
Всего: 16

Мы случайно проверили проект 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. Это и стало причиной опечатки. Код работает только потому, что повезло и значения этих имен совпадают.

Заключение

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

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. http://www.viva64.com/go.php?url=713
Библиотека VivaCore. http://www.viva64.com/ru/vivacore-library/

--------------------

Карпов Андрей, DevRel в PVS-Studio.

bsa

Дата 10.8.2011, 11:50 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Модератор
Сообщений: 9185
Регистрация: 6.4.2006
Где: Москва, Россия

Репутация: 63
Всего: 196

Цитата(Thunderbolt @ 10.8.2011, 11:23

)

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

Думаю, что это явный баг.

--------------------

Правильно заданный вопрос - половина ответа
Отзывы о работе модератора.

borisbn

Дата 10.8.2011, 13:44 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Завсегдатай
Сообщений: 4875
Регистрация: 6.2.2010
Где: Ростов-на-Дону

Репутация: 22
Всего: 135

Цитата(bsa @ 10.8.2011, 11:50

)

Думаю, что это явный баг.

возможно из-за этого

Цитата(Thunderbolt @ 10.8.2011, 11:23

)

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

Thunderbolt, а сами себя проверяли (PVS-Studio'ей саму PVS-Studio'ю) ? smile

Если да, и были ошибки, то поделитесь опытом smile

--------------------

Женщины отличаются от программистов тем, что у них чары состоят из стрингов

bsa

Дата 10.8.2011, 14:03 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Модератор
Сообщений: 9185
Регистрация: 6.4.2006
Где: Москва, Россия

Репутация: 63
Всего: 196

Цитата(borisbn @ 10.8.2011, 13:44)

Цитата(bsa @ 10.8.2011, 11:50

)

Думаю, что это явный баг.

возможно из-за этого

Цитата(Thunderbolt @ 10.8.2011, 11:23

)

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

не думаю. Так как за '\r' всегда идет '\n'. А потом, обычно, файлы читаются в тестовом режиме, а это гарантирует преобразование '\r\n' в '\n'

--------------------

Правильно заданный вопрос - половина ответа
Отзывы о работе модератора.

Thunderbolt

Дата 10.8.2011, 14:31 (ссылка)

(нет голосов)

Загрузка ...

DevRel

Профиль
Группа: Участник
Сообщений: 122
Регистрация: 7.11.2007
Где: Тула

Репутация: 10
Всего: 16

Цитата(borisbn @ 10.8.2011, 13:44)

Thunderbolt, а сами себя проверяли (PVS-Studio'ей саму PVS-Studio'ю) ? smile

Если да, и были ошибки, то поделитесь опытом smile

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

--------------------

Карпов Андрей, DevRel в PVS-Studio.

0 Пользователей читают эту тему (0 Гостей и 0 Скрытых Пользователей)
0 Пользователей:
« Предыдущая тема \| C/C++: Общие вопросы \| Следующая тема »