Мы используем куки, чтобы пользоваться сайтом
было удобно.
С последней проверки проекта LLVM инструментом PVS-Studio прошло уже больше года и аж два релиза, так что пришло время снова стать трудолюбивой птичкой и покопаться в свежей версии LLVM 21.
Думаю, нет C++ программиста, что не слышал про LLVM, но на всякий случай сделаю краткий дисклеймер. LLVM — гигантский зонтичный open source проект, содержащий огромный набор различных инструментов для написания оптимизирующих компиляторов любой сложности.
В нашей проверке участвовал код только непосредственно LLVM (без Clang, libc, lldb и т.д.). За основу взят коммит 253d18d032bb main-ветки. Ошибки брались из High и Medium уровней группы общего назначения.
Приведённые ссылки на конкретные коммиты добавлены для упрощения верифицирования и исправления найденных ошибок и не имеют цели скомпрометировать или каким-то образом принизить их авторов.
Ну-с, с вводной покончено, погнали наконец за работу.
Фрагмент N1
Предупреждение PVS-Studio: V501 There are identical sub-expressions '!Subtarget.hasZeroCycleRegMoveFPR64()' to the left and to the right of the '&&' operator. AArch64InstrInfo.cpp 5430
void AArch64InstrInfo::copyPhysReg(....) const {
....
if (Subtarget.hasZeroCycleRegMoveFPR128() &&
!Subtarget.hasZeroCycleRegMoveFPR64() &&
!Subtarget.hasZeroCycleRegMoveFPR64() && Subtarget.isNeonAvailable())
....
}Начнём расследование, расчехлив наш главный на сегодня инструмент — git blame. В нём можно заметить, что кроме этого подозрительного места в коммите добавили ещё четыре идентичные проверки:
if (Subtarget.hasZeroCycleRegMoveFPR128() &&
!Subtarget.hasZeroCycleRegMoveFPR64() &&
!Subtarget.hasZeroCycleRegMoveFPR32() && ....Если присмотреться, то в blame у всех них можно увидеть такую структуру:
if (Subtarget.hasZeroCycleRegMoveFPR128() &&
!Subtarget.hasZeroCycleRegMoveFPR64() &&
!Subtarget.hasZeroCycleRegMoveFPR32() && Subtarget.isNeonAvailable()) {
/*
* new code
*/
}
else if (Subtarget.hasZeroCycleRegMoveFPR64() &&
!Subtarget.hasZeroCycleRegMoveFPR32())
/*
* existing code
*/
}Ошибка копипасты налицо, но на всякий случай проверим, что hasZeroCycleRegMoveFPR64 не имеет побочных эффектов, ради которых его могли бы вызвать дважды. Ну а вдруг. Утверждая что-то о таком большом и сложном проекте, как LLVM, всегда стоит быть максимально осторожным. Итак, реализация лежит в файле AArch64GenSubtargetInfo.inc, сгенерированном с помощью внутреннего DSL TableGen:
GET_SUBTARGETINFO_MACRO(HasZeroCycleRegMoveFPR64,
false,
hasZeroCycleRegMoveFPR64)Макрос определён в соседнем файле AArch64Subtarget.h:
class AArch64Subtarget final : public AArch64GenSubtargetInfo {
// Bool members corresponding to the SubtargetFeatures defined in tablegen
#define GET_SUBTARGETINFO_MACRO(ATTRIBUTE, DEFAULT, GETTER)\
bool ATTRIBUTE = DEFAULT;
#include "AArch64GenSubtargetInfo.inc"
// Getters for SubtargetFeatures defined in tablegen
#define GET_SUBTARGETINFO_MACRO(ATTRIBUTE, DEFAULT, GETTER)\
bool GETTER() const { return ATTRIBUTE; }
#include "AArch64GenSubtargetInfo.inc"
....
}Как видим, макрос разворачивается в простейшие геттер-сеттер, и теперь сомнений не осталось: нужно заменить второе hasZeroCycleRegMoveFPR64 на hasZeroCycleRegMoveFPR32.
Фрагмент N2
Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '==' operator: PeekArg.getValNo() == PeekArg.getValNo() PPCISelLowering.cpp 7865
SDValue PPCTargetLowering::LowerCall_AIX(....) const {
....
for (unsigned I = 0, E = ArgLocs.size(); I != E;) {
....
CCValAssign &GPR1 = VA;
....
if (I != E) {
// If only 1 GPR was available, there will only be one custom GPR and
// the argument will also pass in memory.
CCValAssign &PeekArg = ArgLocs[I];
if (PeekArg.isRegLoc() && PeekArg.getValNo() == PeekArg.getValNo()) // <=
{
assert(PeekArg.needsCustom() && "A second custom GPR is expected.");
CCValAssign &GPR2 = ArgLocs[I++];
RegsToPass.push_back(std::make_pair(GPR2.getLocReg(),
DAG.getZExtOrTrunc(ArgAsInt, dl, MVT::i32)));
}
}
....
}Предварительный диагноз — ещё одна жертва копипасты. Сразу же проверяем getValNo на наличие побочных эффектов:
class CCValAssign{
....
unsigned ValNo;
unsigned getValNo() const { return ValNo; }
}Что ж, ничего необычного. Посмотрим код до последнего затронувшего его коммита:
CCValAssign &GPR1 = VA;
....
assert(I != E && "A second custom GPR is expected!");
CCValAssign &GPR2 = ArgLocs[I++];
assert(GPR2.isRegLoc() && GPR2.getValNo() == GPR1.getValNo() &&
GPR2.needsCustom() && "A second custom GPR is expected!");
RegsToPass.push_back(
std::make_pair(GPR2.getLocReg(),
DAG.getZExtOrTrunc(ArgAsInt, dl, MVT::i32)));Суть понятна: исключительный случай, ранее прикрывавшийся assert, редизайнился в рядовое ветвление. Об этом говорит и текст коммита:
This patch implements the caller side of placing function call arguments
in stack memory. This removes the current limitation where LLVM on AIX
will report fatal error when arguments can't be contained in registers.
Можно заметить, что кроме найденной ошибки есть ещё странное присваивание:
CCValAssign &PeekArg = ArgLocs[I];
....
CCValAssign &GPR2 = ArgLocs[I++]; // здесь PeekArg == GPR2Скорее всего, автор хотел написать вот так:
if (I != E) {
CCValAssign &GPR2 = ArgLocs[I];
if (GPR2.isRegLoc() && PeekArg.getValNo() == GPR1.getValNo())
{
assert(PeekArg.needsCustom() && "A second custom GPR is expected.");
I++;
RegsToPass.push_back(std::make_pair(
GPR2.getLocReg(), DAG.getZExtOrTrunc(ArgAsInt, dl, MVT::i32)));
}
}Но для больший выразительности разделил GPR2 на PeekArg, дабы показать, что в отличие от безусловного прежнего кода нужно сперва "подсмотреть" аргумент. А в процессе копипасты потерял GPR1 в условии.
Думаю, корректная версия if должна выглядеть так:
if (PeekArg.isRegLoc() && PeekArg.getValNo() == GPR1.getValNo())Что интересно, до недавнего полного переезда на GitHub у LLVM был отдельный сайт для код-ревью, и в коммите есть ссылка на него. Там можно заметить, что ручное ревью не всегда спасает:
Фрагмент N3
Предупреждение PVS-Studio: V557 Array overrun is possible. The value of 'regIdx' index could reach 31. VEAsmParser.cpp 696
static const MCPhysReg MISCRegs[31] = { .... };
static bool MorphToMISCReg(VEOperand &Op) {
const auto *ConstExpr = dyn_cast<MCConstantExpr>(Op.getImm());
if (!ConstExpr)
return false;
unsigned regIdx = ConstExpr->getValue();
if (regIdx > 31 || MISCRegs[regIdx] == VE::NoRegister)
return false;
Op.Kind = k_Register;
Op.Reg.RegNum = MISCRegs[regIdx];
return true;
}Как известно, UB для компиляторов не существует. Но, к сожалению, это относится только к коду обрабатываемых программ, код же самого компилятора всё ещё им подвержен. А тут мы видим классическое UB в случае, если regIdx == 31. Скорее всего, такой вход невозможен или очень мало вероятен, поэтому ошибка прошла незамеченной. Но кто знает, что будет завтра?
Осталось выяснить локацию ошибки: неверное число в условии или в массив забыли добавить элемент?
Функция находится в коде парсера Vector Engine Assembly Language и, судя по всему, проверяет, что непосредственный операнд инструкции SMIR валиден. Смотрим документацию:
SMIR — Save Miscellaneous Register
smir %sx, I
smir %sx, %usrcc
smir %sx, %psw
smir %sx, %sar
smir %sx, %pmmr
smir %sx, %pmcrMM
smir %sx, %pmcNN
I = 0 – 2, 7 – 11, 16 - 30
MM = 0 – 3
NN = 0 - 14Нас интересует первый вариант, smir %sx, I, где валидными являются значения I = 0 – 2, 7 – 11, 16 – 30. Сверяем с массивом:
static const MCPhysReg MISCRegs[31] = {
VE::USRCC, VE::PSW, VE::SAR, VE::NoRegister,
VE::NoRegister, VE::NoRegister, VE::NoRegister, VE::PMMR,
VE::PMCR0, VE::PMCR1, VE::PMCR2, VE::PMCR3,
VE::NoRegister, VE::NoRegister, VE::NoRegister, VE::NoRegister,
VE::PMC0, VE::PMC1, VE::PMC2, VE::PMC3,
VE::PMC4, VE::PMC5, VE::PMC6, VE::PMC7,
VE::PMC8, VE::PMC9, VE::PMC10, VE::PMC11,
VE::PMC12, VE::PMC13, VE::PMC14};Всё совпадает, значит, каких-то двусмысленностей больше быть не может: как минимум нужно заменить regIdx > 31 на regIdx > 30, а ещё лучше на regIdx >= std::size(MISCRegs).
Любопытно, что в этом же коммите была добавлена похожая функция, в которой проверка сделана правильно:
static const unsigned MiscRegDecoderTable[] = {....}; // <= тоже 31 элемент
static DecodeStatus DecodeMISCRegisterClass(MCInst &Inst, unsigned RegNo,
uint64_t Address,
const void *Decoder) {
if (RegNo > 30)
return MCDisassembler::Fail;
unsigned Reg = MiscRegDecoderTable[RegNo];
if (Reg == VE::NoRegister)
return MCDisassembler::Fail;
Inst.addOperand(MCOperand::createReg(Reg));
return MCDisassembler::Success;
}Видимо, автора ввело в заблуждение явное число 31 в объявлении массива MISCRegs.
Фрагмент N4
Предупреждение PVS-Studio: V519 The 'FeaturesValue' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1190, 1192. LVCodeViewReader.cpp 1192
Error LVCodeViewReader::loadTargetInfo(const ObjectFile &Obj) {
....
Expected<SubtargetFeatures> Features = Obj.getFeatures();
SubtargetFeatures FeaturesValue;
if (!Features) {
consumeError(Features.takeError());
FeaturesValue = SubtargetFeatures();
}
FeaturesValue = *Features;
....
return loadGenericTargetInfo(TT.str(), FeaturesValue.getString(), CPU);
}Выглядит как явный dead store, но, может быть, первое присваивание делают ради побочных эффектов? Смотрим:
class SubtargetFeatures {
std::vector<std::string> Features;
....
SubtargetFeatures::SubtargetFeatures(StringRef Initial = "") {
// Break up string into separate features
Split(this->Features, Initial);
}
}Здесь функция Split разбивает строку (в нашем случае пустую) по запятым и записывает во внутренний вектор.
И можно было бы записывать это срабатывание в раздел мелких неоптимальностей, если бы не функция Expected<T>::takeError:
template <class T> class [[nodiscard]] Expected {
....
/// Take ownership of the stored error.
/// After calling this the Expected<T> is in an indeterminate state that can
/// only be safely destructed. No further calls (beside the destructor) should
/// be made on the Expected<T> value.
Error takeError() {
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
Unchecked = false;
#endif
return HasError ? Error(std::move(*getErrorStorage())) : Error::success();
}
}Спасибо подробнейшим комментариям в LLVM, которых, по ощущениям, не меньше, чем кода.
Теперь можно проследить путь потенциального UB. Глянем сперва на то, что вызовется при проверке условия if:
/// Return false if there is an error.
explicit operator bool() {
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
Unchecked = HasError;
#endif
return !HasError;
}Если Features содержит ошибку, мы провалимся в ветку then:
Error LVCodeViewReader::loadTargetInfo(const ObjectFile &Obj) {
....
if (!Features) {
consumeError(Features.takeError());
FeaturesValue = SubtargetFeatures();
}
FeaturesValue = *Features;
....
return loadGenericTargetInfo(TT.str(), FeaturesValue.getString(), CPU);
}Далее произойдёт передача владения внутренним полем Features, и оно останется в неопределённом состоянии. После этого происходит его разыменование:
reference operator*() const {
assertIsChecked();
return *getStorage();
}Конечно, ещё есть последний рубеж обороны в виде assertIsChecked, но, к сожалению, он используется только для проверки после значительных изменений в ABI.
Дополнение. Как подметили в комментариях, Unchecked == false после выполнения takeError, а значит, сообщения об ошибке не будет, даже если включён LLVM_ENABLE_ABI_BREAKING_CHECKS.
void assertIsChecked() const {
/* Define to enable checks that alter the LLVM C++ ABI */
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
if (LLVM_UNLIKELY(Unchecked))
fatalUncheckedExpected();
#endif
}Файл добавили в этом коммите, где также изменили огромное количество кода — 45 файлов и больше 7 тысяч строк, и по высокоуровневому описанию в тексте сложно понять детали реализации. С высокой долей вероятности можно утверждать, что здесь автор забыл вставить ветку else, и код должен выглядеть как-то так:
if (!Features) {
consumeError(Features.takeError());
FeaturesValue = SubtargetFeatures();
}
else
FeaturesValue = *Features;В таком варианте исправляется непосредственная ошибка. Но и тут есть что улучшить: первое присвоение FeaturesValue всё ещё бессмысленно, т. к. просто дважды вызывается конструктор по умолчанию. Хотя конкретно в этом случае это дало возможность найти более серьёзную потенциальную ошибку.
Более корректным вариантом будет:
Expected<SubtargetFeatures> Features = Obj.getFeatures();
SubtargetFeatures FeaturesValue;
if (!Features) {
consumeError(Features.takeError());
} else {
FeaturesValue = *Features;
}Можно зайти ещё дальше и улучшить читаемость, написав, например, так:
SubtargetFeatures FeaturesValue;
if (Expected<SubtargetFeatures> Features = Obj.getFeatures())
FeaturesValue = std::move(*Features);
else
consumeError(Features.takeError());Фрагмент N5
Предупреждение PVS-Studio: V609 Divide by zero. Denominator range [0..255]. RISCVCustomBehaviour.cpp 268
unsigned RISCVInstrumentManager::getSchedClassID(....) const {
....
// getBaseInfo works with (Opcode, LMUL, 0) if no SEW instrument,
// or (Opcode, LMUL, SEW) if SEW instrument is active,
// and depends on LMUL and SEW,
// or (Opcode, LMUL, 0) if does not depend on SEW.
uint8_t SEW = SI ? SI->getSEW() : 0;
if (const auto *VXMO = RISCV::getVXMemOpInfo(Opcode)) {
// Calculate the expected index EMUL. For indexed operations,
// the DataEEW and DataEMUL are equal to
// SEW and LMUL, respectively.
unsigned IndexEMUL = ((1 << VXMO->Log2IdxEEW) * LMUL) / SEW;
....
}
else ....
....
}Комментарий к переменной SEW даёт понять, что её равенство нулю — вполне валидный сценарий, что делает весьма странным немедленное бесстрашное потенциальное деление на ноль. Так что копилка UB пополняется третьим экземпляром.
Раскопки в git blame показывают, что if и его then ветка, где и происходит деление, появились позже, чем расчёт SEW и else ветка, которая тогда была единственным и безусловным вариантом выполнения.
Как видим, в этом исконном пути SEW == 0 — действительно нормальное поведение:
// Check if it depends on LMUL and SEW
const auto *RVV = RISCVVInversePseudosTable::getBaseInfo(Opcode, LMUL, SEW);
// Check if it depends only on LMUL
if (!RVV)
RVV = RISCVVInversePseudosTable::getBaseInfo(Opcode, LMUL, 0);Не буду приводить код getBaseInfo, но можете поверить (или проверить!), что там нет никаких опасных операций с нулём, только большая пачка разнообразных сравнений полей. Осталось проверить, что условия, когда SEW == 0 и когда мы делим на него несовместны, т. е. что не может случится так:
!SI && RISCV::getVXMemOpInfo(Opcode)Раскрываем getVXMemOpInfo и снова попадаем в файл TableGen (RISCVGenSearchableTables.inc):
const VXMemOpInfo *getVXMemOpInfo(unsigned BaseInstr) {
struct KeyType {
unsigned BaseInstr;
};
KeyType Key = {BaseInstr};
struct Comp {
bool operator()(const VXMemOpInfo &LHS,
const KeyType &RHS) const {
if (LHS.BaseInstr < RHS.BaseInstr)
return true;
if (LHS.BaseInstr > RHS.BaseInstr)
return false;
return false;
}
};
auto Table = ArrayRef(RISCVBaseVXMemOpTable);
auto Idx = std::lower_bound(Table.begin(),
Table.end(),
Key,
Comp());
if (Idx == Table.end() ||
Key.BaseInstr != Idx->BaseInstr)
return nullptr;
return &*Idx;
}Выглядит объёмно, но суть проста: для кода операции проверяется, есть ли он в таблице RISCVBaseVXMemOpTable. Она, судя по всему, представляет индексные векторные операции RISC V:
constexpr VXMemOpInfo RISCVBaseVXMemOpTable[] = {
{ 0x4, 0x1, 0x0, 0x0, VLOXEI16_V }, // 0
{ 0x5, 0x1, 0x0, 0x0, VLOXEI32_V }, // 1
....
{ 0x5, 0x0, 0x1, 0x8, VSUXSEG8EI32_V }, // 98
{ 0x3, 0x0, 0x1, 0x8, VSUXSEG8EI8_V }, // 99
};Теперь посмотрим, когда SI == nullptr:
// Unpack all possible RISC-V instruments from IVec.
RISCVLMULInstrument *LI = nullptr;
RISCVSEWInstrument *SI = nullptr;
for (auto &I : IVec) {
if (I->getDesc() == RISCVLMULInstrument::DESC_NAME)
LI = static_cast<RISCVLMULInstrument *>(I);
else if (I->getDesc() == RISCVSEWInstrument::DESC_NAME)
SI = static_cast<RISCVSEWInstrument *>(I);
}Значит, UB произойдёт, если одновременно в качестве аргументов пришли код векторной операции и набор инструментов без RISCVSEWInstrument.
Исходя из логики и документации по RISC V, такой сценарий выглядит невозможным, но точно это может сказать только компетентный в этом бэкенде программист и, скорее всего, только после тщательного разбирательства. Но лучше, конечно, явно исправить потенциально серьёзную ошибку либо прямой проверкой в духе if (SEW != 0), либо, если это действительно невозможно, с помощью assert. Так сделано в другой ветке else этого же каскада проверок (внутри getEEWAndEMUL):
assert(SEW >= 8 && "Unexpected SEW value")Фрагмент N6
Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. VEISelDAGToDAG.cpp 321
bool VEDAGToDAGISel::SelectInlineAsmMemoryOperand(
const SDValue &Op,
InlineAsm::ConstraintCode ConstraintID,
std::vector<SDValue> &OutOps) {
....
case InlineAsm::ConstraintCode::m:
// Try to match ADDRri since reg+imm style is safe
// for all VE instructions with a memory operand.
if (selectADDRri(Op, Op0, Op1)) {
OutOps.push_back(Op0);
OutOps.push_back(Op1);
return false;
}
// Otherwise, require the address to be in a register and immediate 0.
OutOps.push_back(Op);
OutOps.push_back(CurDAG->getTargetConstant(0, SDLoc(Op), MVT::i32));
return false;
....
}С первого взгляда непонятно, откуда взялось предупреждение про недостижимый код, но стоит взглянуть на внутренности функции selectADDRri, как всё встаёт на места:
bool VEDAGToDAGISel::selectADDRri(SDValue Addr,
SDValue &Base,
SDValue &Offset) {
if (matchADDRri(Addr, Base, Offset))
return true;
Base = Addr;
Offset = CurDAG->getTargetConstant(0, SDLoc(Addr), MVT::i32);
return true;
}Достаём git blame. Возвращаемый результат selectADDRri ранее был более разнообразным (оставлены только возвраты):
bool VEDAGToDAGISel::SelectADDRri(....) {
if (....) {
....
return true;
}
if (....)
return false;
if (....) {
....
return true;
}
....
return true;
}Впоследствии это тело переехало в функцию matchADDRri, а selectADDRri получило нынешнее.
И, казалось бы, всё просто: неудачный рефакторинг, в котором изменили смысл функции, не озаботившись проверить места использования. Но git blame даёт обратное: 06.04.2020 изменили тело selectADDRri, а через два года, 23.08.2022, тот же самый разработчик использовал её в if. И даже можно было бы решить, что он знает, что делает, если бы не комментарии. Например, какой смысл писать "Otherwise", если ты знаешь, что код недостижим?
// Try to match ADDRri since reg+imm style is safe
// for all VE instructions with a memory operand.
if (selectADDRri(Op, Op0, Op1)) {
....
return false;
}
// Otherwise, require the address to be in a register and immediate 0.Эти же комментарии дают подсказку: "try to match". А что, если вместо selectADDRri должна была вызываться matchADDRri?
Подставим и сравним:
if (matchADDRri(Op, Op0, Op1)) {
OutOps.push_back(Op0);
OutOps.push_back(Op1);
return false;
}
OutOps.push_back(Op);
OutOps.push_back(CurDAG->getTargetConstant(0, SDLoc(Op), MVT::i32));Если matchADDRri вернул true, то в вектор положили Op1 и Op2. Иначе кладём Op и константу 0.
А что там будет в selectADDRri, если заменить имена формальных параметров на фактические?
bool VEDAGToDAGISel::selectADDRri(SDValue Op,
SDValue &Op1,
SDValue &Op2) {
if (matchADDRri(Op, Op1, Op2))
return true;
Op1 = Op;
Op2 = CurDAG->getTargetConstant(0, SDLoc(Addr), MVT::i32);
return true;
}В этом случае, если matchADDRri вернёт true, то в Op и Op2 будут значения, возвращённые этой функцией. Если нет, то Op и константа 0. Ничего не напоминает? Возможно, автор "подсматривал" алгоритм в selectADDRri, а в конце случайно вставил название источника вдохновения вместо matchADDRri.
Фрагмент N7
Предупреждение PVS-Studio: V560 A part of conditional expression is always false: Opc == ARM::MVE_VORR. ARMBaseInstrInfo.cpp 752
void ARMBaseInstrInfo::copyPhysReg(....) const {
....
unsigned Opc = 0;
if (SPRDest && SPRSrc)
Opc = ARM::VMOVS;
else if (GPRDest && SPRSrc)
Opc = ARM::VMOVRS;
else if (SPRDest && GPRSrc)
Opc = ARM::VMOVSR;
else if (ARM::DPRRegClass.contains(DestReg, SrcReg) && Subtarget.hasFP64())
Opc = ARM::VMOVD;
else if (ARM::QPRRegClass.contains(DestReg, SrcReg))
Opc = Subtarget.hasNEON() ? ARM::VORRq : ARM::MQPRCopy;
if (Opc) {
MachineInstrBuilder MIB = BuildMI(MBB, I, DL, get(Opc), DestReg);
MIB.addReg(SrcReg, getKillRegState(KillSrc));
if (Opc == ARM::VORRq || Opc == ARM::MVE_VORR) // <=
MIB.addReg(SrcReg, getKillRegState(KillSrc));
if (Opc == ARM::MVE_VORR) // <=
addUnpredicatedMveVpredROp(MIB, DestReg);
else if (Opc != ARM::MQPRCopy)
MIB.add(predOps(ARMCC::AL));
return;
}
....
}Следующая жертва рефакторинга. Как видно по коду, Opc не может принимать значение ARM::MVE_VORR, но сравнение с ним есть. Сразу же проверим, что они не равны:
ARM::MVE_VORR == 1464
ARM::MQPRCopy == 385Смотрим git blame. Судя по тексту коммита и коду, автор решил ввести новую высокоуровневую инструкцию MQPRCopy, что раскрывается в одну из двух более низкоуровневых. Изменены всего два места:
В принципе, не произошло ничего страшного: мёртвый код, который компилятор выкинет. Но тут есть ещё странности, что выделяются среди многих других подобных срабатываний. Например, ниже по коду в этой же функции всё ещё используется ARM::MVE_VORR:
if (ARM::QQPRRegClass.contains(DestReg, SrcReg)) {
Opc = Subtarget.hasNEON() ? ARM::VORRq : ARM::MVE_VORR;
....
}Наверное, только автор или другой компетентный в этом коде разработчик сможет точно сказать, как именно его исправить: просто убрать бесполезные условия, заменить на ARM::MQPRCopy или сделать какую-то другую, более комплексную правку.
Фрагмент N8
Предупреждение PVS-Studio: V590 Consider inspecting this expression. The expression is excessive or contains a misprint. DwarfDebug.cpp 485
void DwarfDebug::addSubprogramNames(....) {
if (getAccelTableKind() != AccelTableKind::Apple &&
NameTableKind != DICompileUnit::DebugNameTableKind::Apple &&
NameTableKind == DICompileUnit::DebugNameTableKind::None)
return;
....
}Продолжаем с пострадавшими от рефакторинга, и, как обычно, заглядываем в git blame. Строчка с DebugNameTableKind::Apple добавлена отдельно. Изначально код был таким:
if (getAccelTableKind() != AccelTableKind::Apple &&
CU.getNameTableKind() == DICompileUnit::DebugNameTableKind::None)
return;В другой функции автор добавил похожее место, но уже более логично:
void DwarfDebug::addAccelNameImpl(....) {
....
if (getAccelTableKind() != AccelTableKind::Apple &&
CU.getNameTableKind() != DICompileUnit::DebugNameTableKind::Apple &&
CU.getNameTableKind() != DICompileUnit::DebugNameTableKind::Default)
return;
....
}Осталось понять, насколько срабатывание серьёзное: просто избыточная проверка или ошибка в логике?
Насколько мне удалось разобраться, оба перечисления означают примерно одно и то же, только AccelTableKind связана с параметрами командной строки, а DebugNameTableKind — с видом неких отладочных данных в самом LLVM IR, которые специфичны для каждого юнита компиляции.
Информации мало, может что-то прояснит текст коммита:
D118754 added a new DICompileUnit::DebugNameTableKind for "Apple".... This creates a problem in the if statements changed, whereby for 'non Apple AccelTableKind' we emit empty tables for any'DebugNameTableKind' that is not 'Default'. We should consider the newly added kind here too.
Судя по комментарию, автор в первую очередь правил функцию addAccelNameImpl и не заметил, что в addSubprogramNames используется оператор == вместо !=, и нужно просто избавится от избыточного условия.
Фрагмент N9
Предупреждение PVS-Studio: V547 Expression 'Value' is always false. LoongArchAsmBackend.cpp 139
static void fixupLeb128(MCContext &Ctx, const MCFixup &Fixup, uint8_t *Data,
uint64_t Value) {
unsigned I;
for (I = 0; Value; ++I, Value >>= 7)
Data[I] |= uint8_t(Value & 0x7f);
if (Value)
Ctx.reportError(Fixup.getLoc(), "Invalid uleb128 value!");
}Ком рефакторинга продолжает нарастать. Изначально код был таким:
static void fixupLeb128(MCContext &Ctx, const MCFixup &Fixup,
MutableArrayRef<char> Data, uint64_t Value) {
unsigned I;
for (I = 0; I != Data.size() && Value; ++I, Value >>= 7)
Data[I] |= uint8_t(Value & 0x7f);
if (Value)
Ctx.reportError(Fixup.getLoc(), "Invalid uleb128 value!");
}Из всех рассмотренных в статье фрагментов кода, этот самый молодой — принят 02.08.2025. Почитав историю, узнаём, что изначально автор работал над совсем другой правкой, после залития которой ему пришлось переделать applyFixup у многих бэкендов из-за проблем с санитайзером. Теперь выше по стеку, в функции MCAssembler::layout, появился assert, что дало возможность передачи параметра Data просто как указателя, без забот о проверках длины:
assert(mc::isRelocRelocation(Fixup.getKind())
|| Fixup.getOffset() <= F.getFixedSize())Из-за такой огромной незапланированной переделки, шутка ли, автор поправил 19 (!!!) бэкендов, в одном из них и возникла мёртвая проверка.
Коммит на GitHub приятно зеленит огромной пачкой тегов затронутых бэкендов. Кстати, я не смог найти, почему среди всех Lanai выделяется более тёмным оттенком.
Говоря об исправлении: нужно ли убрать if совсем, каким-то образом его изменить или переделать логику функции вообще? Точно может сказать только автор, надеюсь он (или другой компетентный контрибьютер) обратит внимание.
Фрагмент N10
Предупреждение PVS-Studio: V547 Expression 'IsLeaf' is always false. PPCInstrInfo.cpp 419
bool PPCInstrInfo::getFMAPatterns(....) const {
....
auto IsReassociableFMA =
[&](const MachineInstr &Instr, int16_t &AddOpIdx,
int16_t &MulOpIdx, bool IsLeaf) {
....
if (IsLeaf) // <=
return true;
AddOpIdx = FMAOpIdxInfo[Idx][InfoArrayIdxAddOpIdx];
const MachineOperand &OpAdd = Instr.getOperand(AddOpIdx);
MachineInstr *MIAdd = MRI->getUniqueVRegDef(OpAdd.getReg());
// If 'add' operand's def is not in current block,
// don't do ILP related opt.
if (!MIAdd || MIAdd->getParent() != MBB)
return false;
// If this is not Leaf FMA Instr, its 'add' operand should only have
// one use as this fma will be changed later.
return IsLeaf ? true : MRI->hasOneNonDBGUse(OpAdd.getReg()); // <=
}
....
}Классический случай... да, да, да, рефакторинга. По истории видно, что прежде проверка раннего выхода была сложнее:
if (IsAdd && IsLeaf)Автор решил разделить лямбду на две отдельных: для сложения и для умножения-сложения (FMA). Из-за этого часть IsAdd перестала быть нужна, а конец функции не попал во внимание и не был отрефакторен.
Следуя логике функции, скорее всего, нужно просто заменить финальный возврат на:
return MRI->hasOneNonDBGUse(OpAdd.getReg());Фрагмент N11
Предупреждение PVS-Studio: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 5820, 5823. PPCInstrInfo.cpp 5823
bool PPCInstrInfo::getMemOperandWithOffsetWidth(....){
if (!LdSt.mayLoadOrStore() || LdSt.getNumExplicitOperands() != 3)
return false;
// Handle only loads/stores with base register
// followed by immediate offset.
if (!LdSt.getOperand(1).isImm() ||
(!LdSt.getOperand(2).isReg() && !LdSt.getOperand(2).isFI()))
return false;
if (!LdSt.getOperand(1).isImm() ||
(!LdSt.getOperand(2).isReg() && !LdSt.getOperand(2).isFI()))
return false;
....
}В файле PPCInstrInfo.cpp из прошлого фрагмента есть ещё одно срабатывание, но уже с более сложной историей. При попытке выяснить, как же так вышло, натурально голова идёт кругом. Коммит с изменением условия if, его правка, реверт коммита из-за санитайзера, повторное слияние и т.д. — всё в один и тот же день. Код ходил ходуном. Например, в какой-то момент была удвоена другая проверка:
bool PPCInstrInfo::getMemOperandWithOffsetWidth(....) const {
if (!LdSt.mayLoadOrStore() || LdSt.getNumExplicitOperands() != 3) // <=
return false;
// Handle only loads/stores with base register
// followed by immediate offset.
if (LdSt.getNumExplicitOperands() != 3) // <=
return false;
if (!LdSt.getOperand(1).isImm() ||
(!LdSt.getOperand(2).isReg() && !LdSt.getOperand(2).isFI()))
return false;
....
}Быстрый успокоительный тест на отсутствие побочных эффектов:
bool isImm() const { return OpKind == MO_Immediate; }
bool isReg() const { return OpKind == MO_Register; }
bool isFI() const { return OpKind == MO_FrameIndex; }LLVM огромен и сложен, но, тем не менее, очень активно развивается, в том числе благодаря относительно лёгкому принятию pull request'ов, например, от разработчиков не требуется прогонять целиком весь титанический объём тестов перед этим. Взамен поощряются и считаются нормальной частью разработки откаты коммитов, т. н. revert to green. Как видим, в некоторых случаях это выходит боком, провоцируя "кривые" слияния.
Фрагмент N12
Предупреждение PVS-Studio: V547 Expression 'Opcode == Instruction::FDiv' is always false. GISelValueTracking.cpp 1451
void GISelValueTracking::computeKnownFPClass(....) {
....
unsigned Opcode = MI.getOpcode(); // variable is never changed
....
switch(Opcode){
....
case TargetOpcode::G_FDIV: // == 188
case TargetOpcode::G_FREM: { // == 189
....
if (LHS == RHS)
if (Opcode == TargetOpcode::G_FDIV) {
....
}
....
if (Opcode == Instruction::FDiv) { // == 21
// Only 0/0, Inf/Inf produce NaN.
....
}
}
....
}Последние несколько срабатываний были довольно безобидными, сейчас перейдём к более весомым ошибкам в логике. В отличиe от прошлых фрагментов, весь этот титанический switch (почти 1000 строк кода!) добавлен целиком, и после изменения вносились точечно вне интересующей нас ветки case.
Выше, к слову, есть похожая странность:
case TargetOpcode::G_FADD: // == 183
case TargetOpcode::G_STRICT_FADD: // == 277
case TargetOpcode::G_FSUB: // == 184
case TargetOpcode::G_STRICT_FSUB: // == 278
.....
if (Opcode == Instruction::FAdd) { // == 14
....
}
....
}Сравнения по двум разным enum выглядят уже очень странно. Вдобавок, кроме этих двух мест, значения из Instruction нигде не используются, тогда как TargetOpcode — не только в case'ах, но и в различных проверках и сравнениях. Весьма вероятно, здесь ошибка копипасты, и должно использоваться значение TargetOpcode::G_FDIV в первом случае и TargetOpcode::G_FADD во втором.
Фрагмент N13
Предупреждение PVS-Studio: V560 A part of conditional expression is always false: Defs.count(R) > 1. HexagonMCChecker.cpp 612
bool HexagonMCChecker::checkRegisters() {
....
if (isLoopRegister(R) && Defs.count(R) > 1 &&
(HexagonMCInstrInfo::isInnerLoop(MCB) ||
HexagonMCInstrInfo::isOuterLoop(MCB))) {
// Error out for definitions of loop registers at the end of a loop.
reportError("loop-setup and some branch instructions "
"cannot be in the same packet");
return false;
}
....
}Вполне обычный код, если не знать, что Defs – это DenseMap:
/// Return 1 if the specified key is in the map, 0 otherwise.
size_type count(const_arg_type_t<KeyT> Val) const {
return contains(Val) ? 1 : 0;
}Как видим, логгирование ошибки никогда не произойдёт и, более того, выполнение всегда продолжится с потенциально некорректным поведением.
Вот что докладывает git blame: в ноябре код празднует десятилетие, и можно предположить, что произошло изменение его смысла позднее. Но нет, история подтверждает: Defs всегда было DenseMap, а её функция count и в те времена возвращала только 0 или 1. А если сделать поиск по другим похожим проверкам, то везде видим использование результата count как бинарного значения. Несколько примеров:
if (Defs.count(Reg))
if (Defs.count(Reg) && !MO.isDead())
if (Uses.count(DstReg) || Defs.count(SrcReg))Судя по всему, поведение DenseMap стремились сделать максимально приближенным к контейнерам стандартной библиотеки, где у std::map и std::multimap есть общий интерфейс count. В случае std::multimap он может вернуть более одного совпадения. Это подтверждается и текстом коммита, что изменил возвращаемый тип функции-члена count с bool на size_type:
The count() function for STL datatypes returns unsigned, even where it's only 1/0 result like std::set.
И к нему прилагается объяснение на сайте ревью LLVM:
If we want our container-like types to be containers, count should return size_type.
В итоге либо должен использоваться другой контейнер, либо правильная проверка должна выглядеть примерно так:
if (isLoopRegister(R) && Defs.count(R) &&
(HexagonMCInstrInfo::isInnerLoop(MCB) ||
HexagonMCInstrInfo::isOuterLoop(MCB)))Фрагмент N14
Предупреждение PVS-Studio: V560 A part of conditional expression is always false: AVX10Ver >= 2. Host.cpp 2177
StringMap<bool> sys::getHostCPUFeatures() {
unsigned EAX = 0, EBX = 0, ECX = 0, EDX = 0;
....
bool HasLeaf24 = MaxLevel >= 0x24
&& !getX86CpuIDAndInfo(0x24, &EAX, &EBX, &ECX, &EDX);
int AVX10Ver = HasLeaf24 && (EBX & 0xff);
Features["avx10.1"] = HasAVX10 && AVX10Ver >= 1;
Features["avx10.2"] = HasAVX10 && AVX10Ver >= 2;
return Features;
}И если прошлую пару срабатываний можно было объединить под лозунгом "False, false и ничего кроме false", то здесь оттенок немного другой. В выражении HasLeaf24 && (EBX & 0xff) сперва оба операнда && приведутся к типу bool, вычислится логическое "И", а затем результат снова расширится до типа int. На выходе получаем значение 0 или 1, и выражение AVX10Ver >= 2 всегда вычислится как false.
Судя по истории, в прошлой итерации было так:
bool HasLeaf24 = MaxLevel >= 0x24
&& !getX86CpuIDAndInfo(0x24, &EAX, &EBX, &ECX, &EDX);
Features["avx10.1-512"] = Features["avx10.1-256"]
&& HasLeaf24
&& ((EBX >> 18) & 1);Давайте обратим внимание на то, что вообще делает функция. А она собирает в Features поддерживаемые возможности процессора, т. е. значения инструкции CPUID.
Действительно, открываем спецификацию Intel Advanced Vector Extension 10.1. Для CPUID указано:
CPUID.(EAX=24H, ECX=00H):EBX[bit 18]
If 1, indicates that 512-bit vector support is present.Всё согласуется с первоначальным вариантом, но в спецификации Advanced Vector Extension 10.2, поддержку которой и добавлял автор, уже иначе:
CPUID.(EAX=0x07,ECX=0x01):EDX.AVX10[19]
and (CPUID.(EAX=0x24,ECX=0x0):EBX[7:0] >= 2)Диапазон значения изменился с одного бита до восьми: с нулевого по седьмой в EBX, что, собственно, достаётся в EBX & 0xff и в bool уже не вмещается.
Судя по спецификации, корректный код должен быть, например, таким:
int AVX10Ver = EBX & 0xff;
Features["avx10.1"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 1;
Features["avx10.2"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 2;Фрагмент N15
Предупреждение PVS-Studio: V547 Expression 'i != e' is always true. MachineFunction.cpp 1444
void MachineJumpTableInfo::print(raw_ostream &OS) const {
if (JumpTables.empty()) return;
OS << "Jump Tables:\n";
for (unsigned i = 0, e = JumpTables.size(); i != e; ++i) {
OS << printJumpTableEntryReference(i) << ':';
for (const MachineBasicBlock *MBB : JumpTables[i].MBBs)
OS << ' ' << printMBBReference(*MBB);
if (i != e)
OS << '\n';
}
OS << '\n';
}Первое срабатывание из серии найденных в функциях-принтерах. Изначально цикл печатал значения единой строкой:
for (unsigned i = 0, e = JumpTables.size(); i != e; ++i) {
OS << printJumpTableEntryReference(i) << ": ";
for (unsigned j = 0, f = JumpTables[i].MBBs.size(); j != f; ++j)
OS << ' ' << printMBBReference(*JumpTables[i].MBBs[j]);
}
OS << '\n';Видимо, по задумке if (i != e) должно было предотвратить два перевода строки подряд после последней строки, но автор неправильно написал условие, и свою роль проверка не отыграла. В этом случае корректный код должен быть примерно таким:
void MachineJumpTableInfo::print(raw_ostream &OS) const {
if (JumpTables.empty()) return;
OS << "Jump Tables:\n";
for (unsigned i = 0, e = JumpTables.size(); i != e; ++i) {
OS << printJumpTableEntryReference(i) << ':';
for (const MachineBasicBlock *MBB : JumpTables[i].MBBs)
OS << ' ' << printMBBReference(*MBB);
OS << '\n';
}
}Фрагмент N16
Предупреждение PVS-Studio: V547 Expression is always true. SystemZAsmPrinter.cpp 1324
static void emitPPA1Flags(....){
....
auto Flags1 = PPA1Flag1(0);
Flags1 |= PPA1Flag1::DSA64Bit;
if (VarArg) Flags1 |= PPA1Flag1::VarArg;
.... // Flags1 hasn't changed
if ((Flags1 & PPA1Flag1::DSA64Bit) == PPA1Flag1::DSA64Bit)
OutStreamer->AddComment(" Bit 0: 1 = 64-bit DSA");
else
OutStreamer->AddComment(" Bit 0: 0 = 32-bit DSA");
....
}Продолжение темы принтеров. Странная проверка, ведь если проставить в число биты, а потом проверить их же наличие через битовое "И", то, конечно, они никуда не денутся, в то время как все остальные флаги проверяются условно через переданные параметры:
static void emitPPA1Flags(std::unique_ptr<MCStreamer> &OutStreamer,
bool VarArg,
bool StackProtector,
bool FPRMask,
bool VRMask,
bool EHBlock,
bool HasName) {
....
auto Flags1 = PPA1Flag1(0);
Flags1 |= PPA1Flag1::DSA64Bit;
if (VarArg)
Flags1 |= PPA1Flag1::VarArg;
if (StackProtector)
Flags2 |= PPA1Flag2::STACKPROTECTOR;
// SavedGPRMask, SavedFPRMask, and SavedVRMask are precomputed in.
if (FPRMask)
Flags3 |= PPA1Flag3::FPRMask; // Add emit FPR mask flag.
if (VRMask)
Flags4 |= PPA1Flag4::VRMask; // Add emit VR mask flag.
if (EHBlock)
Flags4 |= PPA1Flag4::EHBlock; // Add optional EH block.
if (HasName)
Flags4 |= PPA1Flag4::ProcedureNamePresent; // Add optional name block.
....
}Можно было бы предположить, что проверка сделана ради единообразия с другими флагами, но зачем тогда ветка else? В коммите указано:
Add the PPA1 to SystemZAsmPrinter.
Окей, обращаемся к документации от IBM:
PPA1 flag 1 offset X'08
'0.......'B GPR Save area is 32 bit. // <=
'1.......'B GPR Save area is 64 bit. // <=
'.0......'B Reserved.
'..0.....'B Own exception model.
'..1.....'B Inherited exception model.
'...0....'B tiny PPA3.
'...1....'B full PPA3.
'....0...'B Do Not call member for Exit DSA event.
'....1...'B Call member for Exit DSA event.
'.....0..'B Do Not treat as PL/I style exit DSA.
'.....1..'B Treat as PL/I style exit DSA.
'......0.'B This is not a Special linkage routine.
'......1.'B This is a Special linkage routine.
'.......0'B Not a Vararg routine.
'.......1'B Vararg routine.Как видим, 32-битный вариант действительно возможен, но почему код устроен именно так, знает лишь автор.
Фрагмент N17
Предупреждение PVS-Studio: V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 1995
void MachineInstr::print(....) const {
....
if (MCSymbol *PreInstrSymbol = getPreInstrSymbol()) {
if (!FirstOp) {
FirstOp = false;
OS << ',';
}
OS << " pre-instr-symbol ";
MachineOperand::printSymbol(OS, *PreInstrSymbol);
}
....
}Завершающая порция проблем с принтерами, но уже не с выводом, а с бессмысленным присваиванием. Всего таких срабатываний в этой функции пять штук. По истории видим, что первоначально были блоки вида:
if (!FirstOp) {
OS << ',';
}Потом впервые добавился один блок с присвоением:
if (!FirstOp) {
FirstOp = false;
OS << ',';
}И в последующем такие же блоки добавлялись различными авторами, скорее всего, путём копипасты первого. Особо обсуждать тут нечего: пять бесполезных строк — это явно что-то лишнее в коде.
Другие срабатывания в файле:
Фрагмент N18
Предупреждение PVS-Studio: V547 Expression 'AllSame' is always true. SimplifyCFG.cpp 1914
bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(....) {
....
bool AllSame = none_of(....);
if (!AllSame)
return false;
if (AllSame) { .... }
....
}Отладка компилятора — весьма сложная затея, поэтому разработчики LLVM густо вставляют всевозможные проверки и assert'ы. Но некоторые из них заставляют усомниться в своём здравомыслии и восприятии реальности, и эта одна из них. И нет, здесь не ошибка мержа или рефакторинга — всё добавлено разом.
Фрагмент N19
Предупреждение PVS-Studio: V547 Expression 'AbbrevDecl' is always true. LVDWARFReader.cpp 398
LVScope *LVDWARFReader::processOneDie(....) {
....
if (const DWARFAbbreviationDeclaration *AbbrevDecl =
TheDIE.getAbbreviationDeclarationPtr())
if (AbbrevDecl)
for (const DWARFAbbreviationDeclaration::AttributeSpec &AttrSpec :
AbbrevDecl->attributes())
processOneAttribute(TheDIE, &CurrentEndOffset, AttrSpec);
....
}Ещё одна паранормальная проверка. Внутренний if избыточен т.к. когда выполнение дойдёт до него, AbbrevDecl и так всегда будет ненулевым.
Фрагмент N20
Предупреждение PVS-Studio: V791 The initial value of the index in the nested loop equals 'I'. Perhaps, 'I + 1' should be used instead. LoopUnrollAndJam.cpp 793
static bool checkDependencies(....){
....
for (size_t I = 0; I < NumInsts; ++I) {
for (size_t J = I; J < NumInsts; ++J) {
if (!checkDependency(CurrentLoadsAndStores[I],
CurrentLoadsAndStores[J],
LoopDepth, CurLoopDepth, true, DI))
return false;
....
}
....
}Напоследок для разнообразия небольшое оптимизационное срабатывание. Как видим, функция сравнения сразу же возвращает true при равенстве аргументов-указателей:
static bool checkDependency(Instruction *Src,
Instruction *Dst,
....) {
if (Src == Dst)
return true;
....
}Как правильно заметил анализатор, с учётом раннего выхода из функции сравнения и отсутствия побочных эффектов нет смысла начинать внутренний цикл с I, а не I + 1.
Как мы увидели сегодня, даже такие квалифицированные разработчики, как контрибьюторы LLVM, совершают порой досадные ошибки: как логические, так и опечаточно-копипастные, не говоря уже об остатках некорректных автоматических мержей. Любой дракон нуждается в постоянной чистке зубов! Для этого можно использовать в том числе и бесплатную версию PVS-Studio для open source проектов.
Ссылка на созданные issues на GitHub LLVM.
0
0
0
27
0
0