Святослав Размыслов

02 Дек 2019

Теги:

#Cpp #Compiler

Проверка кода компилятора Ark Compiler, недавно открытого компанией Huawei

02 Дек 2019

Автор: Святослав Размыслов

Введение
Обзор дефектов кода
Заключение
Дополнительные ссылки

Во время презентаций летом 2019 года Huawei анонсировала технологию Ark Compiler. По заверениям представителей компании, этот проект с открытым исходным кодом позволяет существенно повысить плавность и отзывчивость Android и сторонних приложений. Новый интересный открытый проект по традиции должен пройти проверку качества кода с помощью PVS-Studio.

Введение

Впервые компилятор Huawei Ark был представлен вместе с запуском смартфонов Huawei P30 и P30 Pro. По заявлению Huawei, компилятор Ark повышает плавность работы Android на 24%, а скорость отклика – на 44%. При этом сторонние приложения для Android, после перекомпиляции с помощью Ark, могут работать на 60% быстрее. Открытый проект имеет название OpenArkCompiler. Его исходный код доступен на китайском аналоге сайта GitHub – Gitee.

Для проверки проекта использовался статический анализатор кода – PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java.

Анализатор быстро справился с проектом на 50К строк кода. Для маленького проекта и результаты анализа скромные: в статью вошло 11 предупреждений из 39 (уровней High и Medium).

Обзор дефектов кода

Предупреждение 1

V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '==' operator. mir_parser.cpp 884

enum Opcode : uint8 {
  kOpUndef,
  ....
  OP_intrinsiccall,
  OP_intrinsiccallassigned,
  ....
  kOpLast,
};

bool MIRParser::ParseStmtIntrinsiccall(StmtNodePtr &stmt, bool isAssigned) {
  Opcode o = !isAssigned ? (....)
                         : (....);
  auto *intrnCallNode = mod.CurFuncCodeMemPool()->New<IntrinsiccallNode>(....);
  lexer.NextToken();
  if (o == !isAssigned ? OP_intrinsiccall : OP_intrinsiccallassigned) {
    intrnCallNode->SetIntrinsic(GetIntrinsicID(lexer.GetTokenKind()));
  } else {
    intrnCallNode->SetIntrinsic(static_cast<MIRIntrinsicID>(....));
  }
  ....
}

Нам интересна следующая часть этого кода:

if (o == !isAssigned ? OP_intrinsiccall : OP_intrinsiccallassigned) {
  ....
}

Оператор '==' имеет более высокий приоритет, чем тернарный оператор (?:). Следовательно, условное выражение вычисляется неправильно. Написанный код эквивалентен следующему:

if ((o == !isAssigned) ? OP_intrinsiccall : OP_intrinsiccallassigned) {
  ....
}

А с учётом того, что константы OP_intrinsiccall и OP_intrinsiccallassigned имеют ненулевые значения, то это условие всегда возвращает истинное значение. Тело ветки else является недостижимым кодом.

Предупреждение 2

V570 The 'theDoubleVal' variable is assigned to itself. lexer.cpp 283

int64 theIntVal = 0;
float theFloatVal = 0.0;
double theDoubleVal = 0.0;

TokenKind MIRLexer
::GetFloatConst(uint32 valStart, uint32 startIdx, bool negative) {
  ....
  theIntVal = static_cast<int>(theFloatVal);
  theDoubleVal = static_cast<double>(theDoubleVal); // <=
  if (theFloatVal == -0) {
    theDoubleVal = -theDoubleVal;
  }
  ....
}

Переменная theDoubleVal присваивается сама себе, при этом никак не изменяясь. Скорее всего, хотели записать результат в переменную theFloatVal. Именно эта переменная затем используется в условии. В этом случае и приведение типа должно быть к float, а не к double. Рискну предположить, что код должен быть таким:

