[clang]Fix warning for signed conversion on LP64

[yaxunl]

Summary

Currently clang emits warning with -Wconversion for the following code
on LP64 system e.g. x86_64-unknown-linux-gnu:

long foo(long x) {
  return 1LL<<x;
}

warning: implicit conversion changes signedness: 'long long' to 'long' [-Wsign-conversion]

return 1ll << x;
~~~~~~ ~~~~^~~~

This does not make sense since all operands are signed.

This patch fixes that to match -m32 and GCC behaviour.

[shafik]

If I look at the clang docs for Wconversion I see it includes -Wshorten-64-to-32 which I believe this is a case of. I think maybe the warning needs a better warning for this case?

[yaxunl]

In D144011#4126553, @shafik wrote:

If I look at the clang docs for Wconversion I see it includes -Wshorten-64-to-32 which I believe this is a case of. I think maybe the warning needs a better warning for this case?

This is done for x86_64 on linux, where long long and long are both 64bit. https://godbolt.org/z/hd3qWW5jj

[MaskRay]

I think it makes sense for -Wsign-conversion to not warn for this LP64 case, like we don't emit a warning for -m32. I do not know whether we need another diagnostic like -Wshorten-64-to-32 for this case, but am inclined to no.

I wonder whether the newly added condition can be merged with the following condition:

if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
    ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
     (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
      LikelySourceRange.Width == TargetRange.Width))) {
  if (S.SourceMgr.isInSystemMacro(CC))

[yaxunl]

In D144011#4126853, @MaskRay wrote:

I think it makes sense for -Wsign-conversion to not warn for this LP64 case, like we don't emit a warning for -m32. I do not know whether we need another diagnostic like -Wshorten-64-to-32 for this case, but am inclined to no.

I wonder whether the newly added condition can be merged with the following condition:

if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
    ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
     (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
      LikelySourceRange.Width == TargetRange.Width))) {
  if (S.SourceMgr.isInSystemMacro(CC))

Yes. Will do.

[yaxunl]

revised by Fanrui's comments

[MaskRay]

Thanks!

clang/lib/Sema/SemaChecking.cpp

14296

Consider adding an example when this condition triggers to help understanding, e.g. long t2 = 1LL << x; on LP64 systems.

[MaskRay]

The description needs to adjusted to say that this confusing warning is for LP64 systems.
It makes sense to match -m32 and GCC. If someone thinks a warning is useful, that can be a separate change.

[yaxunl]

clang/lib/Sema/SemaChecking.cpp

14296

will do when committing

clang/lib/Sema/SemaChecking.cpp

if (TargetRange.Width == LikelySourceRange.Width &&
     // Warn when doing a signed to signed conversion, warn if the positive
     // source value is exactly the width of the target type, which will
     // cause a negative value to be stored.
 
     Expr::EvalResult Result;
     if (E->EvaluateAsInt(Result, S.Context, Expr::SE_AllowSideEffects) &&
         !S.SourceMgr.isInSystemMacro(CC)) {
       llvm::APSInt Value = Result.Val.getInt();
       if (isSameWidthConstantConversion(S, E, T, CC)) {
         std::string PrettySourceValue = toString(Value, 10);
         std::string PrettyTargetValue = PrettyPrintInRange(Value, TargetRange);
 
         S.DiagRuntimeBehavior(
             E->getExprLoc(), E,
             S.PDiag(diag::warn_impcast_integer_precision_constant)
                 << PrettySourceValue << PrettyTargetValue << E->getType() << T
                 << E->getSourceRange() << SourceRange(CC));
         return;
       }
     }
 
     // Fall through for non-constants to give a sign conversion warning.
   }
 
   if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
       ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
        (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
         LikelySourceRange.Width == TargetRange.Width))) {
     if (S.SourceMgr.isInSystemMacro(CC))
       return;
 
+    if (SourceBT && SourceBT->isInteger() && TargetBT &&
+        TargetBT->isInteger() &&
+        Source->isSignedIntegerType() == Target->isSignedIntegerType()) {
+      return;
+    }
+
     unsigned DiagID = diag::warn_impcast_integer_sign;
 
     // Traditionally, gcc has warned about this under -Wsign-compare.
     // We also want to warn about it in -Wconversion.
     // So if -Wconversion is off, use a completely identical diagnostic
     // in the sign-compare group.
     // The conditional-checking code will
     if (ICContext) {

clang/test/Sema/sign-conversion.c

 // RUN: %clang_cc1 -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -verify -Wsign-conversion %s
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -fsyntax-only -verify -Wsign-conversion %s
 
 // PR9345: make a subgroup of -Wconversion for signedness changes
 
 void test(int x) {
   unsigned t0 = x; // expected-warning {{implicit conversion changes signedness}}
   unsigned t1 = (t0 == 5 ? x : 0); // expected-warning {{operand of ? changes signedness}}
+
+  // Clang has special treatment for left shift of literal '1'.
+  // Make sure there is no diagnostics.
+  long t2 = 1LL << x;
 }