[LLVMdev] Multiple one-line bugs in LLVM

[Jordy Rose]

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