Multiple one-line bugs in LLVM

Project Infrastructure LLVM Dev List Archives
#1

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.

#2

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.

#3

I think you mean that it should be CurChar != '\r', which seems more logical. :slight_smile:

-bw

#4

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

#5

Fixed in r136908, thanks.

-Chris