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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jacek Caban via Phabricator via cfe-commits]

jacek added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/AST/FormatString.cpp:407
+if ((isSizeT() || isPtrdiffT()) &&
+C.getTargetInfo().getTriple().isOSMSVCRT() &&
+C.getTargetInfo().getTriple().isArch32Bit())

amccarth wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.




Comment at: clang/lib/AST/FormatString.cpp:407
+if ((isSizeT() || isPtrdiffT()) &&
+C.getTargetInfo().getTriple().isOSMSVCRT() &&
+C.getTargetInfo().getTriple().isArch32Bit())

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Ping, @rnk?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78508/new/

https://reviews.llvm.org/D78508



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jacek Caban via Phabricator via cfe-commits]

jacek created this revision.
jacek added a reviewer: clang.
Herald added subscribers: cfe-commits, mstorsjo.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78508

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/AST/PrintfFormatString.cpp
  clang/test/Sema/format-strings-ms.c


Index: clang/test/Sema/format-strings-ms.c
===
--- clang/test/Sema/format-strings-ms.c
+++ clang/test/Sema/format-strings-ms.c
@@ -21,17 +21,35 @@
 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) {
Index: clang/lib/AST/PrintfFormatString.cpp
===
--- clang/lib/AST/PrintfFormatString.cpp
+++ clang/lib/AST/PrintfFormatString.cpp
@@ -527,8 +527,8 @@
 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"));
@@ -562,8 +562,10 @@
 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"));
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -401,8 +401,21 @@
   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