theFloatVal = static_cast<float>(theDoubleVal);
if (theFloatVal == -0) {
  theDoubleVal = -theDoubleVal;

или даже таким, если просто перепутали переменную в условии:

if (theDoubleVal == -0) {
  theDoubleVal = -theDoubleVal;

Хотя, возможно, я не прав, и всё должно быть по-другому. Код выглядит весьма непонятно для стороннего программиста, такого как я.

Предупреждения 3-5

V524 It is odd that the body of '-' function is fully equivalent to the body of '+' function. mpl_number.h 158

template <typename T, typename Tag>
inline Number<T, Tag> operator+(const Number<T, Tag> &lhs,
                                const Number<T, Tag> &rhs) {
  return Number<T, Tag>(lhs.get() + rhs.get());
}

template <typename T, typename Tag>
inline Number<T, Tag> operator-(const Number<T, Tag> &lhs,
                                const Number<T, Tag> &rhs) {
  return Number<T, Tag>(lhs.get() + rhs.get());
}

В заголовочном файле mpl_number.h продублировали много кода с незначительными изменениями. И, конечно, допустили ошибки. В этом примере операторы сложения и вычитания реализованы одинаково. В теле оператора вычитания забыли поменять знак операции.

Ещё несколько примеров приведу списком:

V524 It is odd that the body of '-' function is fully equivalent to the body of '+' function. mpl_number.h 233
V524 It is odd that the body of '-' function is fully equivalent to the body of '+' function. mpl_number.h 238

Предупреждение 6

V560 A part of conditional expression is always false: !firstImport. parser.cpp 2633

bool MIRParser::ParseMIRForImport() {
  ....
  if (paramIsIPA && firstImport) {
    BinaryMplt *binMplt = new BinaryMplt(mod);
    mod.SetBinMplt(binMplt);
    if (!(*binMplt).Import(...., paramIsIPA && !firstImport, paramIsComb)) {
      ....
    }
    ....
  }
  ....
}

В теле первого условного выражения переменная firstImport всегда имеет значение true. В этом случае выражение

paramIsIPA && !firstImport

всегда будет иметь значение false. Этот фрагмент кода либо содержит логическую ошибку, либо его можно упростить, передав константу false в функцию Import.

Предупреждение 7

V547 Expression 'idx >= 0' is always true. Unsigned type value is always >= 0. lexer.h 129

char GetCharAtWithLowerCheck(uint32 idx) const {
  return idx >= 0 ? line[idx] : 0;
}

Проверка переменной-индекса idx таким образом (>= 0) не имеет никакого смысла, так как это беззнаковый тип. Возможно, здесь стоит добавить проверку другой границы доступа к массиву line, либо просто удалить эту бессмысленную проверку.

Предупреждение 8

V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'c != '\"'' and 'c == '\"''. lexer.cpp 400

TokenKind MIRLexer::GetTokenWithPrefixDoubleQuotation() {
  ....
  char c = GetCurrentCharWithUpperCheck();
  while ((c != 0) &&
         (c != '\"' || (c == '\"' && GetCharAtWithLowerCheck(....) == '\\'))) {
    ....
  }
  ....
}

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

A || (!A && smth)

Выражение !A будет всегда иметь значение true. Тогда исходный пример можно упростить до такого:

while ((c != 0) && (c != '\"' || (GetCharAtWithLowerCheck(....) == '\\'))) {
  ....
}

Предупреждения 9-10

V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. mir_nodes.cpp 1552

bool BinaryNode::Verify() const {
  ....
  if ((IsAddress(GetBOpnd(0)->GetPrimType()) &&
      !IsAddress(GetBOpnd(1)->GetPrimType()))
    ||
     (!IsAddress(GetBOpnd(0)->GetPrimType()) &&
       IsAddress(GetBOpnd(1)->GetPrimType()))) {
    ....
  }
  ....
}

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

if (IsAddress(GetBOpnd(0)->GetPrimType()) !=
    IsAddress(GetBOpnd(1)->GetPrimType()))
  ....
}

Ещё одно место, где можно провести аналогичный рефакторинг:

V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression. bin_mpl_import.cpp 702

Предупреждение 11

V1048 The 'floatSpec->floatStr' variable was assigned the same value. input.inl 1356

static void SecInitFloatSpec(SecFloatSpec *floatSpec)
{
  floatSpec->floatStr = floatSpec->buffer;
  floatSpec->allocatedFloatStr = NULL;
  floatSpec->floatStrSize = sizeof(floatSpec->buffer) /
                            sizeof(floatSpec->buffer[0]);
  floatSpec->floatStr = floatSpec->buffer;
  floatSpec->floatStrUsedLen = 0;
}

Анализатор обнаружил 2 одинаковые строки инициализации переменной floatSpec->floatStr. Скорее всего, лишнюю строку можно удалить.

Заключение

Совсем недавно мы делали обзор кода Huawei Cloud DIS SDK. Компания Huawei начала активно открывать код для общественности, что не может не радовать сообщество разработчиков. Такие проекты, как Ark Compiler или Harmony OS, только появились и ещё не стали массовыми. Вложиться в контроль качества кода проектов на этом этапе будет очень выгодным, так как можно избежать появления потенциальных уязвимостей и критики пользователей.