It's been more than two years since the last code review of the LLVM project with our PVS-Studio analyzer. Let's ensure that the PVS-Studio analyzer is still the leading tool for detecting errors and potential vulnerabilities. To do this, we will check and find new mistakes in the LLVM 8.0.0 release.
An article that should be written
To be honest, I didnāt want to write this article. Itās not interesting to write about a project weāve already reviewed numerous times (, , ). It would be better to write about something new, but I have no choice.
Every time a new version of LLVM is released or updated , we receive questions in our inbox like this:
Look, the new version of Clang Static Analyzer has learned to find new bugs! It seems to me that the relevance of using PVS-Studio is decreasing. Clang is finding more bugs than before and catching up with PVS-Studio's capabilities. What do you think about this?
My usual response is something along the lines of:
We are not idle either! We have significantly improved the capabilities of the PVS-Studio analyzer. So donāt worry, we continue to lead as before.
Unfortunately, thatās a poor answer. It lacks proof. And thatās precisely why I am writing this article now. So, the LLVM project has once again been reviewed, and a variety of errors were found. I will now demonstrate the ones I found interesting. These errors cannot be found by the Clang Static Analyzer (or it is extremely inconvenient to do so with its help). But we can. In fact, I found and documented all these errors in one evening.
However, writing the article dragged on for several weeks. I just couldnāt bring myself to put it all into text :).
By the way, if youāre interested in what technologies are used in the PVS-Studio analyzer to detect errors and potential vulnerabilities, I suggest you check out this .
New and Old Diagnostics
As already noted, about two years ago, the LLVM project was reviewed again, and the identified errors were corrected. Now this article will present a new set of errors. Why were new errors found? There are three reasons for this:
- The LLVM project is evolving; old code is being modified, and new code is being added. Naturally, there are new errors in the changed and newly written code. This clearly demonstrates that static analysis should be applied regularly, not occasionally. Our articles effectively showcase the capabilities of the PVS-Studio analyzer, but this has no correlation with improving code quality and reducing the cost of fixing errors. Use a static code analyzer regularly!
- We are refining and enhancing the existing diagnostics. Therefore, the analyzer can identify errors that were missed in previous checks.
- New diagnostics have appeared in PVS-Studio that weren't available two years ago. I decided to highlight them in a separate section to visually demonstrate the development of PVS-Studio.
Defects identified by diagnostics that existed two years ago
Fragment N1: Copy-Paste
static bool ShouldUpgradeX86Intrinsic(Function *F, StringRef Name) {
if (Name == "addcarryx.u32" || // Added in 8.0
....
Name == "avx512.mask.cvtps2pd.128" || // Added in 7.0
Name == "avx512.mask.cvtps2pd.256" || // Added in 7.0
Name == "avx512.cvtusi2sd" || // Added in 7.0
Name.startswith("avx512.mask.permvar.") || // Added in 7.0 // <=
Name.startswith("avx512.mask.permvar.") || // Added in 7.0 // <=
Name == "sse2.pmulu.dq" || // Added in 7.0
Name == "sse41.pmuldq" || // Added in 7.0
Name == "avx2.pmulu.dq" || // Added in 7.0
....
}PVS-Studio Warning: [CWE-570] There are identical sub-expressions āName.startswith(Ā«avx512.mask.permvar.Ā»)ā to the left and to the right of the ā||ā operator. AutoUpgrade.cpp 73
It is checked twice that the name starts with the substring Ā«avx512.mask.permvar.Ā». In the second check, something else was explicitly intended to be written, but the copied text was left unchanged.
Fragment N2: Typo
enum CXNameRefFlags {
CXNameRange_WantQualifier = 0x1,
CXNameRange_WantTemplateArgs = 0x2,
CXNameRange_WantSinglePiece = 0x4
};
void AnnotateTokensWorker::HandlePostPonedChildCursor(
CXCursor Cursor, unsigned StartTokenIndex) {
const auto flags = CXNameRange_WantQualifier | CXNameRange_WantQualifier;
....
}PVS-Studio Warning: V501 There are identical sub-expressions āCXNameRange_WantQualifierā to the left and to the right of the ā|ā operator. CIndex.cpp 7245
Due to a typo, the same named constant is used twice CXNameRange_WantQualifier.
Fragment N3: Operator Precedence Confusion
int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
....
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
return 0;
....
}PVS-Studio Warning: [CWE-783] Perhaps the ā?:ā operator works in a different way than it was expected. The ā?:ā operator has a lower priority than the ā==ā operator. PPCTargetTransformInfo.cpp 404
In my opinion, this is a very beautiful mistake. Yes, I know I have strange views on beauty :).
Currently, according to , the expression is evaluated as follows:
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0From a practical standpoint, this condition is meaningless as it can be simplified to:
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())This is a clear error. Most likely, 0/1 were meant to be compared with the variable Index. To fix the code, parentheses need to be added around the ternary operator:
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))By the way, the ternary operator is very dangerous and can lead to logical errors. Be very cautious with it and don't hesitate to use parentheses. I discussed this topic in , in the chapter 'Beware of the ?: operator and enclose it in parentheses.'
Fragment N4, N5: Null pointer
Init *TGParser::ParseValue(Record *CurRec, RecTy *ItemType, IDParseMode Mode) {
....
TypedInit *LHS = dyn_cast(Result);
....
LHS = dyn_cast(
UnOpInit::get(UnOpInit::CAST, LHS, StringRecTy::get())
->Fold(CurRec));
if (!LHS) {
Error(PasteLoc, Twine("can't cast '") + LHS->getAsString() +
"' to string");
return nullptr;
}
....
}PVS-Studio Warning: [CWE-476] Dereferencing of the null pointer āLHSā might take place. TGParser.cpp 2152
If the pointer LHS is null, a warning should be issued. However, instead, dereferencing of that null pointer will occur: LHS->getAsString().
This is a very typical situation where an error is hidden within the error handler, as no one tests them. Static analyzers check all reachable code, regardless of how often it is used. This is a very good example of how static analysis complements other testing and error-prevention methodologies.
A similar pointer handling error RHS occurs in the code a little further down: V522 [CWE-476] Dereferencing of the null pointer āRHSā might take place. TGParser.cpp 2186
Fragment N6: Using a pointer after moving
static Expected
ExtractBlocks(....)
{
....
std::unique_ptr ProgClone = CloneModule(BD.getProgram(), VMap);
....
BD.setNewProgram(std::move(ProgClone)); // getFunction(MisCompFunctions[i].first); // <=
assert(NewF && "Function not found??");
MiscompiledFunctions.push_back(NewF);
}
....
}PVS-Studio warning: V522 [CWE-476] Dereferencing of the null pointer āProgCloneā might take place. Miscompilation.cpp 601
At the beginning, the smart pointer ProgClone ceases to own the object:
BD.setNewProgram(std::move(ProgClone));In fact, now ProgClone This is a null pointer. Therefore, dereferencing a null pointer should happen a bit later:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);However, in reality, this will not happen! Note that the loop does not actually execute.
At the beginning of the container MiscompiledFunctions is being cleared:
MiscompiledFunctions.clear();Next, the size of this container is used in the loop condition:
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {It's easy to see that the loop does not start. I think this is also an error, and the code should be written differently.
It seems we've encountered that famous parity of errors! One error masks another :).
Fragment N7: Using a pointer after moving
static Expected TestOptimizer(BugDriver &BD, std::unique_ptr Test,
std::unique_ptr Safe) {
outs() << " Optimizing functions being tested: ";
std::unique_ptr Optimized =
BD.runPassesOn(Test.get(), BD.getPassesToRun());
if (!Optimized) {
errs() << " Error running this sequence of passes"
<< " on the input program!n";
BD.setNewProgram(std::move(Test)); // <=
BD.EmitProgressBitcode(*Test, "pass-error", false); // <=
if (Error E = BD.debugOptimizerCrash())
return std::move(E);
return false;
}
....
}Warning PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer āTestā might take place. Miscompilation.cpp 709
Once again, the same situation. Initially, the content of the object is moved, and then it is used as if nothing happened. I am encountering this situation more often in program code, after move semantics were introduced in C++. This is why I love C++! New ways to shoot oneself in the foot keep emerging. The PVS-Studio analyzer will always have work to do.:)
Fragment N8: Null pointer
void FunctionDumper::dump(const PDBSymbolTypeFunctionArg &Symbol) {
uint32_t TypeId = Symbol.getTypeId();
auto Type = Symbol.getSession().getSymbolById(TypeId);
if (Type)
Printer << "";
else
Type->dump(*this);
}Warning PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer āTypeā might take place. PrettyFunctionDumper.cpp 233
Besides error handlers, debugging data print functions are usually not tested either. This is just such a case. The function is waiting for the user, who instead of solving their problems, will have to fix it.
Correct:
if (Type)
Type->dump(*this);
else
Printer << "";Fragment N9: Null pointer
void SearchableTableEmitter::collectTableEntries(
GenericTable &Table, const std::vector &Items) {
....
RecTy *Ty = resolveTypes(Field.RecType, TI->getType());
if (!Ty) // getAsString() + " vs. " + // getType()->getAsString());
....
}PVS-Studio Warning: V522 [CWE-476] Dereferencing of the null pointer āTyā might take place. SearchableTableEmitter.cpp 614
I think everything is clear and doesn't require any explanations.
Fragment N10: Typo
bool FormatTokenLexer::tryMergeCSharpNullConditionals() {
....
auto &Identifier = *(Tokens.end() - 2);
auto &Question = *(Tokens.end() - 1);
....
Identifier->ColumnWidth += Question->ColumnWidth;
Identifier->Type = Identifier->Type; // <=
Tokens.erase(Tokens.end() - 1);
return true;
}PVS-Studio Warning: The āIdentifier->Typeā variable is assigned to itself. FormatTokenLexer.cpp 249
Thereās no point in assigning a variable to itself. Most likely, it should have been written as:
Identifier->Type = Question->Type;Fragment N11: Suspicious break
void SystemZOperand::print(raw_ostream &OS) const {
switch (Kind) {
break;
case KindToken:
OS << "Token:" << getToken();
break;
case KindReg:
OS << "Reg:" << SystemZInstPrinter::getRegisterName(getReg());
break;
....
}PVS-Studio Warning: [CWE-478] Consider inspecting the āswitchā statement. Itās possible that the first ācaseā operator is missing. SystemZAsmParser.cpp 652
There is a very suspicious operator at the beginning break. Did they forget to write something else here?
Fragment N12: Pointer check after dereference
InlineCost AMDGPUInliner::getInlineCost(CallSite CS) {
Function *Callee = CS.getCalledFunction();
Function *Caller = CS.getCaller();
TargetTransformInfo &TTI = TTIWP->getTTI(*Callee);
if (!Callee || Callee->isDeclaration())
return llvm::InlineCost::getNever("undefined callee");
....
}PVS-Studio Warning: [CWE-476] The āCalleeā pointer was utilized before it was verified against nullptr. Check lines: 172, 174. AMDGPUInline.cpp 172
Pointer Callee is dereferenced at the time of the function call getTTI.
Then it turns out that this pointer should be checked for equality nullptr:
if (!Callee || Callee->isDeclaration())But it's too late...
Fragment N13 ā Nā¦: Pointer check after dereference
The situation discussed in the previous code fragment is not unique. It appears here:
static Value *optimizeDoubleFP(CallInst *CI, IRBuilder &B,
bool isBinary, bool isPrecise = false) {
....
Function *CalleeFn = CI->getCalledFunction();
StringRef CalleeNm = CalleeFn->getName(); // getAttributes();
if (CalleeFn && !CalleeFn->isIntrinsic()) { // <=
....
}PVS-Studio Warning: V595 [CWE-476] The āCalleeFnā pointer was utilized before it was verified against nullptr. Check lines: 1079, 1081. SimplifyLibCalls.cpp 1079
And here:
void Sema::InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
const Decl *Tmpl, Decl *New,
LateInstantiatedAttrVec *LateAttrs,
LocalInstantiationScope *OuterMostScope) {
....
NamedDecl *ND = dyn_cast(New);
CXXRecordDecl *ThisContext =
dyn_cast_or_null(ND->getDeclContext()); // isCXXInstanceMember()); // <=
....
}Warning PVS-Studio: V595 [CWE-476] The āNDā pointer was utilized before it was verified against nullptr. Check lines: 532, 534. SemaTemplateInstantiateDecl.cpp 532
And here:
- V595 [CWE-476] The āUā pointer was utilized before it was verified against nullptr. Check lines: 404, 407. DWARFFormValue.cpp 404
- V595 [CWE-476] The āNDā pointer was utilized before it was verified against nullptr. Check lines: 2149, 2151. SemaTemplateInstantiate.cpp 2149
I lost interest in studying warnings with number V595. So I donāt know if there are other similar errors besides those listed here. Most likely there are.
Fragment N17, N18: Suspicious shift
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}PVS-Studio Warning: [CWE-190] Consider inspecting the ā~(Size ā 1) << 1ā expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. AArch64AddressingModes.h 260
Perhaps this is not an error, and the code works exactly as intended. But this is clearly a very suspicious place, and it needs to be checked.
Let's say the variable Size is equal to 16, and then the author of the code intended to get the value in the variable NImms to be:
1111111111111111111111111111111111111111111111111111111111100000
However, in reality, it will produce the value:
0000000000000000000000000000000011111111111111111111111111100000
The thing is that all calculations occur using the 32-bit unsigned type. Only then will this 32-bit unsigned type be implicitly extended to uint64_t. In this case, the higher bits will be zero.
The situation can be fixed like this:
uint64_t NImms = ~static_cast(Size-1) << 1;Similar situation: V629 [CWE-190] Consider inspecting the āImmr << 6ā expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. AArch64AddressingModes.h 269
Fragment N19: Missing keyword else?
void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
....
if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
// VOP2b (v_add_u32, v_sub_u32 ...) dpp use "vcc" token.
// Skip it.
continue;
} if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) { // <=
Op.addRegWithFPInputModsOperands(Inst, 2);
} else if (Op.isDPPCtrl()) {
Op.addImmOperands(Inst, 1);
} else if (Op.isImm()) {
// Handle optional arguments
OptionalIdx[Op.getImmTy()] = I;
} else {
llvm_unreachable("Invalid operand type");
}
....
}PVS-Studio Warning: [CWE-670] Consider inspecting the applicationās logic. Itās possible that āelseā keyword is missing. AMDGPUAsmParser.cpp 5655
There is no error here. Since the then-block of the first if ends with continue, it doesn't matter whether there's a keyword else or not. In any case, the code will work the same way. However, missing else makes the code more confusing and dangerous. If later continue it disappears, the code will start behaving completely differently. In my opinion, it's better to add else.
Fragment N20: Four similar typos
LLVM_DUMP_METHOD void Symbol::dump(raw_ostream &OS) const {
std::string Result;
if (isUndefined())
Result += "(undef) ";
if (isWeakDefined())
Result += "(weak-def) ";
if (isWeakReferenced())
Result += "(weak-ref) ";
if (isThreadLocalValue())
Result += "(tlv) ";
switch (Kind) {
case SymbolKind::GlobalSymbol:
Result + Name.str(); // <=
break;
case SymbolKind::ObjectiveCClass:
Result + "(ObjC Class) " + Name.str(); // <=
break;
case SymbolKind::ObjectiveCClassEHType:
Result + "(ObjC Class EH) " + Name.str(); // <=
break;
case SymbolKind::ObjectiveCInstanceVariable:
Result + "(ObjC IVar) " + Name.str(); // <=
break;
}
OS << Result;
}PVS-Studio warnings:
- V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the 'Result + Name.str()' expression. Symbol.cpp 32
- V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the 'Result + "(ObjC Class) " + Name.str()' expression. Symbol.cpp 35
- V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the 'Result + "(ObjC Class EH) " + Name.str()' expression. Symbol.cpp 38
- V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the 'Result + "(ObjC IVar) " + Name.str()' expression. Symbol.cpp 41
Accidentally using the + operator instead of += results in meaningless constructions.
Fragment N21: Undefined behavior
static void getReqFeatures(std::map &FeaturesMap,
const std::vector &ReqFeatures) {
for (auto &R : ReqFeatures) {
StringRef AsmCondString = R->getValueAsString("AssemblerCondString");
SmallVector Ops;
SplitString(AsmCondString, Ops, ",");
assert(!Ops.empty() && "AssemblerCondString cannot be empty");
for (auto &Op : Ops) {
assert(!Op.empty() && "Empty operator");
if (FeaturesMap.find(Op) == FeaturesMap.end())
FeaturesMap[Op] = FeaturesMap.size();
}
}
}Try to find the dangerous code by yourself. And here's a distraction image, so you don't look at the answer immediately:
PVS-Studio Warning: [CWE-758] Dangerous construction is used: 'FeaturesMap[Op] = FeaturesMap.size()', where 'FeaturesMap' is of 'map' class. This may lead to undefined behavior. RISCVCompressInstEmitter.cpp 490
Problematic line:
FeaturesMap[Op] = FeaturesMap.size();If the element Op is not found, a new element is created in the map, and the current size of the map is recorded there. However, it is unknown whether the function size will be called before or after the new element is added.
Fragment N22-N24: Repeated assignments
Error MachOObjectFile::checkSymbolTable() const {
....
} else {
MachO::nlist STE = getSymbolTableEntry(SymDRI);
NType = STE.n_type; // <=
NType = STE.n_type; // <=
NSect = STE.n_sect;
NDesc = STE.n_desc;
NStrx = STE.n_strx;
NValue = STE.n_value;
}
....
}PVS-Studio Warning: [CWE-563] The āNTypeā variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1663, 1664. MachOObjectFile.cpp 1664
I think there isn't an actual error here. It's just an unnecessary repeated assignment. Still, it's a slip.
Similarly:
- V519 [CWE-563] The āB.NDescā variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 59, 61. coff2yaml.cpp 61
Fragment N25-N27: More repeated assignments
Now let's consider a slightly different case of repeated assignment.
bool Vectorizer::vectorizeLoadChain(
ArrayRef Chain,
SmallPtrSet *InstructionsProcessed) {
....
unsigned Alignment = getAlignment(L0);
....
unsigned NewAlign = getOrEnforceKnownAlignment(L0->getPointerOperand(),
StackAdjustedAlignment,
DL, L0, nullptr, &DT);
if (NewAlign != 0)
Alignment = NewAlign;
Alignment = NewAlign;
....
}PVS-Studio Warning: V519 [CWE-563] The āAlignmentā variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1158, 1160. LoadStoreVectorizer.cpp 1160
This is a very strange piece of code that apparently contains a logical error. Initially, the variable Alignment is assigned a value based on a condition. Then, another assignment occurs, but now without any checks.
Similar situations can be seen here:
- V519 [CWE-563] The āEffectsā variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] The āExpectNoDerefChunkā variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 4970, 4973. SemaType.cpp 4973
Fragment N28: Always true condition
static int readPrefixes(struct InternalInstruction* insn) {
....
uint8_t byte = 0;
uint8_t nextByte;
....
if (byte == 0xf3 && (nextByte == 0x88 || nextByte == 0x89 ||
nextByte == 0xc6 || nextByte == 0xc7)) {
insn->xAcquireRelease = true;
if (nextByte != 0x90) // PAUSE instruction support // <=
break;
}
....
}PVS-Studio Warning: [CWE-571] Expression ānextByte != 0x90ā is always true. X86DisassemblerDecoder.cpp 379
The check is meaningless. The variable nextByte is always not equal to the value 0x90, which follows from the previous check. This is some sort of logical error.
Fragment N29 ā Nā¦: Always true/false conditions
The analyzer emits many warnings regarding the fact that the entire condition () or its part () is always true or false. Often these are not real errors, but simply sloppy code, the result of macro expansions and the like. However, it makes sense to take a look at all these warnings, as sometimes genuine logical errors do occur. For example, this section of code is suspicious:
static DecodeStatus DecodeGPRPairRegisterClass(MCInst & Inst, unsigned RegNo,
uint64_t Address, const void *Decoder) {
DecodeStatus S = MCDisassembler::Success;
if (RegNo > 13)
return MCDisassembler::Fail;
if ((RegNo & 1) || RegNo == 0xe)
S = MCDisassembler::SoftFail;
....
}PVS-Studio Warning: [CWE-570] A part of conditional expression is always false: RegNo == 0xe. ARMDisassembler.cpp 939
The constant 0xE is the value 14 in decimal. The check RegNo == 0xe makes no sense, since if RegNo > 13, the function will terminate.
There were many other warnings with identifiers V547 and V560, but, like with , I found it uninteresting to study these warnings. It was already clear that I had enough material to write an article :). Therefore, it is unknown how many such errors can be identified in LLVM using PVS-Studio.
Here's an example of why studying these triggers is boring. The analyzer is completely right to issue a warning for the following code. But it is not an error.
bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
tok::TokenKind ClosingBraceKind) {
bool HasError = false;
....
HasError = true;
if (!ContinueOnSemicolons)
return !HasError;
....
}PVS-Studio Warning: V547 [CWE-570] Expression ā!HasErrorā is always false. UnwrappedLineParser.cpp 1635
Fragment N30: Suspicious return
static bool
isImplicitlyDef(MachineRegisterInfo & MRI, unsigned Reg) {
for (MachineRegisterInfo::def_instr_iterator It = MRI.def_instr_begin(Reg),
E = MRI.def_instr_end(); It != E; ++It) {
return (*It).isImplicitDef();
}
....
}PVS-Studio Warning: [CWE-670] An unconditional āreturnā within a loop. R600OptimizeVectorRegisters.cpp 63
This is either an error or a specific trick meant to clarify something for programmers reading the code. This construction does not clarify anything for me and looks very suspicious. It's better not to write like this :).
Tired? Then it's time to brew some tea or coffee.
Defects identified by new diagnostics
I think 30 triggers from old diagnostics is enough. Now let's see what interesting things can be found with the new diagnostics that have appeared in the analyzer since check. During this time, 66 general-purpose diagnostics have been added to the C++ analyzer.
Fragment N31: Unreachable code
Error CtorDtorRunner::run() {
....
if (auto CtorDtorMap =
ES.lookup(JITDylibSearchList({{&JD, true}}), std::move(Names),
NoDependenciesToRegister, true))
{
....
return Error::success();
} else
return CtorDtorMap.takeError();
CtorDtorsByPriority.clear();
return Error::success();
}PVS-Studio Warning: [CWE-561] Unreachable code detected. It is possible that an error is present. ExecutionUtils.cpp 146
As you can see, both branches of the operator if end with a call to the operator return. Consequently, the container CtorDtorsByPriority will never be cleared.
Fragment N32: Unreachable code
bool LLParser::ParseSummaryEntry() {
....
switch (Lex.getKind()) {
case lltok::kw_gv:
return ParseGVEntry(SummaryID);
case lltok::kw_module:
return ParseModuleEntry(SummaryID);
case lltok::kw_typeid:
return ParseTypeIdEntry(SummaryID); // <=
break; // <=
default:
return Error(Lex.getLoc(), "unexpected summary kind");
}
Lex.setIgnoreColonInIdentifiers(false); // <=
return false;
}PVS-Studio warning: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. LLParser.cpp 835
An interesting situation. Let's first look at this point:
return ParseTypeIdEntry(SummaryID);
break;At first glance, it seems that there is no error here. It appears that the operator break is unnecessary here and can simply be removed. However, it is not that simple.
The analyzer issues a warning for the lines:
Lex.setIgnoreColonInIdentifiers(false);
return false;And indeed, this code is unreachable. All cases in switch end with the call to the operator return. And now the pointless lone break does not seem so harmless! Perhaps one of the branches should end with break, instead of return?
Fragment N33: Random nullification of higher bits
unsigned getStubAlignment() override {
if (Arch == Triple::systemz)
return 8;
else
return 1;
}
Expected
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
const SectionRef &Section,
bool IsCode) {
....
uint64_t DataSize = Section.getSize();
....
if (StubBufSize > 0)
DataSize &= ~(getStubAlignment() - 1);
....
}PVS-Studio Warning: The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. RuntimeDyld.cpp 815
Note that the function getStubAlignment returns type unsigned. Let's compute the value of the expression, assuming the function returns 8:
~(getStubAlignment() - 1)
~(8u - 1)
0xFFFFFFF8u
Now note that the variable DataSize has a 64-bit unsigned type. Thus, when performing the operation DataSize & 0xFFFFFFF8u, all thirty-two higher bits will be cleared. This is likely not what the programmer intended. I suspect he meant to compute: DataSize & 0xFFFFFFFFFFFFFFF8u.
To fix the error, it should be written like this:
DataSize &= ~(static_cast(getStubAlignment()) - 1);Or like this:
DataSize &= ~(getStubAlignment() - 1ULL);Fragment N34: Explicit type casting failed.
template <typename T>
void scaleShuffleMask(int Scale, ArrayRef<T> Mask,
SmallVectorImpl<T> &ScaledMask) {
assert(0 < Scale && "Unexpected scaling factor");
int NumElts = Mask.size();
ScaledMask.assign(static_cast<size_t>(NumElts * Scale), -1);
....
}PVS-Studio Warning: [CWE-190] Possible overflow. Consider casting operands of the āNumElts * Scaleā operator to the āsize_tā type, not the result. X86ISelLowering.h 1577
Explicit type casting is used to avoid overflow when multiplying variables of type. int. However, here explicit type casting does not protect against overflow. Initially, the variables will be multiplied, and only then the 32-bit result of the multiplication will be cast to the type. .
Fragment N35: Unsuccessful Copy-Paste.
Instruction *InstCombiner::visitFCmpInst(FCmpInst &I) {
....
if (!match(Op0, m_PosZeroFP()) && isKnownNeverNaN(Op0, &TLI)) {
I.setOperand(0, ConstantFP::getNullValue(Op0->getType()));
return &I;
}
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op0->getType()));
return &I;
}
....
}[CWE-682] Two similar code fragments were found. Perhaps, this is a typo and āOp1ā variable should be used instead of āOp0ā. InstCombineCompares.cpp 5507
This new insightful diagnostic reveals situations where a fragment of code was copied, and some names began to change, but one place was overlooked.
Note that in the second block, they changed Op0 to Op1. But one part was left unchanged. It should have probably been written as:
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}Fragment N36: Confusion in variables.
struct Status {
unsigned Mask;
unsigned Mode;
Status() : Mask(0), Mode(0){};
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
Mode &= Mask;
};
....
};PVS-Studio Warning: [CWE-563] The āModeā variable is assigned but is not used by the end of the function. SIModeRegister.cpp 48
It is very dangerous to give function arguments the same names as class members. Itās very easy to get confused. This is exactly such a case. This expression makes no sense:
Mode &= Mask;The function argument changes. And thatās it. This argument is not used in any other way. It should have probably been written as:
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};Fragment N37: Confusion in variables.
class SectionBase {
....
uint64_t Size = 0;
....
};
class SymbolTableSection : public SectionBase {
....
};
void SymbolTableSection::addSymbol(Twine Name, uint8_t Bind, uint8_t Type,
SectionBase *DefinedIn, uint64_t Value,
uint8_t Visibility, uint16_t Shndx,
uint64_t Size) {
....
Sym.Value = Value;
Sym.Visibility = Visibility;
Sym.Size = Size;
Sym.Index = Symbols.size();
Symbols.emplace_back(llvm::make_unique(Sym));
Size += this->EntrySize;
}PVS-Studio warning: V1001 [CWE-563] The āSizeā variable is assigned but is not used by the end of the function. Object.cpp 424
The situation is similar to the previous one. It should be written as:
this->Size += this->EntrySize;Fragment N38-N47: The pointer was forgotten to be checked
Earlier, we looked at examples of diagnostic triggers . Its essence is that the pointer is dereferenced at the beginning and only then checked. This is a new diagnostic is the inverse of the previous one in meaning but also detects a lot of errors. It identifies situations where the pointer was checked at the beginning but was then forgotten. Let's consider such cases found within LLVM.
int getGEPCost(Type *PointeeType, const Value *Ptr,
ArrayRef Operands) {
....
if (Ptr != nullptr) { // <=
assert(....);
BaseGV = dyn_cast(Ptr->stripPointerCasts());
}
bool HasBaseReg = (BaseGV == nullptr);
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType()); // <=
....
}PVS-Studio warning: V1004 [CWE-476] The āPtrā pointer was used unsafely after it was verified against nullptr. Check lines: 729, 738. TargetTransformInfoImpl.h 738
The variable Ptr may equal nullptr, as indicated by the check:
if (Ptr != nullptr)However, below this pointer is dereferenced already without prior checking:
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());Let's consider another similar case.
llvm::DISubprogram *CGDebugInfo::getFunctionFwdDeclOrStub(GlobalDecl GD,
bool Stub) {
....
auto *FD = dyn_cast(GD.getDecl());
SmallVector ArgTypes;
if (FD) // parameters())
ArgTypes.push_back(Parm->getType());
CallingConv CC = FD->getType()->castAs()->getCallConv(); // <=
....
}PVS-Studio warning: V1004 [CWE-476] The āFDā pointer was used unsafely after it was verified against nullptr. Check lines: 3228, 3231. CGDebugInfo.cpp 3231
Take note of the pointer FD. I am sure the problem is well visible, and no special explanation is needed.
And one more thing:
static void computePolynomialFromPointer(Value &Ptr, Polynomial &Result,
Value *&BasePtr,
const DataLayout &DL) {
PointerType *PtrTy = dyn_cast(Ptr.getType());
if (!PtrTy) { // getPointerAddressSpace()); // <=
....
}PVS-Studio Warning: V1004 [CWE-476] The āPtrTyā pointer was used unsafely after it was verified against nullptr. Check lines: 960, 965. InterleavedLoadCombinePass.cpp 965
How to protect yourself from such errors? Be more careful during Code Review and use the static analyzer PVS-Studio for regular code checks.
It makes no sense to provide other code snippets with such types of errors. I will leave only a list of warnings in the article:
- V1004 [CWE-476] The āExprā pointer was used unsafely after it was verified against nullptr. Check lines: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] The āPIā pointer was used unsafely after it was verified against nullptr. Check lines: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] The āStatepointCallā pointer was used unsafely after it was verified against nullptr. Check lines: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] The āRVā pointer was used unsafely after it was verified against nullptr. Check lines: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] The āCalleeFnā pointer was used unsafely after it was verified against nullptr. Check lines: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] The āTCā pointer was used unsafely after it was verified against nullptr. Check lines: 1819, 1824. Driver.cpp 1824
Fragment N48-N60: Not critical, but a defect (possible memory leak)
std::unique_ptr createISelMutator() {
....
std::vector<std::unique_ptr> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}PVS-Studio Warning: [CWE-460] A pointer without an owner is added to the āStrategiesā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 58
To add an element to the end of a container of type std::vector<std::unique_ptr> you cannot simply write xxx.push_back(new X), as there is no implicit conversion from X* downward API support (simultaneously with this in std::unique_ptr.
A common solution is to write xxx.emplace_back(new X), as it compiles: the method emplace_back constructs the element directly from arguments and can therefore use explicit constructors.
This is unsafe. If the vector is full, a memory reallocation occurs. The reallocation operation may fail, resulting in a thrown exception std::bad_alloc. In this case, the pointer will be lost, and the created object will never be deleted.
A safe solution is to create a unique_ptr, which will own the pointer before the vector attempts to reallocate memory:
xxx.push_back(std::unique_ptr(new X))Starting with C++14, you can use 'std::make_unique':
xxx.push_back(std::make_unique())This type of defect is not critical for LLVM. If memory allocation fails, the compiler's operation will simply stop. However, for applications with long , which cannot simply terminate if memory cannot be allocated, this could be a serious problem.
So, while this code does not pose a practical risk for LLVM, I thought it helpful to discuss this error pattern and how the PVS-Studio analyzer has learned to detect it.
Other warnings of this type:
- V1023 [CWE-460] A pointer without an owner is added to the āPassesā container by the āemplace_backā method. A memory leak will occur in case of an exception. PassManager.h 546
- V1023 [CWE-460] A pointer without an owner is added to the āAAsā container by the āemplace_backā method. A memory leak will occur in case of an exception. AliasAnalysis.h 324
- V1023 [CWE-460] A pointer without an owner is added to the āEntriesā container by the āemplace_backā method. A memory leak will occur in case of an exception. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] A pointer without an owner is added to the āAllEdgesā container by the āemplace_backā method. A memory leak will occur in case of an exception. CFGMST.h 268
- V1023 [CWE-460] A pointer without an owner is added to the āVMapsā container by the āemplace_backā method. A memory leak will occur in case of an exception. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] A pointer without an owner is added to the āRecordsā container by the āemplace_backā method. A memory leak will occur in case of an exception. FDRLogBuilder.h 30
- V1023 [CWE-460] A pointer without an owner is added to the āPendingSubmodulesā container by the āemplace_backā method. A memory leak will occur in case of an exception. ModuleMap.cpp 810
- V1023 [CWE-460] A pointer without an owner is added to the āObjectsā container by the āemplace_backā method. A memory leak will occur in case of an exception. DebugMap.cpp 88
- V1023 [CWE-460] A pointer without an owner is added to the āStrategiesā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 685
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 686
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 688
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 689
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 690
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 691
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 692
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 693
- V1023 [CWE-460] A pointer without an owner is added to the āModifiersā container by the āemplace_backā method. A memory leak will occur in case of an exception. llvm-stress.cpp 694
- V1023 [CWE-460] A pointer without an owner is added to the āOperandsā container by the āemplace_backā method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] A pointer without an owner is added to the āStashā container by the āemplace_backā method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] A pointer without an owner is added to the āMatchersā container by the āemplace_backā method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2702
Conclusion
In total, I listed 60 warnings and then stopped. Are there other defects that the PVS-Studio analyzer detects in LLVM? Yes, there are. However, while I was compiling code snippets for the article, it was late evening, rather even night, and I decided it was time to wrap up.
I hope you found it interesting and will want to try the PVS-Studio analyzer.
You can download the analyzer and get a trial key at .
Most importantly, use static analysis regularly. One-off checks, carried out by us to popularize the methodology of static analysis and PVS-Studio, are not a normal scenario.
Good luck in improving the quality and reliability of the code!
If you want to share this article with an English-speaking audience, please use the link to the translation: Andrey Karpov. .
Source: habr.com
