Fixes simple issue found static analyzer #169958
Open
+20 −32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Member
| @llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-transforms Author: None (Seraphimt) ChangesHi! I found several issues in LLVM code using static analyzer tool PVS-studio. I hope this will be helpful.
Full diff: https://github.com/llvm/llvm-project/pull/169958.diff 9 Files Affected:
diff --git a/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp b/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp index ecba323f8d6bf..125b5ad6d57d0 100644 --- a/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp +++ b/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp @@ -1373,7 +1373,7 @@ void GISelValueTracking::computeKnownFPClass(Register R, (KnownLHS.isKnownNeverInfinity() || KnownRHS.isKnownNeverInfinity())) Known.knownNot(fcNan); - if (Opcode == Instruction::FAdd) { + if (Opcode == TargetOpcode::G_FADD) { if (KnownLHS.cannotBeOrderedLessThanZero() && KnownRHS.cannotBeOrderedLessThanZero()) Known.knownNot(KnownFPClass::OrderedLessThanZeroMask); @@ -1488,7 +1488,7 @@ void GISelValueTracking::computeKnownFPClass(Register R, KnownLHS, Depth + 1); } - if (Opcode == Instruction::FDiv) { + if (Opcode == TargetOpcode::G_FDIV) { // Only 0/0, Inf/Inf produce NaN. if (KnownLHS.isKnownNeverNaN() && KnownRHS.isKnownNeverNaN() && (KnownLHS.isKnownNeverInfinity() || diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp index 634547ded992f..8d7694537e07d 100644 --- a/llvm/lib/CodeGen/MachineFunction.cpp +++ b/llvm/lib/CodeGen/MachineFunction.cpp @@ -1439,8 +1439,7 @@ void MachineJumpTableInfo::print(raw_ostream &OS) const { OS << printJumpTableEntryReference(i) << ':'; for (const MachineBasicBlock *MBB : JumpTables[i].MBBs) OS << ' ' << printMBBReference(*MBB); - if (i != e) - OS << '\n'; + OS << '\n'; } OS << '\n'; diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp index 18111156efa4f..c801d3deafa0f 100644 --- a/llvm/lib/CodeGen/MachineInstr.cpp +++ b/llvm/lib/CodeGen/MachineInstr.cpp @@ -2011,7 +2011,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, // operands. if (MCSymbol *PreInstrSymbol = getPreInstrSymbol()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " pre-instr-symbol "; @@ -2019,7 +2018,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MCSymbol *PostInstrSymbol = getPostInstrSymbol()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " post-instr-symbol "; @@ -2027,7 +2025,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MDNode *HeapAllocMarker = getHeapAllocMarker()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " heap-alloc-marker "; @@ -2035,7 +2032,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MDNode *PCSections = getPCSections()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " pcsections "; @@ -2043,7 +2039,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MDNode *MMRA = getMMRAMetadata()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " mmra "; diff --git a/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp b/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp index 3ba5061718144..772d821dcda81 100644 --- a/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp +++ b/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp @@ -395,10 +395,9 @@ LVScope *LVDWARFReader::processOneDie(const DWARFDie &InputDIE, LVScope *Parent, if (abbrCode) { if (const DWARFAbbreviationDeclaration *AbbrevDecl = TheDIE.getAbbreviationDeclarationPtr()) - if (AbbrevDecl) - for (const DWARFAbbreviationDeclaration::AttributeSpec &AttrSpec : - AbbrevDecl->attributes()) - processOneAttribute(TheDIE, &CurrentEndOffset, AttrSpec); + for (const DWARFAbbreviationDeclaration::AttributeSpec &AttrSpec : + AbbrevDecl->attributes()) + processOneAttribute(TheDIE, &CurrentEndOffset, AttrSpec); } }; diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp index 904577b8233d5..fe1640e2bdd6a 100644 --- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp +++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp @@ -5732,7 +5732,7 @@ void AArch64InstrInfo::copyPhysReg(MachineBasicBlock &MBB, AArch64::FPR8RegClass.contains(SrcReg)) { if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && - !Subtarget.hasZeroCycleRegMoveFPR64() && Subtarget.isNeonAvailable()) { + !Subtarget.hasZeroCycleRegMoveFPR32() && Subtarget.isNeonAvailable()) { MCRegister DestRegQ = RI.getMatchingSuperReg(DestReg, AArch64::bsub, &AArch64::FPR128RegClass); MCRegister SrcRegQ = RI.getMatchingSuperReg(SrcReg, AArch64::bsub, diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp index 366a7b6d0135a..94348608d6603 100644 --- a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp +++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp @@ -416,7 +416,7 @@ bool PPCInstrInfo::getFMAPatterns(MachineInstr &Root, // 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()); + return MRI->hasOneNonDBGUse(OpAdd.getReg()); }; int16_t AddOpIdx = -1; @@ -5809,9 +5809,6 @@ bool PPCInstrInfo::getMemOperandWithOffsetWidth( 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; if (!LdSt.hasOneMemOperand()) return false; diff --git a/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp b/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp index 9b47d237f0702..eceddd66e39d5 100644 --- a/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp +++ b/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp @@ -694,7 +694,7 @@ class VEOperand : public MCParsedAsmOperand { if (!ConstExpr) return false; unsigned regIdx = ConstExpr->getValue(); - if (regIdx > 31 || MISCRegs[regIdx] == VE::NoRegister) + if (regIdx >= std::size(MISCRegs) || MISCRegs[regIdx] == VE::NoRegister) return false; Op.Kind = k_Register; Op.Reg.Reg = MISCRegs[regIdx]; diff --git a/llvm/lib/TargetParser/Host.cpp b/llvm/lib/TargetParser/Host.cpp index cb793d60a286f..d4aed19b2cb68 100644 --- a/llvm/lib/TargetParser/Host.cpp +++ b/llvm/lib/TargetParser/Host.cpp @@ -2195,9 +2195,9 @@ StringMap<bool> sys::getHostCPUFeatures() { 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; + int AVX10Ver = EBX & 0xff; + Features["avx10.1"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 1; + Features["avx10.2"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 2; return Features; } diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index ed2a5c292fa54..3285147539501 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -1909,17 +1909,15 @@ bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(Instruction *TI, }); if (!AllSame) return false; - if (AllSame) { - LockstepReverseIterator<true> LRI(Succs); - while (LRI.isValid()) { - Instruction *I0 = (*LRI)[0]; - if (any_of(*LRI, [I0](Instruction *I) { - return !areIdenticalUpToCommutativity(I0, I); - })) { - return false; - } - --LRI; + LockstepReverseIterator<true> LRI(Succs); + while (LRI.isValid()) { + Instruction *I0 = (*LRI)[0]; + if (any_of(*LRI, [I0](Instruction *I) { + return !areIdenticalUpToCommutativity(I0, I); + })) { + return false; } + --LRI; } // Now we know that all instructions in all successors can be hoisted. Let // the loop below handle the hoisting. |
Member
| @llvm/pr-subscribers-llvm-globalisel Author: None (Seraphimt) ChangesHi! I found several issues in LLVM code using static analyzer tool PVS-studio. I hope this will be helpful.
Full diff: https://github.com/llvm/llvm-project/pull/169958.diff 9 Files Affected:
diff --git a/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp b/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp index ecba323f8d6bf..125b5ad6d57d0 100644 --- a/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp +++ b/llvm/lib/CodeGen/GlobalISel/GISelValueTracking.cpp @@ -1373,7 +1373,7 @@ void GISelValueTracking::computeKnownFPClass(Register R, (KnownLHS.isKnownNeverInfinity() || KnownRHS.isKnownNeverInfinity())) Known.knownNot(fcNan); - if (Opcode == Instruction::FAdd) { + if (Opcode == TargetOpcode::G_FADD) { if (KnownLHS.cannotBeOrderedLessThanZero() && KnownRHS.cannotBeOrderedLessThanZero()) Known.knownNot(KnownFPClass::OrderedLessThanZeroMask); @@ -1488,7 +1488,7 @@ void GISelValueTracking::computeKnownFPClass(Register R, KnownLHS, Depth + 1); } - if (Opcode == Instruction::FDiv) { + if (Opcode == TargetOpcode::G_FDIV) { // Only 0/0, Inf/Inf produce NaN. if (KnownLHS.isKnownNeverNaN() && KnownRHS.isKnownNeverNaN() && (KnownLHS.isKnownNeverInfinity() || diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp index 634547ded992f..8d7694537e07d 100644 --- a/llvm/lib/CodeGen/MachineFunction.cpp +++ b/llvm/lib/CodeGen/MachineFunction.cpp @@ -1439,8 +1439,7 @@ void MachineJumpTableInfo::print(raw_ostream &OS) const { OS << printJumpTableEntryReference(i) << ':'; for (const MachineBasicBlock *MBB : JumpTables[i].MBBs) OS << ' ' << printMBBReference(*MBB); - if (i != e) - OS << '\n'; + OS << '\n'; } OS << '\n'; diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp index 18111156efa4f..c801d3deafa0f 100644 --- a/llvm/lib/CodeGen/MachineInstr.cpp +++ b/llvm/lib/CodeGen/MachineInstr.cpp @@ -2011,7 +2011,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, // operands. if (MCSymbol *PreInstrSymbol = getPreInstrSymbol()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " pre-instr-symbol "; @@ -2019,7 +2018,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MCSymbol *PostInstrSymbol = getPostInstrSymbol()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " post-instr-symbol "; @@ -2027,7 +2025,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MDNode *HeapAllocMarker = getHeapAllocMarker()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " heap-alloc-marker "; @@ -2035,7 +2032,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MDNode *PCSections = getPCSections()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " pcsections "; @@ -2043,7 +2039,6 @@ void MachineInstr::print(raw_ostream &OS, ModuleSlotTracker &MST, } if (MDNode *MMRA = getMMRAMetadata()) { if (!FirstOp) { - FirstOp = false; OS << ','; } OS << " mmra "; diff --git a/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp b/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp index 3ba5061718144..772d821dcda81 100644 --- a/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp +++ b/llvm/lib/DebugInfo/LogicalView/Readers/LVDWARFReader.cpp @@ -395,10 +395,9 @@ LVScope *LVDWARFReader::processOneDie(const DWARFDie &InputDIE, LVScope *Parent, if (abbrCode) { if (const DWARFAbbreviationDeclaration *AbbrevDecl = TheDIE.getAbbreviationDeclarationPtr()) - if (AbbrevDecl) - for (const DWARFAbbreviationDeclaration::AttributeSpec &AttrSpec : - AbbrevDecl->attributes()) - processOneAttribute(TheDIE, &CurrentEndOffset, AttrSpec); + for (const DWARFAbbreviationDeclaration::AttributeSpec &AttrSpec : + AbbrevDecl->attributes()) + processOneAttribute(TheDIE, &CurrentEndOffset, AttrSpec); } }; diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp index 904577b8233d5..fe1640e2bdd6a 100644 --- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp +++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp @@ -5732,7 +5732,7 @@ void AArch64InstrInfo::copyPhysReg(MachineBasicBlock &MBB, AArch64::FPR8RegClass.contains(SrcReg)) { if (Subtarget.hasZeroCycleRegMoveFPR128() && !Subtarget.hasZeroCycleRegMoveFPR64() && - !Subtarget.hasZeroCycleRegMoveFPR64() && Subtarget.isNeonAvailable()) { + !Subtarget.hasZeroCycleRegMoveFPR32() && Subtarget.isNeonAvailable()) { MCRegister DestRegQ = RI.getMatchingSuperReg(DestReg, AArch64::bsub, &AArch64::FPR128RegClass); MCRegister SrcRegQ = RI.getMatchingSuperReg(SrcReg, AArch64::bsub, diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp index 366a7b6d0135a..94348608d6603 100644 --- a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp +++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp @@ -416,7 +416,7 @@ bool PPCInstrInfo::getFMAPatterns(MachineInstr &Root, // 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()); + return MRI->hasOneNonDBGUse(OpAdd.getReg()); }; int16_t AddOpIdx = -1; @@ -5809,9 +5809,6 @@ bool PPCInstrInfo::getMemOperandWithOffsetWidth( 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; if (!LdSt.hasOneMemOperand()) return false; diff --git a/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp b/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp index 9b47d237f0702..eceddd66e39d5 100644 --- a/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp +++ b/llvm/lib/Target/VE/AsmParser/VEAsmParser.cpp @@ -694,7 +694,7 @@ class VEOperand : public MCParsedAsmOperand { if (!ConstExpr) return false; unsigned regIdx = ConstExpr->getValue(); - if (regIdx > 31 || MISCRegs[regIdx] == VE::NoRegister) + if (regIdx >= std::size(MISCRegs) || MISCRegs[regIdx] == VE::NoRegister) return false; Op.Kind = k_Register; Op.Reg.Reg = MISCRegs[regIdx]; diff --git a/llvm/lib/TargetParser/Host.cpp b/llvm/lib/TargetParser/Host.cpp index cb793d60a286f..d4aed19b2cb68 100644 --- a/llvm/lib/TargetParser/Host.cpp +++ b/llvm/lib/TargetParser/Host.cpp @@ -2195,9 +2195,9 @@ StringMap<bool> sys::getHostCPUFeatures() { 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; + int AVX10Ver = EBX & 0xff; + Features["avx10.1"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 1; + Features["avx10.2"] = HasAVX10 && HasLeaf24 && AVX10Ver >= 2; return Features; } diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index ed2a5c292fa54..3285147539501 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -1909,17 +1909,15 @@ bool SimplifyCFGOpt::hoistCommonCodeFromSuccessors(Instruction *TI, }); if (!AllSame) return false; - if (AllSame) { - LockstepReverseIterator<true> LRI(Succs); - while (LRI.isValid()) { - Instruction *I0 = (*LRI)[0]; - if (any_of(*LRI, [I0](Instruction *I) { - return !areIdenticalUpToCommutativity(I0, I); - })) { - return false; - } - --LRI; + LockstepReverseIterator<true> LRI(Succs); + while (LRI.isValid()) { + Instruction *I0 = (*LRI)[0]; + if (any_of(*LRI, [I0](Instruction *I) { + return !areIdenticalUpToCommutativity(I0, I); + })) { + return false; } + --LRI; } // Now we know that all instructions in all successors can be hoisted. Let // the loop below handle the hoisting. |
20 tasks
arsenm reviewed Nov 30, 2025
Contributor
arsenm left a comment
arsenm left a comment There was a problem hiding this comment.
Ideally this would be split into separate PRs for each of these contexts, particularly the ones that show the behavior is broken
| Known.knownNot(fcNan); | ||
| | ||
| if (Opcode == Instruction::FAdd) { | ||
| if (Opcode == TargetOpcode::G_FADD) { |
Contributor
There was a problem hiding this comment.
Clearly there is missing test coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Hi! I found several issues in LLVM code using static analyzer tool PVS-studio. I hope this will be helpful.
Full descriptions in the article: https://pvs-studio.com/en/blog/posts/cpp/1318/
I took the liberty of correcting some simple fragments myself:
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
The PVS-Studio warning: V557 Array overrun is possible. The value of 'regIdx' index could reach 31. VEAsmParser.cpp 696
The PVS-Studio warning: V547 Expression 'IsLeaf' is always false. PPCInstrInfo.cpp 419
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
The PVS-Studio warning: V547 Expression 'Opcode == Instruction::FDiv' is always false. GISelValueTracking.cpp 1451
The PVS-Studio warning: V560 A part of conditional expression is always false: AVX10Ver >= 2. Host.cpp 2177
The PVS-Studio warning: V547 Expression 'i != e' is always true. MachineFunction.cpp 1444
The PVS-Studio warning: V1048 The 'FirstOp' variable was assigned the same value. MachineInstr.cpp 1995
The PVS-Studio warning: V547 Expression 'AllSame' is always true. SimplifyCFG.cpp 1914
The PVS-Studio warning: V547 Expression 'AbbrevDecl' is always true. LVDWARFReader.cpp 398