Hi. There are few one-line bugs Andrey Karpov have found with static analisys.
He wrote a big article in russian on http://habrahabr.ru/blogs/compilers/125626/
for advertising purposes of static analyzer for Visual Studio his company
developed.
Most of the problems are easy to fix, so I list them in here for trunk version.
Also few problems in clang code were found, I don't list them in here.
----
lib/Target/X86/X86ISelLowering.cpp:11689
!DAG.isKnownNeverZero(LHS) && !DAG.isKnownNeverZero(LHS))
Note that there are identical subexpressions '!DAG.isKnownNeverZero
(LHS)' to
the left and to the right of the '&&' operator.
The second subexpression should probably be !DAG.isKnownNeverZero(RHS)).
----
lib/Transforms/Scalar/ObjCARC.cpp:1138
void clearBottomUpPointers() {
PerPtrTopDown.clear();
}
void clearTopDownPointers() {
PerPtrTopDown.clear();
}
Note that the body of 'clearTopDownPointers' function is fully
equivalent to
the body of 'clearBottomUpPointers' function. The advised solution is to
change
this code into
void clearBottomUpPointers() {
PerPtrBottomUp.clear();
}
----
lib/Analysis/InstructionSimplify.cpp:1891
bool NUW = LBO->hasNoUnsignedWrap() &&
LBO->hasNoUnsignedWrap();
There are identical sub-expressions 'LBO->hasNoUnsignedWrap()' to the
left and
to the right of the '&&' operator. Looks like the correct code
is
bool NUW = LBO->hasNoUnsignedWrap() &&
RBO->hasNoUnsignedWrap();
----
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1384
if (InputByteNo < ByteValues.size()/2) {
if (ByteValues.size()-1-DestByteNo != InputByteNo)
return true;
} else {
if (ByteValues.size()-1-DestByteNo != InputByteNo)
return true;
}
Note that 'then' and 'else' are the same. It can be a problem or
can not.
----
lib/Target/X86/InstPrinter/X86InstComments.cpp:208
case X86::VPERMILPSri:
DecodeVPERMILPSMask(4, MI->getOperand(2).getImm(),
ShuffleMask);
Src1Name = getRegName(MI->getOperand(0).getReg());
case X86::VPERMILPSYri:
DecodeVPERMILPSMask(8, MI->getOperand(2).getImm(),
ShuffleMask);
Src1Name = getRegName(MI->getOperand(0).getReg());
The 'Src1Name' variable is assigned values twice successively. So break
instruction should be at line 212.
----
lib/MC/MCParser/AsmLexer.cpp:149
while (CurChar != '\n' && CurChar != '\n' &&
CurChar != EOF)
There are identical sub-expressions to the left and to the right of the
'&&'
operator: CurChar != '\n' && CurChar != '\n'. The second
expression should
probably be CurChar != '\n'?
----
lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:505
result |= (icmp_eq ? (FoldMskICmp_Mask_AllZeroes |
FoldMskICmp_Mask_AllZeroes |
FoldMskICmp_AMask_Mixed |
FoldMskICmp_BMask_Mixed)
: (FoldMskICmp_Mask_NotAllZeroes |
FoldMskICmp_Mask_NotAllZeroes |
FoldMskICmp_AMask_NotMixed |
FoldMskICmp_BMask_NotMixed));
There are identical sub-expressions 'FoldMskICmp_Mask_AllZeroes' and
'FoldMskICmp_Mask_NotAllZeroes' to the left and to the right of the
'|'
operator. Probably not a bug, just some duplicated code.
----
include/llvm/Target/TargetLowering.h:267
switch (getTypeAction(Context, VT)) {
case Legal:
return VT;
case Expand:
Note that the values of different enum types (LegalizeAction and
LegalizeTypeAction) are compared. This code works well because of the same order
for enums, but it would be better to change this into
switch (getTypeAction(Context, VT)) {
case TypeLegal:
return VT;
case TypeExpand:
----
Thanks a lots for any fixes and answers.
Hi Lockal S,> ---- > > lib/Target/X86/X86ISelLowering.cpp:11689 > !DAG.isKnownNeverZero(LHS)&& !DAG.isKnownNeverZero(LHS)) > > Note that there are identical subexpressions '!DAG.isKnownNeverZero (LHS)' to > the left and to the right of the '&&' operator. > The second subexpression should probably be !DAG.isKnownNeverZero(RHS)).a patch fixing this was posted by Ivan Krasin. It seems correct, but is waiting on someone writing a testcase.> > ---- > > lib/Transforms/Scalar/ObjCARC.cpp:1138 > void clearBottomUpPointers() { > PerPtrTopDown.clear(); > } > > void clearTopDownPointers() { > PerPtrTopDown.clear(); > } > > Note that the body of 'clearTopDownPointers' function is fully equivalent to > the body of 'clearBottomUpPointers' function. The advised solution is to change > this code into > > void clearBottomUpPointers() { > PerPtrBottomUp.clear(); > }This is probably correct. Hopefully John can comment.> > ---- > > lib/Analysis/InstructionSimplify.cpp:1891 > bool NUW = LBO->hasNoUnsignedWrap()&& LBO->hasNoUnsignedWrap(); > > There are identical sub-expressions 'LBO->hasNoUnsignedWrap()' to the left and > to the right of the '&&' operator. Looks like the correct code is > > bool NUW = LBO->hasNoUnsignedWrap()&& RBO->hasNoUnsignedWrap();That is correct. Ivan sent a patch for this too and I applied it.> > ---- > > lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1384 > if (InputByteNo< ByteValues.size()/2) { > if (ByteValues.size()-1-DestByteNo != InputByteNo) > return true; > } else { > if (ByteValues.size()-1-DestByteNo != InputByteNo) > return true; > } > > Note that 'then' and 'else' are the same. It can be a problem or can not.I've CC'd Chris since he wrote this code.> > ---- > > lib/Target/X86/InstPrinter/X86InstComments.cpp:208 > case X86::VPERMILPSri: > DecodeVPERMILPSMask(4, MI->getOperand(2).getImm(), > ShuffleMask); > Src1Name = getRegName(MI->getOperand(0).getReg()); > case X86::VPERMILPSYri: > DecodeVPERMILPSMask(8, MI->getOperand(2).getImm(), > ShuffleMask); > Src1Name = getRegName(MI->getOperand(0).getReg()); > > The 'Src1Name' variable is assigned values twice successively. So break > instruction should be at line 212.I've added the missing "break".> > ---- > > lib/MC/MCParser/AsmLexer.cpp:149 > while (CurChar != '\n'&& CurChar != '\n'&& CurChar != EOF) > > There are identical sub-expressions to the left and to the right of the '&&' > operator: CurChar != '\n'&& CurChar != '\n'. The second expression should > probably be CurChar != '\n'?Chris also added this code, hopefully he will comment.> > ---- > lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:505 > result |= (icmp_eq ? (FoldMskICmp_Mask_AllZeroes | > FoldMskICmp_Mask_AllZeroes | > FoldMskICmp_AMask_Mixed | > FoldMskICmp_BMask_Mixed) > : (FoldMskICmp_Mask_NotAllZeroes | > FoldMskICmp_Mask_NotAllZeroes | > FoldMskICmp_AMask_NotMixed | > FoldMskICmp_BMask_NotMixed)); > > There are identical sub-expressions 'FoldMskICmp_Mask_AllZeroes' and > 'FoldMskICmp_Mask_NotAllZeroes' to the left and to the right of the > '|' > operator. Probably not a bug, just some duplicated code.I've CC'd Owen who added this code.> > ---- > > include/llvm/Target/TargetLowering.h:267 > switch (getTypeAction(Context, VT)) { > case Legal: > return VT; > case Expand: > > Note that the values of different enum types (LegalizeAction and > LegalizeTypeAction) are compared. This code works well because of the same order > for enums, but it would be better to change this into > > switch (getTypeAction(Context, VT)) { > case TypeLegal: > return VT; > case TypeExpand:Yup, I've made this change.> ---- > > Thanks a lots for any fixes and answers.Thanks for letting us know about these issues. Ciao, Duncan.
On Aug 4, 2011, at 9:03 AM, Duncan Sands wrote:>> >> lib/MC/MCParser/AsmLexer.cpp:149 >> while (CurChar != '\n'&& CurChar != '\n'&& CurChar != EOF) >> >> There are identical sub-expressions to the left and to the right of the '&&' >> operator: CurChar != '\n'&& CurChar != '\n'. The second expression should >> probably be CurChar != '\n'? > > Chris also added this code, hopefully he will comment. >I think you mean that it should be CurChar != '\r', which seems more logical. :) -bw
By the way, PR9952 is the feature request for adding many of these warnings to Clang (identical LHS and RHS). What it doesn't cover is identical function bodies and identical then/else clauses. As for the last two, I'm not sure if the mixed enum case has an existing warning (or existing PR). The 'break' one should be caught by the dead store checker, although it won't recognize the missing break. If you think the checks we're missing are important, can you file bugs for them? Jordy On Aug 4, 2011, at 3:50, Lockal S wrote:> Hi. There are few one-line bugs Andrey Karpov have found with static analisys. > He wrote a big article in russian on http://habrahabr.ru/blogs/compilers/125626/ > for advertising purposes of static analyzer for Visual Studio his company > developed. > > Most of the problems are easy to fix, so I list them in here for trunk version. > Also few problems in clang code were found, I don't list them in here. > > ---- > > lib/Target/X86/X86ISelLowering.cpp:11689 > !DAG.isKnownNeverZero(LHS) && !DAG.isKnownNeverZero(LHS)) > > Note that there are identical subexpressions '!DAG.isKnownNeverZero (LHS)' to > the left and to the right of the '&&' operator. > The second subexpression should probably be !DAG.isKnownNeverZero(RHS)). > > ---- > > lib/Transforms/Scalar/ObjCARC.cpp:1138 > void clearBottomUpPointers() { > PerPtrTopDown.clear(); > } > > void clearTopDownPointers() { > PerPtrTopDown.clear(); > } > > Note that the body of 'clearTopDownPointers' function is fully equivalent to > the body of 'clearBottomUpPointers' function. The advised solution is to change > this code into > > void clearBottomUpPointers() { > PerPtrBottomUp.clear(); > } > > ---- > > lib/Analysis/InstructionSimplify.cpp:1891 > bool NUW = LBO->hasNoUnsignedWrap() && LBO->hasNoUnsignedWrap(); > > There are identical sub-expressions 'LBO->hasNoUnsignedWrap()' to the left and > to the right of the '&&' operator. Looks like the correct code is > > bool NUW = LBO->hasNoUnsignedWrap() && RBO->hasNoUnsignedWrap(); > > ---- > > lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1384 > if (InputByteNo < ByteValues.size()/2) { > if (ByteValues.size()-1-DestByteNo != InputByteNo) > return true; > } else { > if (ByteValues.size()-1-DestByteNo != InputByteNo) > return true; > } > > Note that 'then' and 'else' are the same. It can be a problem or can not. > > ---- > > lib/Target/X86/InstPrinter/X86InstComments.cpp:208 > case X86::VPERMILPSri: > DecodeVPERMILPSMask(4, MI->getOperand(2).getImm(), > ShuffleMask); > Src1Name = getRegName(MI->getOperand(0).getReg()); > case X86::VPERMILPSYri: > DecodeVPERMILPSMask(8, MI->getOperand(2).getImm(), > ShuffleMask); > Src1Name = getRegName(MI->getOperand(0).getReg()); > > The 'Src1Name' variable is assigned values twice successively. So break > instruction should be at line 212. > > ---- > > lib/MC/MCParser/AsmLexer.cpp:149 > while (CurChar != '\n' && CurChar != '\n' && CurChar != EOF) > > There are identical sub-expressions to the left and to the right of the '&&' > operator: CurChar != '\n' && CurChar != '\n'. The second expression should > probably be CurChar != '\n'? > > ---- > lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:505 > result |= (icmp_eq ? (FoldMskICmp_Mask_AllZeroes | > FoldMskICmp_Mask_AllZeroes | > FoldMskICmp_AMask_Mixed | > FoldMskICmp_BMask_Mixed) > : (FoldMskICmp_Mask_NotAllZeroes | > FoldMskICmp_Mask_NotAllZeroes | > FoldMskICmp_AMask_NotMixed | > FoldMskICmp_BMask_NotMixed)); > > There are identical sub-expressions 'FoldMskICmp_Mask_AllZeroes' and > 'FoldMskICmp_Mask_NotAllZeroes' to the left and to the right of the > '|' > operator. Probably not a bug, just some duplicated code. > > ---- > > include/llvm/Target/TargetLowering.h:267 > switch (getTypeAction(Context, VT)) { > case Legal: > return VT; > case Expand: > > Note that the values of different enum types (LegalizeAction and > LegalizeTypeAction) are compared. This code works well because of the same order > for enums, but it would be better to change this into > > switch (getTypeAction(Context, VT)) { > case TypeLegal: > return VT; > case TypeExpand: > > ---- > > Thanks a lots for any fixes and answers. > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev