LLVM 21 Under Static Analysis: Bugs, UB Risks, and Refactor Fallout

It has been over a year since the last check of the LLVM project with PVS-Studio, and in that time, two releases have come out. So, it's a good time to get back and analyze the fresh LLVM 21. Intro Although pretty much all C++ developers have at least heard of LLVM, we'll make a brief disclaimer. LLVM is a massive open-source umbrella project that contains a huge set of various tools for writing optimizing compilers of any complexity. I checked only the 253d18d032bb commit from the main branch without Clang, libc, lldb, and other components, and focused on High- and Medium-severity errors detected by the general-purpose diagnostic rules. 253d18d032bb main The commit links are provided to help verify and fix detected errors. They aren't intended to compromise or undermine the work of the contributors. Well, let's get started! Analysis results Fragment N1 Fragment N1 The PVS-Studio warning: V501 There are identical sub-expressions '!Subtarget.hasZeroCycleRegMoveFPR64()' to the left and to the right of the '&&' operator. AArch64InstrInfo.cpp 5430 V501 AArch64InstrInfo.cpp 5430 void AArch64InstrInfo::copyPhysReg(....) const { .... if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR64() && Subtarget.isNeonAvailable()) .... } void AArch64InstrInfo::copyPhysReg(....) const { .... if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR64() && Subtarget.isNeonAvailable()) .... } Let's start our investigation with a git blame. We can see that, along with this suspicious fragment in the commit, the devs added four identical checks: git blame commit if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR32() && .... if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR32() && .... Upon closer inspection, we can notice the following in the blame: blame if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR32() && Subtarget.isNeonAvailable() { /* * new code */ if (Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR32()) /* * existing code */ } if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR32() && Subtarget.isNeonAvailable() { /* * new code */ if (Subtarget.hasZeroCycleRegMoveFPR64() && !Subtarget.hasZeroCycleRegMoveFPR32()) /* * existing code */ } This looks like a classic copy-paste error, but, before jumping to conclusions, let's check whether hasZeroCycleRegMoveFPR64 has side effects that could cause it to be called twice. After all, when dealing with a project as large and intricate as LLVM, it's important to be extra cautious. hasZeroCycleRegMoveFPR64 The implementation lies in the AArch64GenSubtargetInfo.inc file generated by the internal TableGen DSL: AArch64GenSubtargetInfo.inc GET_SUBTARGETINFO_MACRO(HasZeroCycleRegMoveFPR64, false, hasZeroCycleRegMoveFPR64) GET_SUBTARGETINFO_MACRO(HasZeroCycleRegMoveFPR64, false, hasZeroCycleRegMoveFPR64) The macro is defined in the nearby AArch64Subtarget.h file: 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" .... } 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" .... } As we can see, the macro generates DSL getters and setters, which means that we just need to replace the second hasZeroCycleRegMoveFPR64 with hasZeroCycleRegMoveFPR32. hasZeroCycleRegMoveFPR64 hasZeroCycleRegMoveFPR32 Fragment N2 Fragment N2 The PVS-Studio warning: V501 There are identical sub-expressions to the left and to the right of the '==' operator: PeekArg.getValNo() == PeekArg.getValNo() PPCISelLowering.cpp 7865 V501 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))); } } .... } 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))); } } .... } We're assuming for now that it's yet another victim of copy-paste. Let's check whether getValNo have any side effects: getValNo class CCValAssign{ .... unsigned ValNo; unsigned getValNo() const { return ValNo; } } class CCValAssign{ .... unsigned ValNo; unsigned getValNo() const { return ValNo; } } Nothing strange here, though. Take a look at the last commit: commit 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))); 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))); The idea is clear: an exceptional case that was previously guarded by an assert was redesigned into a regular branch. The commit text also points to that. 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. 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. It can be noted that, in addition to the error found, there's another strange assignment: CCValAssign &PeekArg = ArgLocs[I]; .... CCValAssign &GPR2 = ArgLocs[I++]; // here PeekArg == GPR2 CCValAssign &PeekArg = ArgLocs[I]; .... CCValAssign &GPR2 = ArgLocs[I++]; // here PeekArg == GPR2 Maybe the developer would like to write something like that: 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))); } } 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))); } } But for clarity, developers broke out PeekArg from GPR2 to show that, unlike the previous unconditional code, the argument now needs to be "peeked" first. And during the copy-paste process, GPR1 was accidentally dropped from the condition. PeekArg GPR2 GPR1 The corrected if likely should be: if if (PeekArg.isRegLoc() && PeekArg.getValNo() == GPR1.getValNo()) if (PeekArg.isRegLoc() && PeekArg.getValNo() == GPR1.getValNo()) Interestingly, before migrating to GitHub, LLVM had a separate code review platform, and the commit included a link to this website. There, we can see that manual review can't always salvage the situation: website Fragment N3 Fragment N3 The PVS-Studio warning: V557 Array overrun is possible. The value of 'regIdx' index could reach 31. VEAsmParser.cpp 696 V557 VEAsmParser.cpp 696 static const MCPhysReg MISCRegs[31] = { .... }; static bool MorphToMISCReg(VEOperand &Op) { const auto *ConstExpr = dyn_cast (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; } static const MCPhysReg MISCRegs[31] = { .... }; static bool MorphToMISCReg(VEOperand &Op) { const auto *ConstExpr = dyn_cast (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; } As we know, undefined behavior is out of compilers' scope. However, it doesn't mean that their codebase is immune to UB. Here we can see a classic UB scenario, triggered only if regIdx == 31. Probably, it seems impossible or very unlikely, so the error has been unnoticed. But who knows what tomorrow will bring? regIdx == 31 The only question left is where the error lies: is there an incorrect number in the condition, or has an element been forgotten when adding to the array? The function is located in the Vector Engine Assembly Language parser's code and, apparently, checks whether the SMIR instruction operand is valid. Take a look at the documentation: 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 — 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 We're interested in the first option, smir %sx, I, where I = 0 – 2, 7 – 11, 16 – 30 are valid. Let's compare it with the array: 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}; 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}; Everything matches, so there can be no ambiguity: regIdx > 31 should be replaced with regIdx > 30, or, even better, regIdx >= std::size(MISCRegs). regIdx > 31 regIdx > 30 regIdx >= std::size(MISCRegs) Interestingly, a similar function was added in the same commit, where the check is correct: commit static const unsigned MiscRegDecoderTable[] = {....}; // 30) return MCDisassembler::Fail; unsigned Reg = MiscRegDecoderTable[RegNo]; if (Reg == VE::NoRegister) return MCDisassembler::Fail; Inst.addOperand(MCOperand::createReg(Reg)); return MCDisassembler::Success; } static const unsigned MiscRegDecoderTable[] = {....}; // 30) return MCDisassembler::Fail; unsigned Reg = MiscRegDecoderTable[RegNo]; if (Reg == VE::NoRegister) return MCDisassembler::Fail; Inst.addOperand(MCOperand::createReg(Reg)); return MCDisassembler::Success; } Maybe the explicit number 31 in the MISCRegs array declaration may mislead developers. MISCRegs Fragment N4 Fragment N4 The PVS-Studio warning: V519 The 'FeaturesValue' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1190, 1192. LVCodeViewReader.cpp 1192 V519 LVCodeViewReader.cpp 1192 Error LVCodeViewReader::loadTargetInfo(const ObjectFile &Obj) { .... Expected Features = Obj.getFeatures(); SubtargetFeatures FeaturesValue; if (!Features) { consumeError(Features.takeError()); FeaturesValue = SubtargetFeatures(); } FeaturesValue = *Features; .... return loadGenericTargetInfo(TT.str(), FeaturesValue.getString(), CPU); } Error LVCodeViewReader::loadTargetInfo(const ObjectFile &Obj) { .... Expected Features = Obj.getFeatures(); SubtargetFeatures FeaturesValue; if (!Features) { consumeError(Features.takeError()); FeaturesValue = SubtargetFeatures(); } FeaturesValue = *Features; .... return loadGenericTargetInfo(TT.str(), FeaturesValue.getString(), CPU); } It looks like a dead store, but maybe the first assignment is intentionally performed for side effects? Let's examine it: class SubtargetFeatures { std::vector Features; .... SubtargetFeatures::SubtargetFeatures(StringRef Initial = "") { // Break up string into separate features Split(this->Features, Initial); } } class SubtargetFeatures { std::vector Features; .... SubtargetFeatures::SubtargetFeatures(StringRef Initial = "") { // Break up string into separate features Split(this->Features, Initial); } } The Split function splits the string (in our case, empty) by commas and stores it in an internal vector. Split This could be a minor suboptimality, if not for the behavior of the Expected ::takeError function: Expected ::takeError template class [[nodiscard]] Expected { .... /// Take ownership of the stored error. /// After calling this the Expected is in an indeterminate state that can /// only be safely destructed. No further calls (beside the destructor) should /// be made on the Expected value. Error takeError() { #if LLVM_ENABLE_ABI_BREAKING_CHECKS Unchecked = false; #endif return HasError ? Error(std::move(*getErrorStorage())) : Error::success(); } } template class [[nodiscard]] Expected { .... /// Take ownership of the stored error. /// After calling this the Expected is in an indeterminate state that can /// only be safely destructed. No further calls (beside the destructor) should /// be made on the Expected value. Error takeError() { #if LLVM_ENABLE_ABI_BREAKING_CHECKS Unchecked = false; #endif return HasError ? Error(std::move(*getErrorStorage())) : Error::success(); } } Thanks to the detailed comments in LLVM, which feel like there are just as many as there is code. They can help track the path to a potential UB. Let's first look at what will be called when the if statement is checked: if /// Return false if there is an error. explicit operator bool() { #if LLVM_ENABLE_ABI_BREAKING_CHECKS Unchecked = HasError; #endif return !HasError; } /// Return false if there is an error. explicit operator bool() { #if LLVM_ENABLE_ABI_BREAKING_CHECKS Unchecked = HasError; #endif return !HasError; } If Features contains an error, we'll end up in the then branch: Features then Error LVCodeViewReader::loadTargetInfo(const ObjectFile &Obj) { .... if (!Features) { consumeError(Features.takeError()); FeaturesValue = SubtargetFeatures(); } FeaturesValue = *Features; .... return loadGenericTargetInfo(TT.str(), FeaturesValue.getString(), CPU); } Error LVCodeViewReader::loadTargetInfo(const ObjectFile &Obj) { .... if (!Features) { consumeError(Features.takeError()); FeaturesValue = SubtargetFeatures(); } FeaturesValue = *Features; .... return loadGenericTargetInfo(TT.str(), FeaturesValue.getString(), CPU); } Next, the ownership of the internal Features data member is passed. The code then proceeds to dereference it, resulting in undefined behavior: Features reference operator*() const { assertIsChecked(); return *getStorage(); } reference operator*() const { assertIsChecked(); return *getStorage(); } Okay, there's still a last line of defense—assertIsChecked_—_but unfortunately, it's only used for check after significant changes to the ABI: assertIsChecked 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 } 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 } The developers added this file to the commit along with many modified code fragments—45 files and 7,000 lines. However, the high-level, abstract description in the message doesn't make the implementation details any clearer. Very likely, we can say that developers just forgot to add the else branch, and the code should look like this: commit else if (!Features) { consumeError(Features.takeError()); FeaturesValue = SubtargetFeatures(); } else FeaturesValue = *Features; if (!Features) { consumeError(Features.takeError()); FeaturesValue = SubtargetFeatures(); } else FeaturesValue = *Features; Here, we just fixed the error; however, there's also room for further improvements, for example, the first FeaturesValue assignment is still redundant because the constructor is called twice by default. However, it helps find more serious potential vulnerability. FeaturesValue The code could be changed like that: Expected Features = Obj.getFeatures(); SubtargetFeatures FeaturesValue; if (!Features) { consumeError(Features.takeError()); } else { FeaturesValue = *Features; } Expected Features = Obj.getFeatures(); SubtargetFeatures FeaturesValue; if (!Features) { consumeError(Features.takeError()); } else { FeaturesValue = *Features; } We can go beyond and enhance readability, for example, by writing as follows: SubtargetFeatures FeaturesValue; if (Expected Features = Obj.getFeatures()) FeaturesValue = std::move(*Features); else consumeError(Features.takeError()); SubtargetFeatures FeaturesValue; if (Expected Features = Obj.getFeatures()) FeaturesValue = std::move(*Features); else consumeError(Features.takeError()); Fragment N5 Fragment N5 The PVS-Studio warning: V609 Divide by zero. Denominator range [0..255]. RISCVCustomBehaviour.cpp 268 V609 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 Log2IdxEEW) * LMUL) / SEW; .... } else .... .... } 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 Log2IdxEEW) * LMUL) / SEW; .... } else .... .... } The comment with the SEW variable lets us know that it's equal to 0. It's still a valid scenario, however, it makes the immediate and fearless potential division by zero very suspicious. The collection of undefined behavior cases was expanded with a third copy. SEW The git blame investigation shows that the if and its then branch, where the division takes place, appeared only after the SEW calculation. Before that, the else branch was the sole, unconditional execution path. git blame shows if then SEW else As we can see, in this original path, SEW == 0 is OK: 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); // 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); We won't show the getBaseInfo code here, but you can trust (or check!) that there are no dangerous zero-related operations—just a large bunch of various data member comparisons. It remains to check that the conditions are incompatible when SEW == 0 and when we divide by it. It means that the following can't happen: getBaseInfo SEW == 0 !SI && RISCV::getVXMemOpInfo(Opcode) !SI && RISCV::getVXMemOpInfo(Opcode) We open getVXMemOpInfo and again end up in the TableGen file (RISCVGenSearchableTables.inc): getVXMemOpInfo 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 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; } 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 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; } It looks bulky, but the essence is clear: the operation code is checked whether it's located within the RISCVBaseVXMemOpTable table that seems to represent RISC-V indexed vector operations: RISCVBaseVXMemOpTable 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 }; 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 }; Now let's see what if SI == nullptr: 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 (I); else if (I->getDesc() == RISCVSEWInstrument::DESC_NAME) SI = static_cast (I); } // 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 (I); else if (I->getDesc() == RISCVSEWInstrument::DESC_NAME) SI = static_cast (I); } So, UB occurs if the vector operation and a set of instruments without RISCVSEWInstrument are sent as arguments at the same time. RISCVSEWInstrument Based on the observations and RISC-V documentation, such a scenario seems impossible, but only backend developers can say for sure—and most likely only after careful analysis. Indeed, it'd be better to fix a potentially serious error either by direct checking via if (SEW != 0), or, if this scenario is truly impossible, using assert. The developers did it in another else branch of the same check chain (inside getEEWAndEMUL): if (SEW != 0) , assert else getEEWAndEMUL assert(SEW >= 8 && "Unexpected SEW value") assert(SEW >= 8 && "Unexpected SEW value") Fragment N6 Fragment N6 The PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. VEISelDAGToDAG.cpp 321 V779 VEISelDAGToDAG.cpp 321 bool VEDAGToDAGISel::SelectInlineAsmMemoryOperand( const SDValue &Op, InlineAsm::ConstraintCode ConstraintID, std::vector &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; .... } bool VEDAGToDAGISel::SelectInlineAsmMemoryOperand( const SDValue &Op, InlineAsm::ConstraintCode ConstraintID, std::vector &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; .... } At first glance, it's unclear why the analyzer issues the warning about unreachable code, but once we look at the innards of the selectADDRri function, everything falls into place: 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; } 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; } Let's look at git blame. We can see the return value of selectADDRri used to be more varied (only returns are left): git blame selectADDRri bool VEDAGToDAGISel::SelectADDRri(....) { if (....) { .... return true; } if (....) return false; if (....) { .... return true; } .... return true; } bool VEDAGToDAGISel::SelectADDRri(....) { if (....) { .... return true; } if (....) return false; if (....) { .... return true; } .... return true; } Subsequently, this body was moved to the matchADDRri function, and selectADDRri received its current one. matchADDRri selectADDRri It seems that it's all quite simple: there's a slightly unsuccessful refactoring, in which the function logic was changed without checking where it was used. However, git blame showed the opposite: on April 6, 2020, the selectADDRri body was modified, and two years later, on August 23, 2022, the same developer used it in if. One might assume they knew exactly what they were doing... if not for the comments. For example, why do they write "Otherwise" when they already know the code path is unreachable? git blame on April 6, 2020 selectADDRri on August 23, 2022 if // 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 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. These same comments provide a hint: "try to match." What if matchADDRri should have been called instead of selectADDRri? matchADDRri selectADDRri Let's substitute and compare: 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)); 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)); If matchADDRri returns true, Op1 and Op2 are placed in the vector. Otherwise, we put Op and the 0 constant. matchADDRri true Op1 Op2 Op 0 What will be in selectADDRri if we replace the names of the formal parameters with the actual ones? 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; } 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; } In this case, if matchADDRri returns true, Op and Op2 contain the values that this function returns. If it returns false, there will be Op and the 0 constant. Does it remind you of anything? Perhaps the developer has inspired by the algorithm in selectADDRri and accidentally inserted its name instead of matchADDRri at the end. matchADDRri true Op Op2 false Op 0 selectADDRri matchADDRri Fragment N7 Fragment N7 The PVS-Studio warnings: V560 A part of conditional expression is always false: Opc == ARM::MVE_VORR. ARMBaseInstrInfo.cpp 752 V560 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; } .... } 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; } .... } Here's another refactoring victim: Opc can't take the ARM::MVE_VORR value, but there's a comparison with it. Let's check for equality: Opc ARM::MVE_VORR ARM::MVE_VORR == 1464 ARM::MQPRCopy == 385 ARM::MVE_VORR == 1464 ARM::MQPRCopy == 385 Now let's glimpse git blame. According to the commit text and code, developers decided to introduce a new high-level instruction, MQPRCopy, which expands into one of two lower-level instructions. They changed two things: git blame commit MQPRCopy Nothing serious happened: it's just a dead code that the compiler will throw away. However, there are bizarre things that stand out among many other similar warnings. For example, the function uses ARM::MVE_VORR in the code below. ARM::MVE_VORR if (ARM::QQPRRegClass.contains(DestReg, SrcReg)) { Opc = Subtarget.hasNEON() ? ARM::VORRq : ARM::MVE_VORR; .... } if (ARM::QQPRRegClass.contains(DestReg, SrcReg)) { Opc = Subtarget.hasNEON() ? ARM::VORRq : ARM::MVE_VORR; .... } Probably only the code author or another contributor can say exactly how to fix it: simply remove useless conditions, replace with ARM::MQPRCopy, or make another, more complex edit. ARM::MQPRCopy Fragment N8 Fragment N8 The PVS-Studio warning: V590 Consider inspecting this expression. The expression is excessive or contains a misprint. DwarfDebug.cpp 485 V590 DwarfDebug.cpp 485 void DwarfDebug::addSubprogramNames(....) { if (getAccelTableKind() != AccelTableKind::Apple && NameTableKind != DICompileUnit::DebugNameTableKind::Apple && NameTableKind == DICompileUnit::DebugNameTableKind::None) return; .... } void DwarfDebug::addSubprogramNames(....) { if (getAccelTableKind() != AccelTableKind::Apple && NameTableKind != DICompileUnit::DebugNameTableKind::Apple && NameTableKind == DICompileUnit::DebugNameTableKind::None) return; .... } Let's look at another refactoring episode. As usual, look at git blame: the line with DebugNameTableKind::Apple was added separately. Initially, the code was as follows: git blame DebugNameTableKind::Apple separately if (getAccelTableKind() != AccelTableKind::Apple && CU.getNameTableKind() == DICompileUnit::DebugNameTableKind::None) return; if (getAccelTableKind() != AccelTableKind::Apple && CU.getNameTableKind() == DICompileUnit::DebugNameTableKind::None) return; In another function, the developer added a similar segment, but it looks like neater: void DwarfDebug::addAccelNameImpl(....) { .... if (getAccelTableKind() != AccelTableKind::Apple && CU.getNameTableKind() != DICompileUnit::DebugNameTableKind::Apple && CU.getNameTableKind() != DICompileUnit::DebugNameTableKind::Default) return; .... } void DwarfDebug::addAccelNameImpl(....) { .... if (getAccelTableKind() != AccelTableKind::Apple && CU.getNameTableKind() != DICompileUnit::DebugNameTableKind::Apple && CU.getNameTableKind() != DICompileUnit::DebugNameTableKind::Default) return; .... } It remains to determine how serious the issue is: is it just an excessive check or a logical error? As demonstrated, both code fragments mean roughly the same thing, except that AccelTableKind is related to command-line parameters, while DebugNameTableKind is related to a kind of debug data embedded in LLVM IR and scoped to each compilation unit. AccelTableKind DebugNameTableKind There's not much information available, but perhaps the commit text sheds some light: 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. 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. Judging by the comment, the developer primarily fixed the addAccelNameImpl function and just overlooked that the == operator is used instead of != in addSubprogramNames. Most likely, the redundant condition should just be removed. addAccelNameImpl == != addSubprogramNames Fragment N9 Fragment N9 The PVS-Studio warning: V547 Expression 'Value' is always false. LoongArchAsmBackend.cpp 139 V547 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, 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!"); } The snowball of refactoring keeps growing. Initially, the code was as follows: Initially static void fixupLeb128(MCContext &Ctx, const MCFixup &Fixup, MutableArrayRef 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!"); } static void fixupLeb128(MCContext &Ctx, const MCFixup &Fixup, MutableArrayRef 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!"); } Among all the code fragments analyzed in this article, this one is the most recent—it's dated on August 2, 2025. After reading the story behind it, we can see that the developer was initially fixing something completely different. Then, they spot the sanitizer problems and switched to redo applyFixup across multiple backends. Higher up the stack, in the MCAssembler::layout function, an assert appeared. It enables passing the Data parameter as a pointer without worrying about length checks: applyFixup MCAssembler::layout assert Data assert(mc::isRelocRelocation(Fixup.getKind()) || Fixup.getOffset() <= F.getFixedSize()) assert(mc::isRelocRelocation(Fixup.getKind()) || Fixup.getOffset() <= F.getFixedSize()) Due to such a huge unplanned rework, no joke, the developer ended up with 19 (!!!) backends. Yet, in one of them, a dead check emerged. The GitHub commit is pleasantly green, filled with a huge bunch of labels for every touched backend. Although we couldn't find why Lanai stands out with a darker shade among all the others. As for how to fix the remaining issue: should the if block be deleted, adjusted, or should the function's logic be rewritten entirely? Only the developer can say for sure. We hope they (or other contributors) will pay attention to this. if Fragment N10 Fragment N10 The PVS-Studio warning: V547 Expression 'IsLeaf' is always false. PPCInstrInfo.cpp 419 V547 PPCInstrInfo.cpp 419 bool PPCInstrInfo::getFMAPatterns(....) const { .... auto IsReassociableFMA = [&](const MachineInstr &Instr, int16_t &AddOpIdx, int16_t &MulOpIdx, bool IsLeaf) { .... if (IsLeaf) // 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()); // <= } .... } bool PPCInstrInfo::getFMAPatterns(....) const { .... auto IsReassociableFMA = [&](const MachineInstr &Instr, int16_t &AddOpIdx, int16_t &MulOpIdx, bool IsLeaf) { .... if (IsLeaf) // 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()); // <= } .... } It's a classic case of...yeah, refactoring. The commit points out that the early exit check was more intricate: commit if (IsAdd && IsLeaf) if (IsAdd && IsLeaf) During refactoring, the developer separated the initial lambda into two dedicated ones: one handling addition and another handling Fused Multiply-Add (FMA). This change made part of the old IsAdd to become redundant, but the end of the function was overlooked and not refactored. IsAdd Given the function's logic, the final return statement should most likely be replaced with: return MRI->hasOneNonDBGUse(OpAdd.getReg()); return MRI->hasOneNonDBGUse(OpAdd.getReg()); Fragment N11 Fragment N11 The PVS-Studio warning: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 5820, 5823. PPCInstrInfo.cpp 5823 V581 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; .... } 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; .... } The analyzer issued another warning for the PPCInstrInfo.cpp file from the previous code snippet, but it has a more complex background. Trying to figure out how this happened naturally makes my head dizzy. There was the commit with a change to the if statement and its edit, a commit revert due to sanitizer, a re-merge, etc.—all on the same day. The code went wild. For example, at one point, another check was doubled: PPCInstrInfo.cpp commit if doubled 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 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; .... } Let's make a quick relaxing test to check for side effects: bool isImm() const { return OpKind == MO_Immediate; } bool isReg() const { return OpKind == MO_Register; } bool isFI() const { return OpKind == MO_FrameIndex; } bool isImm() const { return OpKind == MO_Immediate; } bool isReg() const { return OpKind == MO_Register; } bool isFI() const { return OpKind == MO_FrameIndex; } LLVM is huge and complex but is nevertheless actively being developed, partly due to its easy acceptance of pull requests. For example, developers aren't required to run the entire titanic volume of tests before submitting a pull request. Commit rollbacks, known as "revert to green", are encouraged and considered a normal development practice. However, it can lead to unexpected consequences caused by corrupted merges. Fragment N12 Fragment N12 The PVS-Studio warning: V547 Expression 'Opcode == Instruction::FDiv' is always false. GISelValueTracking.cpp 1451 V547 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. .... } } .... } 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. .... } } .... } The last few warnings were fairly harmless. Now, let's move on to more significant logical errors. Unlike previous fragments, this entire titanic switch (almost 1,000 lines of code!) was added at once, and only afterward did developers patch small pieces outside the case branch we're looking at. switch added case By the way, there's a similar oddity above: 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 .... } .... } 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 .... } .... } Comparisons between two different enums look odd. Yet, apart from these two places, the Instruction values aren't used anywhere else, whereas TargetOpcode is used not only in the case statements, but in various checks and comparisons. It seems to be a copy-paste error: the first comparison should involve TargetOpcode::G_FDIV, and the second one should use TargetOpcode::G_FADD. enum Instruction TargetOpcode case TargetOpcode::G_FDIV TargetOpcode::G_FADD Fragment N13 Fragment N13 The PVS-Studio warning: V560 A part of conditional expression is always false: Defs.count(R) > 1. HexagonMCChecker.cpp 612 V560 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; } .... } 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; } .... } At first glance, this looks like perfectly ordinary code until we realize thatDefs is a DenseMap: Defs DenseMap /// Return 1 if the specified key is in the map, 0 otherwise. size_type count(const_arg_type_t Val) const { return contains(Val) ? 1 : 0; } /// Return 1 if the specified key is in the map, 0 otherwise. size_type count(const_arg_type_t Val) const { return contains(Val) ? 1 : 0; } As we can see, error logging will never be executed, and, moreover, execution will always continue with potentially incorrect behavior. Here's what git blame reports: the code celebrates its tenth anniversary in November. One might think its meaning had changed over time. But no, history confirms that Defs has always been DenseMap, and its count function only returned 0 or 1 back then. If we search for other similar checks, we see that the count result is used as a binary value everywhere. git blame tenth Defs DenseMap count 0 1 count Here are a few examples: if (Defs.count(Reg)) if (Defs.count(Reg) && !MO.isDead()) if (Uses.count(DstReg) || Defs.count(SrcReg)) if (Defs.count(Reg)) if (Defs.count(Reg) && !MO.isDead()) if (Uses.count(DstReg) || Defs.count(SrcReg)) It seems the developers were trying to makeDenseMap behave similarly to standard library containers, where std::map and std::multimap have a common count interface. In the case of std::multimap, it may return more than one match. The commit text confirms it: developers changed the return type of the count member function from bool to size_type: 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. The count() function for STL datatypes returns unsigned, even where it's only 1/0 result like std::set. Here's an explanation from the LLVM code review website: If we want our container-like types to be containers, count should return size_type. If we want our container-like types to be containers, count should return size_type. As a result, either a different container should be used, or the correct check should look something like this: if (isLoopRegister(R) && Defs.count(R) && (HexagonMCInstrInfo::isInnerLoop(MCB) || HexagonMCInstrInfo::isOuterLoop(MCB))) if (isLoopRegister(R) && Defs.count(R) && (HexagonMCInstrInfo::isInnerLoop(MCB) || HexagonMCInstrInfo::isOuterLoop(MCB))) Fragment N14 Fragment N14 The PVS-Studio warning: V560 A part of conditional expression is always false: AVX10Ver >= 2. Host.cpp 2177 V560 Host.cpp 2177 StringMap 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; } StringMap 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; } While the last couple of warnings could be summed up as "False, false, and nothing but false", this one is slightly more nuanced. In the HasLeaf24 && (EBX & 0xff) expression, both operands of && are first converted to the bool type, the logical AND is evaluated, and the result is expanded back to the int type. The output is either 0 or 1, and the AVX10Ver >= 2 expression always evaluates to false. HasLeaf24 && (EBX & 0xff) && bool AND int 0 1 AVX10Ver >= 2 false Looking at history, the previous iteration of the code was something like this: history bool HasLeaf24 = MaxLevel >= 0x24 && !getX86CpuIDAndInfo(0x24, &EAX, &EBX, &ECX, &EDX); Features["avx10.1-512"] = Features["avx10.1-256"] && HasLeaf24 && ((EBX >> 18) & 1); bool HasLeaf24 = MaxLevel >= 0x24 && !getX86CpuIDAndInfo(0x24, &EAX, &EBX, &ECX, &EDX); Features["avx10.1-512"] = Features["avx10.1-256"] && HasLeaf24 && ((EBX >> 18) & 1); Let's take a look at what the function does in general. It collects the supported processor features in Features, i.e., the values of the CPUID instruction. Features CPUID Open the Intel Advanced Vector Extension 10.1 specification for CPUID: CPUID CPUID.(EAX=24H, ECX=00H):EBX[bit 18] If 1, indicates that 512-bit vector support is present. CPUID.(EAX=24H, ECX=00H):EBX[bit 18] If 1, indicates that 512-bit vector support is present. Everything matches the original version, but in the Advanced Vector Extension 10.2 specification, which the developer added support for, it's already different: CPUID.(EAX=0x07,ECX=0x01):EDX.AVX10[19] and (CPUID.(EAX=0x24,ECX=0x0):EBX[7:0] >= 2) CPUID.(EAX=0x07,ECX=0x01):EDX.AVX10[19] and (CPUID.(EAX=0x24,ECX=0x0):EBX[7:0] >= 2) The value range changed from one bit to eight: from zero to seven in EBX, which actually ends up in EBX & 0xff and no longer fits in bool. EBX EBX & 0xff bool According to the specification, the correct code should be, for example, as follows: int AVX10Ver = EBX & 0xff; Features["avx10.1"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 1; Features["avx10.2"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 2; int AVX10Ver = EBX & 0xff; Features["avx10.1"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 1; Features["avx10.2"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 2; Fragment N15 Fragment N15 The PVS-Studio warning: V547 Expression 'i != e' is always true. MachineFunction.cpp 1444 V547 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'; } 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'; } This warning is related to printer functions. Initially, the loop outputs values in a single line: Initially 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'; 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'; Maybe the developer supposed if (i != e) to prevent two consecutive line breaks after the last line, but the condition is wrong and doesn't work as expected. In this case, the fixed code should be something like this: 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'; } } 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'; } } Fragment N16 Fragment N16 The PVS-Studio warning: V547 Expression is always true. SystemZAsmPrinter.cpp 1324 V547 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(....){ .... 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"); .... } Let's get back to the printer code. This is a peculiar check because if you set the bits in the value and then check their presence using the bitwise AND operation, of course, those bits will still be set. Meanwhile, all the other flags in this function are validated conditionally, based on the parameters passed in. AND static void emitPPA1Flags(std::unique_ptr &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. .... } static void emitPPA1Flags(std::unique_ptr &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. .... } One might assume that the check was made for consistency with other flags. But then the else branch makes the whole thing even stranger. The commit message gives us a hint: else commit Add the PPA1 to SystemZAsmPrinter. Add the PPA1 to SystemZAsmPrinter. Okay, let's refer to the IBM documentation: 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. 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. It confirms that a 32-bit version is indeed possible, but only the developer can clarify what behavior was intended here. Fragment N17 Fragment N17 The PVS-Studio warning: V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 1995 V1048 MachineInstr.cpp 1995 void MachineInstr::print(....) const { .... if (MCSymbol *PreInstrSymbol = getPreInstrSymbol()) { if (!FirstOp) { FirstOp = false; OS << ','; } OS << " pre-instr-symbol "; MachineOperand::printSymbol(OS, *PreInstrSymbol); } .... } void MachineInstr::print(....) const { .... if (MCSymbol *PreInstrSymbol = getPreInstrSymbol()) { if (!FirstOp) { FirstOp = false; OS << ','; } OS << " pre-instr-symbol "; MachineOperand::printSymbol(OS, *PreInstrSymbol); } .... } There's the final chunk of printer issues. This time, the problem is with a redundant assignment, not output. There are five such warnings in this function. If we trace the history, we see that the original pattern looked like this: if (!FirstOp) { OS << ','; } if (!FirstOp) { OS << ','; } Then, the first block of this type was added to include an assignment: added if (!FirstOp) { FirstOp = false; OS << ','; } if (!FirstOp) { FirstOp = false; OS << ','; } Then, similar fragments began to appear, most likely copied and pasted by different developers. As a result, we now have five redundant lines that do nothing and shouldn't be there at all. Other warnings in the file: V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2003 V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2011 V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2019 V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2027 V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2003 V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2011 V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2019 V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 2027 Fragment N18 Fragment N18 The PVS-Studio warning: V547 Expression 'AllSame' is always true. SimplifyCFG.cpp 1914 V547 SimplifyCFG.cpp 1914 bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(....) { .... bool AllSame = none_of(....); if (!AllSame) return false; if (AllSame) { .... } .... } bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(....) { .... bool AllSame = none_of(....); if (!AllSame) return false; if (AllSame) { .... } .... } Debugging a compiler is a very challenging endeavor, that's why the LLVM developers often write various checks and asserts. However, some of them make you question your sanity and perception of reality. This is one of them. No, this is neither a merge issue nor a refactoring error—it was added at once. assert at once Fragment N19 Fragment N19 The PVS-Studio warning: V547 Expression 'AbbrevDecl' is always true. LVDWARFReader.cpp 398 V547 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); .... } LVScope *LVDWARFReader::processOneDie(....) { .... if (const DWARFAbbreviationDeclaration *AbbrevDecl = TheDIE.getAbbreviationDeclarationPtr()) if (AbbrevDecl) for (const DWARFAbbreviationDeclaration::AttributeSpec &AttrSpec : AbbrevDecl->attributes()) processOneAttribute(TheDIE, &CurrentEndOffset, AttrSpec); .... } Here's another paranormal check. The inner if is redundant because when execution reaches it, AbbrevDecl always be non-zero anyway. if AbbrevDecl Fragment N20 Fragment N20 The PVS-Studio warning: V791 The initial value of the index in the nested loop equals 'I'. Perhaps, 'I + 1' should be used instead. LoopUnrollAndJam.cpp 793 V791 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; .... } .... } 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; .... } .... } And finally, for a change of pace, let's look at a small optimization warning. In the comparison function, we can see that it immediately returns true when the pointer arguments are equal: true static bool checkDependency(Instruction *Src, Instruction *Dst, ....) { if (Src == Dst) return true; .... } static bool checkDependency(Instruction *Src, Instruction *Dst, ....) { if (Src == Dst) return true; .... } As the analyzer correctly pointed out, since the comparison function early and has no side effects, there's no point in starting the inner loop at I instead of I + 1. I I + 1 Outro As we've seen today, even seasoned developers, as LLVM contributors, sometimes make mistakes: both logical and copy-paste errors, not to mention the remnants of incorrect automatic merges. It's okay! That's perfectly normal; even mighty dragons need to maintain good code hygiene. And if you want help with that, remember that you can use the free PVS-Studio version for open-source projects. open-source projects