[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham added a comment.

I now realise that my previous comment was nonsense: looking at @thakis's link 
more carefully, there are actually 5 failing tests that are nothing to do with 
the one I modified, and a lot of them don't even have any obvious 
`-fms-extension` option in the cc1 command line at all. (Perhaps clang will 
even turn //that// on automatically when it autodetects MSVC in the 
environment?) So just fixing my own test differently wouldn't help.

So I don't know what to propose. I'd like to see this error check reland, but I 
see that it will take some work before the test suite is sufficiently 
environment-independent to not make it cause this kind of problem all over the 
place.

Right off the top of my head, one thought that strikes me is: why is 
auto-detection of MS compat mode from the environment happening in //cc1?// 
(Which I assume it must be, if those cc1 tests are failing.) If the point of it 
is to make clang-cl a drop-in replacement for the `cl.exe` installed on the 
same system, would it not make more sense to do the auto-detection in the 
clang-cl driver, and pass explicit options to cc1 containing whatever was 
detected (if it wasn't overridden by the driver command line)? Then cc1 would 
behave the same everywhere given the same command line, and all the cc1 runs in 
the test suite would be automatically stable, and the only place you'd have to 
take care would be in tests of the clang-cl driver itself. But that's the kind 
of structural change that would surely cause a completely different collection 
of failures in places nobody would have thought of :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D73457#1844475 , @thakis wrote:

> Since we auto-detect -fmsc-version if it's not explicitly installed and since 
> this warning is on by default, it makes the test suite depend on the 
> environment a good bit. Given how old 2015 is by now, I'm not sure this 
> complexity and subtlety is worth the benefit?


I think it's worth the benefit -- the point to the check is to help catch 
buffer overflows, and getting the length calculation correct for that is 
important.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham added a comment.

I wanted this to go in because I'm actually using it – pre-2015 C libraries are 
useful to link against if you need an application to run on very old versions 
of Windows, and that means you need the compiler to warn you if you do 
something those libraries don't support.

I see that auto-detecting the default MS compatibility version is not very 
compatible with keeping the test suite independent of the environment, but 
surely that's a general problem with the autodetection policy, nothing to do 
with this particular patch?

Would you be happy with just removing the RUN line in this test that doesn't 
specify a default compatibility version, so that every remaining RUN line 
specifies an explicit default?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks tests on Windows: http://45.33.8.238/win/6812/step_7.txt

Since we auto-detect -fmsc-version if it's not explicitly installed and since 
this warning is on by default, it makes the test suite depend on the 
environment a good bit. Given how old 2015 is by now, I'm not sure this 
complexity and subtlety is worth the benefit?

In any case, I'll revert this for now to heal the bots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham added a comment.

In D73457#1842605 , @amccarth wrote:

> clang will still (by default) use the MS runtime libraries for Windows 
> builds, in which case it's important for the compatibility version to match 
> the one for the libraries that are going to be used.  I think we can/should 
> reconsider this when clang-cl starts using different run-time libraries for 
> Windows builds.


Ah, yes, I was half wondering if there might need to be separate options for 
"behave like version x of the MSVC compiler" and "expect version y of the MSVC 
library [or some other library, etc]". Sounds as if that's a potential future 
thing but not now.

In D73457#1842668 , @aaron.ballman 
wrote:

> LGTM!


Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Simon Tatham via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfe0d1b6a8ac5: [Clang] Warn about z printf 
modifier in old MSVC. (authored by simon_tatham).

Changed prior to commit:
  https://reviews.llvm.org/D73457?vs=240626=240796#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457

Files:
  clang/lib/AST/FormatString.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
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -fms-compatibility-version=18 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -fms-compatibility-version=19 -DSIZE_T_OK %s
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
@@ -85,4 +87,11 @@
   scanf("%Z", p); // expected-warning{{invalid conversion specifier 'Z'}}
 }
 
+void size_t_test(size_t s) {
+  printf("%zu", s);
+#ifndef SIZE_T_OK
+  // expected-warning@-2 {{length modifier 'z' results in undefined behavior 
or no effect with 'u' conversion specifier}}
+#endif
+}
+
 #endif
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -748,6 +748,15 @@
 case LengthModifier::AsIntMax:
 case LengthModifier::AsSizeT:
 case LengthModifier::AsPtrDiff:
+  if (LM.getKind() == LengthModifier::AsSizeT &&
+  Target.getTriple().isOSMSVCRT() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length
+// modifier for size_t. So if the MS compatibility version is less than
+// that, reject.
+return false;
+  }
+
   switch (CS.getKind()) {
 case ConversionSpecifier::dArg:
 case ConversionSpecifier::DArg:


Index: clang/test/Sema/format-strings-ms.c
===
--- clang/test/Sema/format-strings-ms.c
+++ clang/test/Sema/format-strings-ms.c
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -fms-compatibility-version=18 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -fms-compatibility-version=19 -DSIZE_T_OK %s
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
@@ -85,4 +87,11 @@
   scanf("%Z", p); // expected-warning{{invalid conversion specifier 'Z'}}
 }
 
+void size_t_test(size_t s) {
+  printf("%zu", s);
+#ifndef SIZE_T_OK
+  // expected-warning@-2 {{length modifier 'z' results in undefined behavior or no effect with 'u' conversion specifier}}
+#endif
+}
+
 #endif
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -748,6 +748,15 @@
 case LengthModifier::AsIntMax:
 case LengthModifier::AsSizeT:
 case LengthModifier::AsPtrDiff:
+  if (LM.getKind() == LengthModifier::AsSizeT &&
+  Target.getTriple().isOSMSVCRT() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length
+// modifier for size_t. So if the MS compatibility version is less than
+// that, reject.
+return false;
+  }
+
   switch (CS.getKind()) {
 case ConversionSpecifier::dArg:
 case ConversionSpecifier::DArg:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!

In D73457#1842605 , @amccarth wrote:

> In D73457#1842493 , @simon_tatham 
> wrote:
>
> > Removed the special case for `MSCompatibilityVersion == 0`. If the default 
> > compatibility setting needs to be changed, that's a separate piece of work 
> > and should be done by someone who understands more than I do about all 
> > those failing tests :-) The new version of this patch just uses the 
> > existing default.
>
>
> +1.  The issue of how the default version gets set is a separate issue.  I 
> think it's best to keep this patch decoupled from that.


Definitely agreed! I just didn't want to do duplicate work if there was a bug 
elsewhere that would impact how we proceed here.

>> With the typical command line I use with the `clang-cl` driver, a specific 
>> version has been set anyway by the time cc1 is running. So probably I 
>> shouldn't have been worrying in the first place about what happens if there 
>> isn't one set.
> 
> The default you found for MSVC15 is the last result default.  (Though I would 
> have expected that to be MSVC17 by now.)  I believe the driver will actually 
> try to figure out the most recent version of MSVC installed on the machine 
> (assuming it's Windows), and use that.  Only if it can't find one, will it 
> use that default.
> 
> As I recall, this is because clang will still (by default) use the MS runtime 
> libraries for Windows builds, in which case it's important for the 
> compatibility version to match the one for the libraries that are going to be 
> used.  I think we can/should reconsider this when clang-cl starts using 
> different run-time libraries for Windows builds.

Ah, thank you for that context!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In D73457#1842493 , @simon_tatham 
wrote:

> Removed the special case for `MSCompatibilityVersion == 0`. If the default 
> compatibility setting needs to be changed, that's a separate piece of work 
> and should be done by someone who understands more than I do about all those 
> failing tests :-) The new version of this patch just uses the existing 
> default.


+1.  The issue of how the default version gets set is a separate issue.  I 
think it's best to keep this patch decoupled from that.

> With the typical command line I use with the `clang-cl` driver, a specific 
> version has been set anyway by the time cc1 is running. So probably I 
> shouldn't have been worrying in the first place about what happens if there 
> isn't one set.

The default you found for MSVC15 is the last result default.  (Though I would 
have expected that to be MSVC17 by now.)  I believe the driver will actually 
try to figure out the most recent version of MSVC installed on the machine 
(assuming it's Windows), and use that.  Only if it can't find one, will it use 
that default.

As I recall, this is because clang will still (by default) use the MS runtime 
libraries for Windows builds, in which case it's important for the 
compatibility version to match the one for the libraries that are going to be 
used.  I think we can/should reconsider this when clang-cl starts using 
different run-time libraries for Windows builds.

I'm not officially a reviewer on this patch, but LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham updated this revision to Diff 240626.
simon_tatham added a comment.

Removed the special case for `MSCompatibilityVersion == 0`. If the default 
compatibility setting needs to be changed, that's a separate piece of work and 
should be done by someone who understands more than I do about all those 
failing tests :-) The new version of this patch just uses the existing default.

With the typical command line I use with the `clang-cl` driver, a specific 
version has been set anyway by the time cc1 is running. So probably I shouldn't 
have been worrying in the first place about what happens if there isn't one set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457

Files:
  clang/lib/AST/FormatString.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
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -fms-compatibility-version=18 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -fms-compatibility-version=19 -DSIZE_T_OK %s
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
@@ -85,4 +87,11 @@
   scanf("%Z", p); // expected-warning{{invalid conversion specifier 'Z'}}
 }
 
