LLVM — open-source проект с огромной кодовой базой. Лучший из лучших, если говорить о качестве кода, учитывая его размеры и открытость. Ведь кому, как не разработчикам инструментов для компиляторов, лучше знать о возможностях языка и правильном их использовании. Их код всегда на высоте, а найти ошибки в нём всегда вызов для нашего анализатора, который мы принимаем.
Пару месяцев назад у LLVM вышла в релиз 18-я версия, поэтому настало время в очередной раз убедиться в качестве их кода. Для тех, кому интересно, наши предыдущие статьи про проверку можно прочитать тут и тут.
Для нас это всегда особые проверки, ведь, по сути, статические анализаторы выполняют схожую с компиляторами работу для проведения анализа. А компиляторы также проводят статический анализ для выдачи предупреждений. Практически двоюродные братья. Но каждый из них хорош в своём деле. Доказательством этого как раз служит эта статья. Компилятор скомпилировал наш анализатор, причём так, чтобы тот работал (кстати, именно Clang, входящий в состав LLVM, и у нас даже есть статья о переходе на него с MSVC). Ну а наш анализатор нашёл ошибки в компиляторе. Чем не доказательство синергии?
Версия LLVM, на которой производилась проверка — LLVM 18.1.0.
Фрагмент N1
Пример, как в простыне из условий может появиться логическая ошибка, приводящая к недостижимому коду.
if (Tok->is(tok::hash)) {
// Start of a macro expansion.
First = Tok;
Tok = Next;
if (Tok)
Tok = Tok->getNextNonComment();
} else if (Tok->is(tok::hashhash)) {
// Concatenation. Skip.
Tok = Next;
if (Tok)
Tok = Tok->getNextNonComment();
} else if (Keywords.isVerilogQualifier(*Tok) ||
Keywords.isVerilogIdentifier(*Tok)) {
First = Tok;
Tok = Next;
// The name may have dots like `interface_foo.modport_foo`.
while (Tok && Tok->isOneOf(tok::period, tok::coloncolon) &&
(Tok = Tok->getNextNonComment())) {
if (Keywords.isVerilogIdentifier(*Tok))
Tok = Tok->getNextNonComment();
}
} else if (!Next) {
Tok = nullptr;
} else if (Tok->is(tok::l_paren)) {
// Make sure the parenthesized list is a drive strength. Otherwise the
// statement may be a module instantiation in which case we have already
// found the instance name.
if (Next->isOneOf(
Keywords.kw_highz0, Keywords.kw_highz1, Keywords.kw_large,
Keywords.kw_medium, Keywords.kw_pull0, Keywords.kw_pull1,
Keywords.kw_small, Keywords.kw_strong0, Keywords.kw_strong1,
Keywords.kw_supply0, Keywords.kw_supply1, Keywords.kw_weak0,
Keywords.kw_weak1)) {
Tok->setType(TT_VerilogStrength);
Tok = Tok->MatchingParen;
if (Tok) {
Tok->setType(TT_VerilogStrength);
Tok = Tok->getNextNonComment();
}
} else {
break;
}
} else if (Tok->is(tok::hash)) {
if (Next->is(tok::l_paren))
Next = Next->MatchingParen;
if (Next)
Tok = Next->getNextNonComment();
}Предупреждение анализатора:
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 3016, 3058. TokenAnnotator.cpp
Давайте посмотрим внимательнее. В первом условии стоит проверка Tok->is(tok::hash). В последнем условии в конструкции else if стоит аналогичная проверка. При этом токен, с которым работают, не меняется. Таким образом, код в последнем else if никогда не выполнится. В этом случае это критично, так как код под этими условиями разный.
if (Tok->is(tok::hash)) {
// Start of a macro expansion.
First = Tok;
Tok = Next;
if (Tok)
Tok = Tok->getNextNonComment();
} else
....
else if (Tok->is(tok::hash)) {
if (Next->is(tok::l_paren))
Next = Next->MatchingParen;
if (Next)
Tok = Next->getNextNonComment();
}Возможно, такую конструкцию стоило бы заменить на switch, и тогда проще было бы заметить ошибку, но это уже дело вкуса.
Фрагмент N2
Интересный фрагмент кода, которым я бы хотел с вами поделиться:
assert(bArgs.size() == reduc.size() + needsUniv ? 1 : 0);Предупреждения анализатора:
Предупреждения два, потому что этот фрагмент встречается в двух разных местах.
Анализатор говорит, что здесь возможно неправильное использование тернарного оператора. Давайте разбираться.
Для начала вспомним приоритет операторов. Приоритет в порядке убывания следующий: оператор +, оператор ==, тернарный оператор. Также необходимо отметить, что переменная needsUniv имеет тип bool.
Получается, что сначала произойдёт сложение результата работы reduc.size() и needsUniv. При этом needsUniv неявно кастанётся к типу size_t. Далее результат сложения сравнится с результатом работы bArgs.size(). Затем выполнится тернарный оператор, который вернёт либо 1, либо 0.
Согласитесь, выглядит немного странновато. Вероятнее всего, хотели написать такой код:
assert(bArgs.size() == reduc.size() + (needsUniv ? 1 : 0));В таком случае сначала выполнится тернарный оператор, который вернёт либо 1, либо 0. Затем это значение сложится с результатом работы reduc.size(), а затем произойдёт сравнение с результатом работы bArgs.size().
Занимательно, что результат работы и в первом, и во втором случае один и тот же.
Ещё занимательнее, что можно было написать так:
assert(bArgs.size() == reduc.size() + needsUniv)Результат и в этом случае был бы тот же, но уже без лишнего выполнения тернарного оператора.
В общем, интересный случай, когда код написан неправильно, но работает правильно.
Фрагмент N3
Довольно любопытное использование постфиксного инкремента. Внимание на второй аргумент кастомной функции Printf, вызываемой у объекта strm:
static void DumpTargetInfo(uint32_t target_idx, Target *target,
const char *prefix_cstr,
bool show_stopped_process_status, Stream &strm)
{
....
uint32_t properties = 0;
if (target_arch.IsValid())
{
strm.Printf("%sarch=", properties++ > 0 ? ", " : " ( ");
target_arch.DumpTriple(strm.AsRawOstream());
properties++;
}
}Предупреждение анализатора:
V547 Expression 'properties ++ > 0' is always false. CommandObjectTarget.cpp:100
На мой взгляд, здесь сначала хотели провести сравнение properties c нулём, и, чтобы дальше не писать лишний код с инкрементацией переменной properties, решили тут же воспользоваться постфиксным инкрементом.
Таким образом, для сравнения бы использовалось старое значение переменной, но при этом оно всё равно увеличилось бы на единицу. Но тогда непонятно, зачем дальше её ещё раз проинкрементировали.
А затем приходит понимание, что тернарный оператор здесь вообще не нужен. Возможно, когда-то этот фрагмент был в цикле. Напишите ваши варианты в комментариях.
Фрагменты N4-8
Рассмотрим следующий код и срабатывание PVS-Studio:
bool areStatementsIdentical(const Stmt *FirstStmt, const Stmt *SecondStmt,
const ASTContext &Context, bool Canonical)
{
....
if (FirstStmt->getStmtClass() != FirstStmt->getStmtClass())
return false;
....
}Предупреждение анализатора:
V501 There are identical sub-expressions 'FirstStmt->getStmtClass()' to the left and to the right of the '!=' operator. ASTUtils.cpp:99
Казалось бы, мелочь. Ну перепутали FirstStmt и SecondStmt, чего бухтеть-то?
А потом встречаем такой код:
static bool sameFunctionParameterTypeLists(Sema &S,
const OverloadCandidate &Cand1,
const OverloadCandidate &Cand2) {
if (!Cand1.Function || !Cand2.Function)
return false;
FunctionDecl *Fn1 = Cand1.Function;
FunctionDecl *Fn2 = Cand2.Function;
if (Fn1->isVariadic() != Fn1->isVariadic())
return false;
....
}Предупреждение анализатора:
V501 There are identical sub-expressions to the left and to the right of the '!=' operator: Fn1->isVariadic() != Fn1->isVariadic(). SemaOverload.cpp:10190
Думаешь: "Ну ошиблись второй раз, с кем не бывает".
А потом видишь такой код:
if (G1->Rank < G1->Rank)
G1->Group = G2;
else {
G2->Group = G1;
}Предупреждение анализатора:
V501 There are identical sub-expressions to the left and to the right of the '<' operator: G1->Rank < G1->Rank. SCCIterator.h:285
Начинают закрадываться сомнения.
А потом появляется такой фрагмент:
ValueBoundsConstraintSet::areOverlappingSlices(MLIRContext *ctx,
HyperrectangularSlice slice1,
HyperrectangularSlice slice2) {
assert(slice1.getMixedOffsets().size() == slice1.getMixedOffsets().size()
&& "expected slices of same rank");
assert(slice1.getMixedSizes().size() == slice1.getMixedSizes().size()
&& "expected slices of same rank");
assert(slice1.getMixedStrides().size() == slice1.getMixedStrides().size()
&& "expected slices of same rank");
....
}Предупреждения анализатора:
И ещё один почти такой же:
ValueBoundsConstraintSet::areEquivalentSlices(MLIRContext *ctx,
HyperrectangularSlice slice1,
HyperrectangularSlice slice2) {
assert(slice1.getMixedOffsets().size() == slice1.getMixedOffsets().size()
&& "expected slices of same rank");
assert(slice1.getMixedSizes().size() == slice1.getMixedSizes().size()
&& "expected slices of same rank");
assert(slice1.getMixedStrides().size() == slice1.getMixedStrides().size()
&& "expected slices of same rank");
....
}Предупреждения анализатора:
И удивляешься: неужели так много ошибок может тихо жить в коде? И ведь из-за каждого из таких мест что-то может отвалиться или работать не так, как задумано.
Кстати, видя все эти Fn1, G1, slice1, вспоминается статья коллеги "Ноль, один, два, Фредди заберёт тебя".
Ну и на закуску аналогичные предупреждения:
Фрагмент N9
А вот ещё интересный фрагмент кода. Попробуйте самостоятельно разобраться, где тут ошибка:
const Expr *CGOpenMPRuntime::getNumTeamsExprForTargetDirective(
CodeGenFunction &CGF, const OMPExecutableDirective &D, int32_t &MinTeamsVal,
int32_t &MaxTeamsVal)
{
....
if (isOpenMPParallelDirective(NestedDir->getDirectiveKind()) ||
isOpenMPSimdDirective(NestedDir->getDirectiveKind())) {
MinTeamsVal = MaxTeamsVal = 1;
return nullptr;
}
MinTeamsVal = MaxTeamsVal = 1;
return nullptr;
....
}Давайте посмотрим, что говорит анализатор.
Предупреждение анализатора:
V523 The 'then' statement is equivalent to the subsequent code fragment. CGOpenMPRuntime.cpp:6040, 6036
Он говорит, что код после if точно такой же, как и в then-ветке. Соответственно, здесь лишняя либо проверка, либо же часть кода после неё.
Фрагмент N10
Следующий фрагмент выглядит весьма странно:
explicit MapLattice(Container C) { C = std::move(C); }Предупреждение анализатора:
V570 The 'C' variable is assigned to itself. MapLattice.h:52
И неспроста. Внутри тела конструктора класса MapLattice произошло сокрытие нестатического поля с тем же именем, что и у параметра. И в этом фрагменте забыли явно указать this слева от оператора присваивания.
Небольшая правка, и код заработает так, как и ожидалось:
explicit MapLattice(Container C) { this->C = std::move(C); }Хотя, на мой взгляд, куда изящнее было бы воспользоваться списком инициализации конструктора:
explicit MapLattice(Container C) : C { std::move(C) } {};В таком случае сокрытия не происходит из-за правил поиска имён (тык, тык).
Ну а вообще, в имена приватных полей можно добавлять префикс или постфикс, чтобы было легче отличать их в коде от параметров.
Фрагмент N11
У вас когда-нибудь возникало чувство, будто вы что-то забыли? Например, вы не помните, взяли ли вы с собой ключи. Начинаете ощупывать карманы и не можете найти их.
Так вот, в следующем фрагменте как будто забыли использовать результат работы функции:
ScalarEvolution::getRangeRefIter(const SCEV *S,
ScalarEvolution::RangeSignHint SignHint)
{
....
for (const SCEV *P : reverse(drop_begin(WorkList))) {
getRangeRef(P, SignHint);
....
}
....
}Предупреждение анализатора:
V530 The return value of function 'getRangeRef' is required to be utilized. ScalarEvolution.cpp:6587
Можно заметить, что результат работы getRangeRef никак не используется.
А вот и сигнатура этой функции, которая подтверждает, что результат работы есть:
const ConstantRange &getRangeRef(const SCEV *S, RangeSignHint Hint,
unsigned Depth = 0);Да и далее по коду видно, что в других местах её результат используется.
Но не всё так просто. Иногда наши чувства нас подводят и оказывается, что ключи всё это время были прямо у нас в руке.
Над самим фрагментом есть такой комментарий:
// Use getRangeRef to compute ranges for items in the worklist in reverse
// order. This will force ranges for earlier operands to be computed before
// their users in most cases.Вероятнее всего, разработчики осознанно написали этот код. Другой вопрос в том, что использовать таким образом функцию с названием get и, возможно, заставлять её выполнять несколько дел одновременно — плохая практика, которая может ввести в заблуждение других разработчиков.
Таким образом, формально анализатор оказался прав, но фактически ошибки здесь нет. Для таких случаев у нас есть способы подавления отдельных срабатываний. Например, с помощью комментария в коде:
for (const SCEV *P : reverse(drop_begin(WorkList))) {
getRangeRef(P, SignHint); //-V530Фрагмент N12
Для самых внимательных. Найдите три отличия кода в if и else ветках. Ладно, хотя бы одно:
case OptionParser::eOptionalArgument:
if (OptionParser::GetOptionArgument() != nullptr) {
option_element_vector.push_back(OptionArgElement(
opt_defs_index,
FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 2],
args),
FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
args)));
} else {
option_element_vector.push_back(OptionArgElement(
opt_defs_index,
FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 2],
args),
FindOriginalIndex(dummy_vec[OptionParser::GetOptionIndex() - 1],
args)));
}И вы абсолютно правы. Их нет.
Предупреждение анализатора:
V523 The 'then' statement is equivalent to the 'else' statement. Options.cpp 1212
Заключение
Всё хорошее имеет свойство заканчиваться. Но к статье это не относится. Большому проекту — большая статья, а ещё лучше две. В этот раз я решил придерживаться второго варианта. В следующей части будут рассмотрены ошибки гораздо серьёзнее, чем в этой (спойлер: связанные с UB).
Даже лучшие из нас совершают ошибки (и не только в коде). Что уж говорить про рядовых пользователей. Но ошибки это не повод расстраиваться. Ошибки — это повод становиться лучше и расти (можете говорить это вашему тимлиду каждый раз, когда что-то отваливается на проде).
Ну а чтобы избегать ошибок в коде, нужно пробовать разные способы их поиска. Например, помимо покрытия кода тестами и code review можно использовать инструменты динамического и статического анализа кода.
Интересно узнать, какие ошибки есть в вашем коде? Тогда вы можете проверить это бесплатно.