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

[simon_tatham]

Summary

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 a value earlier than
MSVC2015, it's useful to warn about 'z' modifiers in printf format
strings we check.

[aaron.ballman]

clang/lib/AST/FormatString.cpp

754

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

[simon_tatham]

clang/lib/AST/FormatString.cpp

754

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.

[aaron.ballman]

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

clang/lib/AST/FormatString.cpp

754

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);

[simon_tatham]

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.

[amccarth]

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.

[aaron.ballman]

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!

[simon_tatham]

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!

[thakis]

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.

[simon_tatham]

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 compatibility version, so that every remaining RUN line specifies an explicit default?

[aaron.ballman]

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.

[simon_tatham]

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 :-)

clang/lib/AST/FormatString.cpp

case LengthModifier::AsShort:
       }
       LLVM_FALLTHROUGH;
     case LengthModifier::AsChar:
     case LengthModifier::AsLongLong:
     case LengthModifier::AsQuad:
     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:
         case ConversionSpecifier::iArg:
         case ConversionSpecifier::oArg:
         case ConversionSpecifier::OArg:
         case ConversionSpecifier::uArg:
         case ConversionSpecifier::UArg:

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

 // 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)));
 int scanf(const char * restrict, ...) ;
 typedef unsigned short wchar_t;
 
 #ifdef NON_ISO_WARNING
 
void z_test(void *p) {
   printf("%Z", p);
   printf("%hZ", p);
   printf("%lZ", p);
   printf("%wZ", p);
   printf("%hhZ", p); // expected-warning{{length modifier 'hh' results in undefined behavior or no effect with 'Z' conversion specifier}}
   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