+void size_t_test(size_t s) {
+  printf("%zu", s);
+#ifndef SIZE_T_OK
+  // expected-warning@-2 {{length modifier 'z' results in undefined behavior 
or no effect with 'u' conversion specifier}}
+#endif
+}
+
 #endif
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -748,6 +748,15 @@
 case LengthModifier::AsIntMax:
 case LengthModifier::AsSizeT:
 case LengthModifier::AsPtrDiff:
+  if (LM.getKind() == LengthModifier::AsSizeT &&
+  Target.getTriple().isOSMSVCRT() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length
+// modifier for size_t. So if the MS compatibility version is specified
+// and less than that, reject.
+return false;
+  }
+
   switch (CS.getKind()) {
 case ConversionSpecifier::dArg:
 case ConversionSpecifier::DArg:


Index: clang/test/Sema/format-strings-ms.c
===
--- clang/test/Sema/format-strings-ms.c
+++ clang/test/Sema/format-strings-ms.c
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -fms-compatibility-version=18 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -fms-compatibility-version=19 -DSIZE_T_OK %s
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
@@ -85,4 +87,11 @@
   scanf("%Z", p); // expected-warning{{invalid conversion specifier 'Z'}}
 }
 
+void size_t_test(size_t s) {
+  printf("%zu", s);
+#ifndef SIZE_T_OK
+  // expected-warning@-2 {{length modifier 'z' results in undefined behavior or no effect with 'u' conversion specifier}}
+#endif
+}
+
 #endif
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -748,6 +748,15 @@
 case LengthModifier::AsIntMax:
 case LengthModifier::AsSizeT:
 case LengthModifier::AsPtrDiff:
