thr3ads.net - llvm dev - [LLVMdev] Multiple one-line bugs in LLVM [Aug 2011]

If this information is useful, please help other people find it:
Share via:

Lockal S

2011-Aug-04 10:50 UTC

[LLVMdev] Multiple one-line bugs in LLVM

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.

Duncan Sands

2011-Aug-04 16:03 UTC

head link

[LLVMdev] Multiple one-line bugs in LLVM

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.

Bill Wendling

2011-Aug-04 17:55 UTC

head link

[LLVMdev] Multiple one-line bugs in LLVM

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

Jordy Rose

2011-Aug-04 18:51 UTC

head link

[LLVMdev] Multiple one-line bugs in LLVM

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

Maybe Matching Threads

Search for more possibly parallel threads

llvm dev - Aug 2011 - [LLVMdev] Multiple one-line bugs in LLVM

[LLVMdev] Multiple one-line bugs in LLVM

[LLVMdev] Multiple one-line bugs in LLVM

[LLVMdev] Multiple one-line bugs in LLVM

[LLVMdev] Multiple one-line bugs in LLVM

Maybe Matching Threads