[Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

[jacek]

Summary

On 32-bit Windows platform, int and long is mixed all over the place in platform headers. The most notable example is SIZE_T, which is unsigned long, unlike size_t, which is unsigned int. %I, %z and %j formats do the right thing for those types, but clang issues a warning, leaving no good way to print SIZE_T (other than disabling warnings or adding useless casts).

GCC supports only %I for mingw targets, but allows both int and long to be used.

This is causing problems when using clang to build Wine PE files.

[mstorsjo]

Ping, @rnk?

[aaron.ballman]

Personally, I don't consider those to be useless casts. They appropriately ensure that the types match between the arguments and the format specifiers, rather than relying on underlying details of what SIZE_T expands to for a given target. I would expect these to at least use the warn_format_conversion_argument_type_mismatch_confusion diagnostic rather than be silenced entirely.

[amccarth]

In D78508#2005311, @aaron.ballman wrote:

Personally, I don't consider those to be useless casts. They appropriately ensure that the types match between the arguments and the format specifiers, rather than relying on underlying details of what SIZE_T expands to for a given target. I would expect these to at least use the warn_format_conversion_argument_type_mismatch_confusion diagnostic rather than be silenced entirely.

+1. This is useful to anyone trying to write code that compiles for both 32- and 64-bit code.

[amccarth]

I'll re-iterate my opinion that the casts currently required to bypass the warnings are useful (e.g., if you ever want to port the code to 64-bit). There are lots of places where the Windows API essentially required uncomfortable conversions, and we can't paper over all of them. The way to get it right in all those other places is to be very aware of the underlying types. Hiding an uncomfortable detail here and there seems like a disservice.

clang/lib/AST/FormatString.cpp

407

I'm not convinced isOSMSVCRT is the right check. The troubling typedefs are [[ https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types#size-t | SIZE_T and SSIZE_T ]], which come from the Windows API and are unrelated to Microsoft's C run-time library. I think isOSWindows would be more precise.

[aaron.ballman]

In D78508#2038793, @amccarth wrote:

I'll re-iterate my opinion that the casts currently required to bypass the warnings are useful (e.g., if you ever want to port the code to 64-bit). There are lots of places where the Windows API essentially required uncomfortable conversions, and we can't paper over all of them. The way to get it right in all those other places is to be very aware of the underlying types. Hiding an uncomfortable detail here and there seems like a disservice.

+1

[hans]

clang issues a warning, leaving no good way to print SIZE_T (other than disabling warnings or adding useless casts)

I also don't think Clang should change here though. The warning is legit for code that cares about portability, and inserting a cast is probably the best fix for such code. For code that doesn't care, disabling the warning seems like the way to go.

[rsmith]

clang/lib/AST/FormatString.cpp

407

We already care about typedef-names used to spell argument types in some cases (see namedTyoeToLengthModifier. If this is a case of MSVC providing a typedef that people *think* is size_t, that is always the same size and representation as size_t, and that is intended to be fully interchangeable with size_t, but sometimes is a different type, then it might be reasonable to detect whether the type was spelled using that SIZE_T typedef when targeting Windows, and exempt only that case.

I mean, assuming that we want to allow such code at all and not push people to cast explicitly from SIZE_T to size_t. I don't have a strong opinion on that; I think it rather depends on intended idiomatic usage on that platform.

[jacek]

I don't mind dropping the patch, we can mitigate the problem in Wine. My understanding is that we could use "pedantic" logic similar to NSInteger in checkFormatExpr, please let me know if you'd like something like that.

I still think that using casts in such cases is not the right pattern. For an example, if developer knows that SIZE_T is a type that will always be compatible with %z, the code:
printf("%zd", size);
is not wrong. Proposed casts would change all such cases to:
printf("%zd", (size_t)size);
Now imagine that I made a mistake and changed type of size to a type of different size. In the first variant, clang will issue a warning. In the second case, the cast will hide the warning, but I'm probably interested in changing the format instead.

[hans]

Doing special handling for SIZE_T, as opposed to long in general, on Windows as Richard suggests seems reasonable to me.

[aaron.ballman]

In D78508#2041432, @hans wrote:

Doing special handling for SIZE_T, as opposed to long in general, on Windows as Richard suggests seems reasonable to me.

I would be fine with this so long as it doesn't allow %zu to match an unsigned int, etc. SIZE_T matching a %z seems reasonable to me.

clang/lib/AST/FormatString.cpp

case SpecificTy: {
             return T == C.UnsignedShortTy ? Match : NoMatch;
           case BuiltinType::UShort:
             return T == C.ShortTy ? Match : NoMatch;
           case BuiltinType::Int:
             return T == C.UnsignedIntTy ? Match : NoMatch;
           case BuiltinType::UInt:
             return T == C.IntTy ? Match : NoMatch;
           case BuiltinType::Long:
+            // Win32 platform mixes long and int for size_t-like purposes.
+            // ssize_t is int, but platform type SSIZE_T is long.
+            if ((isSizeT() || isPtrdiffT()) &&
+                C.getTargetInfo().getTriple().isOSMSVCRT() &&
+                C.getTargetInfo().getTriple().isArch32Bit())
+              return Match;
             return T == C.UnsignedLongTy ? Match : NoMatch;
           case BuiltinType::ULong:
+            // Win32 platform mixes long and int for size_t-like purposes.
+            // size_t is unsigned int, but platform type SIZE_T is unsigned
+            // long.
+            if ((isSizeT() || isPtrdiffT()) &&
+                C.getTargetInfo().getTriple().isOSMSVCRT() &&
+                C.getTargetInfo().getTriple().isArch32Bit())
+              return Match;
             return T == C.LongTy ? Match : NoMatch;
           case BuiltinType::LongLong:
             return T == C.UnsignedLongLongTy ? Match : NoMatch;
           case BuiltinType::ULongLong:
             return T == C.LongLongTy ? Match : NoMatch;
         }
       return NoMatch;
     }

clang/lib/AST/PrintfFormatString.cpp

switch (LM.getKind()) {
       case LengthModifier::AsInt64:
         return ArgType(Ctx.LongLongTy, "__int64");
       case LengthModifier::AsIntMax:
         return ArgType(Ctx.getIntMaxType(), "intmax_t");
       case LengthModifier::AsSizeT:
         return ArgType::makeSizeT(ArgType(Ctx.getSignedSizeType(), "ssize_t"));
       case LengthModifier::AsInt3264:
         return Ctx.getTargetInfo().getTriple().isArch64Bit()
-                   ? ArgType(Ctx.LongLongTy, "__int64")
-                   : ArgType(Ctx.IntTy, "__int32");
+                   ? ArgType::makeSizeT(ArgType(Ctx.LongLongTy, "__int64"))
+                   : ArgType::makeSizeT(ArgType(Ctx.IntTy, "__int32"));
       case LengthModifier::AsPtrDiff:
         return ArgType::makePtrdiffT(
             ArgType(Ctx.getPointerDiffType(), "ptrdiff_t"));
       case LengthModifier::AsAllocate:
       case LengthModifier::AsMAllocate:
       case LengthModifier::AsWide:
         return ArgType::Invalid();
     }
switch (LM.getKind()) {
       case LengthModifier::AsInt64:
         return ArgType(Ctx.UnsignedLongLongTy, "unsigned __int64");
       case LengthModifier::AsIntMax:
         return ArgType(Ctx.getUIntMaxType(), "uintmax_t");
       case LengthModifier::AsSizeT:
         return ArgType::makeSizeT(ArgType(Ctx.getSizeType(), "size_t"));
       case LengthModifier::AsInt3264:
         return Ctx.getTargetInfo().getTriple().isArch64Bit()
-                   ? ArgType(Ctx.UnsignedLongLongTy, "unsigned __int64")
-                   : ArgType(Ctx.UnsignedIntTy, "unsigned __int32");
+                   ? ArgType::makeSizeT(
+                         ArgType(Ctx.UnsignedLongLongTy, "unsigned __int64"))
+                   : ArgType::makeSizeT(
+                         ArgType(Ctx.UnsignedIntTy, "unsigned __int32"));
       case LengthModifier::AsPtrDiff:
         return ArgType::makePtrdiffT(
             ArgType(Ctx.getUnsignedPointerDiffType(), "unsigned ptrdiff_t"));
       case LengthModifier::AsAllocate:
       case LengthModifier::AsMAllocate:
       case LengthModifier::AsWide:
         return ArgType::Invalid();
     }

clang/test/Sema/format-strings-ms.c

void non_iso_warning_test(__int32 i32, __int64 i64, wchar_t c, void *p) {
   printf("%Z", p); // expected-warning{{'Z' conversion specifier is not supported by ISO C}}
 }
 
 #else
 
 void signed_test() {
   short val = 30;
   printf("val = %I64d\n", val); // expected-warning{{format specifies type '__int64' (aka 'long long') but the argument has type 'short'}}
+  printf("val = %Id\n", val);
   long long bigval = 30;
   printf("val = %I32d\n", bigval); // expected-warning{{format specifies type '__int32' (aka 'int') but the argument has type 'long long'}}
   printf("val = %Id\n", bigval); // expected-warning{{format specifies type '__int32' (aka 'int') but the argument has type 'long long'}}
+  int ival = 30;
+  printf("%Id", ival);
+  printf("%zd", ival);
+  printf("%td", ival);
+  long lval = 30;
+  printf("%Id", lval);
+  printf("%zd", lval);
+  printf("%td", lval);
 }
 
 void unsigned_test() {
   unsigned short val = 30;
   printf("val = %I64u\n", val); // expected-warning{{format specifies type 'unsigned __int64' (aka 'unsigned long long') but the argument has type 'unsigned short'}}
+  printf("val = %Iu\n", val);
   unsigned long long bigval = 30;
   printf("val = %I32u\n", bigval); // expected-warning{{format specifies type 'unsigned __int32' (aka 'unsigned int') but the argument has type 'unsigned long long'}}
   printf("val = %Iu\n", bigval); // expected-warning{{format specifies type 'unsigned __int32' (aka 'unsigned int') but the argument has type 'unsigned long long'}}
+  unsigned int ival = 30;
+  printf("%Iu", ival);
+  printf("%zu", ival);
+  printf("%tu", ival);
+  unsigned long lval = 30;
+  printf("%Iu", lval);
+  printf("%zu", lval);
+  printf("%tu", lval);
 }
 
 void w_test(wchar_t c, wchar_t *s) {
   printf("%wc", c);
   printf("%wC", c);
   printf("%C", c);
   printf("%ws", s);
   printf("%wS", s);