+  if (LM.getKind() == LengthModifier::AsSizeT &&
+  Target.getTriple().isOSMSVCRT() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length
+// modifier for size_t. So if the MS compatibility version is specified
+// and less than that, reject.
+return false;
+  }
+
   switch (CS.getKind()) {
 case ConversionSpecifier::dArg:
 case ConversionSpecifier::DArg:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: rnk, majnemer, zturner.
aaron.ballman added a comment.

Adding some more Windows reviewers to see if there are more opinions on how to 
handle this.




Comment at: clang/lib/AST/FormatString.cpp:754
+  LO.isMSCompatibilityVersionSpecified() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length

simon_tatham wrote:
> aaron.ballman wrote:
> > I'd rather not see `isMSCompatibilityVersionSpecified()` be introduced, but 
> > instead make `isCompatibleWithMSVC()` do the right thing when it's not 
> > specified.
> I tried making `isCompatibleWithMSVC` return true if `MSCompatibilityVersion 
> == 0` on the basis that it should be considered 'newest' rather than 
> 'oldest'. That caused a lot of knock-on test failures which I don't really 
> understand all of:
> ```Clang :: CodeGenCXX/dllexport-no-dllexport-inlines.cpp
> Clang :: CodeGenCXX/exceptions-cxx-new.cpp
> Clang :: CodeGenCXX/mangle-ms-exception-spec.cpp
> Clang :: CodeGenCXX/msabi-blocks.cpp
> Clang :: Rewriter/properties.m
> Clang :: SemaCXX/microsoft-cxx0x.cpp
> Clang :: SemaCXX/pragma-init_seg.cpp
> Clang :: SemaCXX/warn-static-outside-class-definition.cpp
> ```
> `mangle-ms-exception-spec.cpp` in particular looks as if it's expecting some 
> kind of completely different mangling without any compatibility version.
> 
> Perhaps I should go with the existing behavior and make 'unspecified' keep 
> defaulting to oldest rather than newest? I think in general the driver will 
> not leave it unspecified, so perhaps it won't make much difference outside 
> the test suite anyway. Hmmm.
I would have expected passing `-fms-compatibility` without passing 
`-fms-compatibility-version` would default to compatibility with the newest 
version of MSVC instead of the oldest version (unless there is more specific 
information from the environment). However, then I found D20136 and it seems 
that we do default to an old version on purpose:
```
  // FIXME: Consider bumping this to 19 (MSVC2015) soon.
  return VersionTuple(18);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham marked an inline comment as done.
simon_tatham added inline comments.



Comment at: clang/lib/AST/FormatString.cpp:754
+  LO.isMSCompatibilityVersionSpecified() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length

aaron.ballman wrote:
> I'd rather not see `isMSCompatibilityVersionSpecified()` be introduced, but 
> instead make `isCompatibleWithMSVC()` do the right thing when it's not 
> specified.
I tried making `isCompatibleWithMSVC` return true if `MSCompatibilityVersion == 
0` on the basis that it should be considered 'newest' rather than 'oldest'. 
That caused a lot of knock-on test failures which I don't really understand all 
of:
```Clang :: CodeGenCXX/dllexport-no-dllexport-inlines.cpp
Clang :: CodeGenCXX/exceptions-cxx-new.cpp
Clang :: CodeGenCXX/mangle-ms-exception-spec.cpp
Clang :: CodeGenCXX/msabi-blocks.cpp
Clang :: Rewriter/properties.m
Clang :: SemaCXX/microsoft-cxx0x.cpp
Clang :: SemaCXX/pragma-init_seg.cpp
Clang :: SemaCXX/warn-static-outside-class-definition.cpp
```
`mangle-ms-exception-spec.cpp` in particular looks as if it's expecting some 
kind of completely different mangling without any compatibility version.

Perhaps I should go with the existing behavior and make 'unspecified' keep 
defaulting to oldest rather than newest? I think in general the driver will not 
leave it unspecified, so perhaps it won't make much difference outside the test 
suite anyway. Hmmm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/FormatString.cpp:754
+  LO.isMSCompatibilityVersionSpecified() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length

I'd rather not see `isMSCompatibilityVersionSpecified()` be introduced, but 
instead make `isCompatibleWithMSVC()` do the right thing when it's not 
specified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73457



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

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham created this revision.
simon_tatham added reviewers: aaron.ballman, lebedev.ri.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The 'z' length modifier, signalling that an integer format specifier
takes a `size_t` sized integer, is only supported by the C library of
MSVC 2015 and later. Earlier versions don't recognize the 'z' at all,
and respond to `printf("%zu", x)` by just printing "zu".

So, if the MS compatibility version is set to something specific which
is earlier than 2015, it's useful to warn about 'z' modifiers in
printf format strings we check.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73457

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/FormatString.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
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -fms-compatibility-version=19 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -fms-compatibility-version=18 -DNO_z_MODIFIER %s
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility 
-triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
@@ -85,4 +87,11 @@
   scanf("%Z", p); // expected-warning{{invalid conversion specifier 'Z'}}
 }
 
+void size_t_test(size_t s) {
+  printf("%zu", s);
+#ifdef NO_z_MODIFIER
+  // expected-warning@-2 {{length modifier 'z' results in undefined behavior 
or no effect with 'u' conversion specifier}}
+#endif
+}
+
 #endif
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -748,6 +748,16 @@
 case LengthModifier::AsIntMax:
 case LengthModifier::AsSizeT:
 case LengthModifier::AsPtrDiff:
+  if (LM.getKind() == LengthModifier::AsSizeT &&
+  Target.getTriple().isOSMSVCRT() &&
+  LO.isMSCompatibilityVersionSpecified() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length
+// modifier for size_t. So if the MS compatibility version is specified
+// and less than that, reject.
+return false;
+  }
+
   switch (CS.getKind()) {
 case ConversionSpecifier::dArg:
 case ConversionSpecifier::DArg:
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -328,6 +328,10 @@
!ObjCSubscriptingLegacyRuntime;
   }
 
+  bool isMSCompatibilityVersionSpecified() const {
+return MSCompatibilityVersion != 0;
+  }
+
   bool isCompatibleWithMSVC(MSVCMajorVersion MajorVersion) const {
 return MSCompatibilityVersion >= MajorVersion * 10U;
   }


Index: clang/test/Sema/format-strings-ms.c
===
--- clang/test/Sema/format-strings-ms.c
+++ clang/test/Sema/format-strings-ms.c
@@ -1,4 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -fms-compatibility-version=19 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -fms-compatibility-version=18 -DNO_z_MODIFIER %s
 // RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -triple=i386-pc-win32 -Wformat-non-iso -DNON_ISO_WARNING %s
 
 int printf(const char *format, ...) __attribute__((format(printf, 1, 2)));
@@ -85,4 +87,11 @@
   scanf("%Z", p); // expected-warning{{invalid conversion specifier 'Z'}}
 }
 
+void size_t_test(size_t s) {
+  printf("%zu", s);
+#ifdef NO_z_MODIFIER
+  // expected-warning@-2 {{length modifier 'z' results in undefined behavior or no effect with 'u' conversion specifier}}
+#endif
+}
+
 #endif
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -748,6 +748,16 @@
 case LengthModifier::AsIntMax:
 case LengthModifier::AsSizeT:
 case LengthModifier::AsPtrDiff:
+  if (LM.getKind() == LengthModifier::AsSizeT &&
+  Target.getTriple().isOSMSVCRT() &&
+  LO.isMSCompatibilityVersionSpecified() &&
+  !LO.isCompatibleWithMSVC(LangOptions::MSVC2015)) {
+// The standard libraries before MSVC2015 didn't support the 'z' length
+// modifier for size_t. So if the MS compatibility version is specified
+// and less