-Wsign-compare results on LLVM/clang

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

-Wsign-compare results on LLVM/clang

John McCall
I've made an audit of the -Wsign-compare warnings that clang is emitting on LLVM/Clang.  And here it is!

lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5382:37: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

  This is legitimate; AFAICT there's an actual bug here where the code uses -1 as a sentinel value and then that gets converted to unsigned and potentially used in arithmetic.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:189:55: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

      unsigned NumParts;
      ...
      unsigned RoundParts = NumParts & (NumParts - 1) ?
        1 << Log2_32(NumParts) : NumParts;

  False positive.  clang can avoid this warning by not diagnosing if the left operand is a bitshift of a literal.

include/llvm/CodeGen/MachORelocation.h:44:54: warning: operands of ? are integers of different signs: 'int32_t const' (aka 'int const') and 'uint32_t const' (aka 'unsigned int const') [-Wsign-compare]

    uint32_t getAddress() const { return r_scattered ? r_value : r_address; }

  If this is noise it's good noise.  Can r_value ever be negative?  If not, why is it signed?

lib/Target/X86/X86FastISel.cpp:306:45: warning: operands of ? are integers of different signs: 'int64_t' (aka 'long long') and 'uint64_t' (aka 'unsigned long long') [-Wsign-compare]

      addFullAddress(BuildMI(MBB, DL, TII.get(Opc)), AM)
                             .addImm(Signed ? CI->getSExtValue() :
                                              CI->getZExtValue());

  False positive because addImm just cares about its operand as a bit-pattern.  Damned if I know how to avoid diagnosing this.

lib/Target/MSP430/AsmPrinter/MSP430AsmPrinter.cpp:127:49: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

      O << ',' << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);

  False positive, same resolution as earlier.

lib/Target/X86/X86ISelLowering.cpp:3245:22: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]

    int Val = isLeft ? (i - NumZeros) : i;

  False positive.  Could easily be worked around by using unsigned instead of int.

lib/Target/X86/X86ISelLowering.cpp:3246:39: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

    int Idx = SVOp->getMaskElt(isLeft ? i : (i - NumZeros));

  Same thing.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp:793:5: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]

  int Offset = isAM2
    ? ARM_AM::getAM2Offset(OffField)
    : (isAM3 ? ARM_AM::getAM3Offset(OffField)
             : ARM_AM::getAM5Offset(OffField) * 4);

  Might be a false positive; probably worth complaining about, though.

lib/Target/SystemZ/SystemZRegisterInfo.cpp:203:44: warning: operands of ? are integers of different signs: 'int64_t' (aka 'long long') and 'uint64_t' (aka 'unsigned long long') [-Wsign-compare]

    uint64_t ThisVal = (Offset > Chunk) ? Chunk : Offset;
    ...                                                  
      .addReg(SystemZ::R15D).addImm((isSub ? -(int64_t)ThisVal : ThisVal));

  False positive, addImm just wants a bit-pattern.  No idea how to avoid warning about this while still being useful.

lib/Target/SystemZ/AsmPrinter/SystemZAsmPrinter.cpp:340:49: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

      O << ',' << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);

  False positive, will be fixed.

lib/Target/ARM/AsmPrinter/ARMAsmPrinter.cpp:1254:55: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

            O << ',' << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);

  False positive, will be fixed.

lib/Target/ARM/AsmPrinter/ARMAsmPrinter.cpp:1261:53: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

          O << "," << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);

  False positive, will be fixed.

lib/Target/X86/AsmPrinter/X86AsmPrinter.cpp:734:55: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
lib/Target/X86/AsmPrinter/X86AsmPrinter.cpp:743:53: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

  I need to look into these.

lib/CodeGen/VirtRegRewriter.cpp:870:11: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]

        int SSorRMId = DoReMat
          ? VRM.getReMatId(NewOp.VirtReg) : NewOp.StackSlotOrReMat;

  False positive, I think.

tools/clang/lib/Basic/SourceManager.cpp:46:17: warning: operands of ? are integers of different signs: 'size_t' (aka 'unsigned long') and 'off_t' (aka 'long long') [-Wsign-compare]

  return Buffer ? Buffer->getBufferSize() : Entry->getSize();

  False positive: getBufferSize() is always positive.  It's possible this could be suppressed by noting that the signed operand is of higher rank than the unsigned operand, even though they're the same size.  I need to look into whether that will bury too many actual problems.

tools/clang/lib/Driver/ArgList.cpp:83:18: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
tools/clang/lib/Driver/ArgList.cpp:84:18: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
tools/clang/lib/Driver/ArgList.cpp:85:18: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]

  int A0Idx = A0 ? A0->getIndex() : -1;
  int A1Idx = A1 ? A1->getIndex() : -1;
  int A2Idx = A2 ? A2->getIndex() : -1;

  These are false positives (assuming fewer than 2^31 arguments) because of the implicit conversion back to signed int.  I don't really have a problem with warning about them, though.

tools/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp:456:25: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]

  The only diagnostic arising from comparisons rather than the conditional operator.  This is a false positive which I can fix pretty easily.

John.
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: -Wsign-compare results on LLVM/clang

Daniel Dunbar
Meta question, what rule is gcc following that we don't get these
warnings with it?

 - Daniel

On Thu, Dec 24, 2009 at 5:11 PM, John McCall <[hidden email]> wrote:

> I've made an audit of the -Wsign-compare warnings that clang is emitting on LLVM/Clang.  And here it is!
>
> lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5382:37: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>  This is legitimate; AFAICT there's an actual bug here where the code uses -1 as a sentinel value and then that gets converted to unsigned and potentially used in arithmetic.
>
> lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:189:55: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>      unsigned NumParts;
>      ...
>      unsigned RoundParts = NumParts & (NumParts - 1) ?
>        1 << Log2_32(NumParts) : NumParts;
>
>  False positive.  clang can avoid this warning by not diagnosing if the left operand is a bitshift of a literal.
>
> include/llvm/CodeGen/MachORelocation.h:44:54: warning: operands of ? are integers of different signs: 'int32_t const' (aka 'int const') and 'uint32_t const' (aka 'unsigned int const') [-Wsign-compare]
>
>    uint32_t getAddress() const { return r_scattered ? r_value : r_address; }
>
>  If this is noise it's good noise.  Can r_value ever be negative?  If not, why is it signed?
>
> lib/Target/X86/X86FastISel.cpp:306:45: warning: operands of ? are integers of different signs: 'int64_t' (aka 'long long') and 'uint64_t' (aka 'unsigned long long') [-Wsign-compare]
>
>      addFullAddress(BuildMI(MBB, DL, TII.get(Opc)), AM)
>                             .addImm(Signed ? CI->getSExtValue() :
>                                              CI->getZExtValue());
>
>  False positive because addImm just cares about its operand as a bit-pattern.  Damned if I know how to avoid diagnosing this.
>
> lib/Target/MSP430/AsmPrinter/MSP430AsmPrinter.cpp:127:49: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>      O << ',' << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);
>
>  False positive, same resolution as earlier.
>
> lib/Target/X86/X86ISelLowering.cpp:3245:22: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
>
>    int Val = isLeft ? (i - NumZeros) : i;
>
>  False positive.  Could easily be worked around by using unsigned instead of int.
>
> lib/Target/X86/X86ISelLowering.cpp:3246:39: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>    int Idx = SVOp->getMaskElt(isLeft ? i : (i - NumZeros));
>
>  Same thing.
>
> lib/Target/ARM/ARMLoadStoreOptimizer.cpp:793:5: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
>
>  int Offset = isAM2
>    ? ARM_AM::getAM2Offset(OffField)
>    : (isAM3 ? ARM_AM::getAM3Offset(OffField)
>             : ARM_AM::getAM5Offset(OffField) * 4);
>
>  Might be a false positive; probably worth complaining about, though.
>
> lib/Target/SystemZ/SystemZRegisterInfo.cpp:203:44: warning: operands of ? are integers of different signs: 'int64_t' (aka 'long long') and 'uint64_t' (aka 'unsigned long long') [-Wsign-compare]
>
>    uint64_t ThisVal = (Offset > Chunk) ? Chunk : Offset;
>    ...
>      .addReg(SystemZ::R15D).addImm((isSub ? -(int64_t)ThisVal : ThisVal));
>
>  False positive, addImm just wants a bit-pattern.  No idea how to avoid warning about this while still being useful.
>
> lib/Target/SystemZ/AsmPrinter/SystemZAsmPrinter.cpp:340:49: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>      O << ',' << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);
>
>  False positive, will be fixed.
>
> lib/Target/ARM/AsmPrinter/ARMAsmPrinter.cpp:1254:55: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>            O << ',' << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);
>
>  False positive, will be fixed.
>
> lib/Target/ARM/AsmPrinter/ARMAsmPrinter.cpp:1261:53: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>          O << "," << (MAI->getAlignmentIsInBytes() ? (1 << Align) : Align);
>
>  False positive, will be fixed.
>
> lib/Target/X86/AsmPrinter/X86AsmPrinter.cpp:734:55: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
> lib/Target/X86/AsmPrinter/X86AsmPrinter.cpp:743:53: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>  I need to look into these.
>
> lib/CodeGen/VirtRegRewriter.cpp:870:11: warning: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>
>        int SSorRMId = DoReMat
>          ? VRM.getReMatId(NewOp.VirtReg) : NewOp.StackSlotOrReMat;
>
>  False positive, I think.
>
> tools/clang/lib/Basic/SourceManager.cpp:46:17: warning: operands of ? are integers of different signs: 'size_t' (aka 'unsigned long') and 'off_t' (aka 'long long') [-Wsign-compare]
>
>  return Buffer ? Buffer->getBufferSize() : Entry->getSize();
>
>  False positive: getBufferSize() is always positive.  It's possible this could be suppressed by noting that the signed operand is of higher rank than the unsigned operand, even though they're the same size.  I need to look into whether that will bury too many actual problems.
>
> tools/clang/lib/Driver/ArgList.cpp:83:18: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
> tools/clang/lib/Driver/ArgList.cpp:84:18: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
> tools/clang/lib/Driver/ArgList.cpp:85:18: warning: operands of ? are integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
>
>  int A0Idx = A0 ? A0->getIndex() : -1;
>  int A1Idx = A1 ? A1->getIndex() : -1;
>  int A2Idx = A2 ? A2->getIndex() : -1;
>
>  These are false positives (assuming fewer than 2^31 arguments) because of the implicit conversion back to signed int.  I don't really have a problem with warning about them, though.
>
> tools/clang/lib/Analysis/CheckSecuritySyntaxOnly.cpp:456:25: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
>
>  The only diagnostic arising from comparisons rather than the conditional operator.  This is a false positive which I can fix pretty easily.
>
> John.
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev