[Sema] -Wformat-pedantic only for NSInteger/NSUInteger %tu/%td on Darwin

[arphaman]

Summary

The '%tu'/'%td' as formatting specifiers have been used to print out the NSInteger/NSUInteger values for a long time. Typically their ABI matches (i.e. ptrdiff_t = NSInteger), but that's not the case on watchOS (ptrdiff_t = long, NSInteger = int). These specifiers trigger -Wformat warnings only for watchOS builds, which is really inconvenient for cross-platform code.

This patch avoids this -Wformat warning for '%tu'/'%td' and NS[U]Integer only, and instead uses the new -Wformat-pedantic warning that JF introduced in https://reviews.llvm.org/D47290. This is acceptable because Darwin guarantees that, despite the watchOS ABI differences, sizeof(ptrdiff_t) == sizeof(NS[U]Integer), so the warning is therefore noisy for pedantic reasons. Once this is in I'll update public documentation.

rdar://41739204

[jfb]

LGTM after a few questions.

include/clang/Analysis/Analyses/FormatString.h

265

"unspecified" seems odd because it *is* specified, we just don't care. How about something like "NothingSpecial" or "DontCare"?

test/SemaObjC/format-size-spec-nsinteger.m

4

We use a bunch of different triples to test WatchOS:

5 thumbv7k-apple-watchos2.0
3 i386-apple-watchos4
2 x86_64-apple-watchos
2 thumbv7k-apple-watchos
2 armv7k-apple-watchos3.0.0
2 armv7k-apple-watchos2.0
2 armv7k-apple-watchos
1 x86_64-apple-watchos-simulator
1 thumbv7k-apple-watchos1.0
1 i686-apple-watchos
1 i386-apple-watchos3.0-simulator
1 i386-apple-watchos3
1 i386-apple-watchos2.1
1 i386-apple-watchos2.0-simulator
1 i386-apple-watchos2.0
1 i386-apple-watchos-simulator
1 i386-apple-watchos
1 armv7k-apple-watchos2.1
1 armv7-apple-watchos
1 arm64-apple-watchos
1 aarch64-apple-watchos

Do we care about v7k only for this test?

[aaron.ballman]

This is acceptable because Darwin guarantees that, despite the watchOS ABI differences, sizeof(ptrdiff_t) == sizeof(NS[U]Integer)

Can you describe these ABI differences please? Also, does Darwin guarantee that alignof(ptrdiff_t) == alignof(NS[U]Integer)?

[arphaman]

In D48852#1153415, @aaron.ballman wrote:

This is acceptable because Darwin guarantees that, despite the watchOS ABI differences, sizeof(ptrdiff_t) == sizeof(NS[U]Integer)

Can you describe these ABI differences please? Also, does Darwin guarantee that alignof(ptrdiff_t) == alignof(NS[U]Integer)?

The ABI difference boils down to the following:

Regular 32-bit Darwin targets (like armv7) use 'ptrdiff_t' of type 'int', which matches 'NSInteger'.
WatchOS arm iOS target (armv7k) uses 'ptrdiff_t' of type 'long', which doesn't match 'NSInteger' of type 'int'.

Also, does Darwin guarantee that alignof(ptrdiff_t) == alignof(NS[U]Integer)?

Yes.

include/clang/Analysis/Analyses/FormatString.h

265

Thanks. Changed to "DontCare".

test/SemaObjC/format-size-spec-nsinteger.m

4

Yes.
armv7k is used to generate thumbv7k .
i386 is used by simulator, and doesn't have this ABI issue.
We don't support other arches for watches.

[arphaman]

Address review comments.

[aaron.ballman]

In D48852#1153598, @arphaman wrote:

In D48852#1153415, @aaron.ballman wrote:

This is acceptable because Darwin guarantees that, despite the watchOS ABI differences, sizeof(ptrdiff_t) == sizeof(NS[U]Integer)

Can you describe these ABI differences please? Also, does Darwin guarantee that alignof(ptrdiff_t) == alignof(NS[U]Integer)?

The ABI difference boils down to the following:

Regular 32-bit Darwin targets (like armv7) use 'ptrdiff_t' of type 'int', which matches 'NSInteger'.
WatchOS arm iOS target (armv7k) uses 'ptrdiff_t' of type 'long', which doesn't match 'NSInteger' of type 'int'.

Thank you for the explanation, I appreciate it.

include/clang/Analysis/Analyses/FormatString.h

enum Kind { UnknownTy, InvalidTy, SpecificTy, ObjCPointerTy, CPointerTy,
               AnyCharTy, CStrTy, WCStrTy, WIntTy };
 
   enum MatchKind { NoMatch = 0, Match = 1, NoMatchPedantic };
 
 private:
   const Kind K;
   QualType T;
   const char *Name = nullptr;
-  bool Ptr = false, IsSizeT = false;
+  bool Ptr = false;
+
+  /// The TypeKind identifies certain well-known types like size_t and
+  /// ptrdiff_t.
+  enum class TypeKind { DontCare, SizeT, PtrdiffT };
+  TypeKind TK = TypeKind::DontCare;
 
 public:
   ArgType(Kind K = UnknownTy, const char *N = nullptr) : K(K), Name(N) {}
   ArgType(QualType T, const char *N = nullptr) : K(SpecificTy), T(T), Name(N) {}
   ArgType(CanQualType T) : K(SpecificTy), T(T) {}
 
   static ArgType Invalid() { return ArgType(InvalidTy); }
   bool isValid() const { return K != InvalidTy; }
 
-  bool isSizeT() const { return IsSizeT; }
+  bool isSizeT() const { return TK == TypeKind::SizeT; }
+
+  bool isPtrdiffT() const { return TK == TypeKind::PtrdiffT; }
 
   /// Create an ArgType which corresponds to the type pointer to A.
   static ArgType PtrTo(const ArgType& A) {
     assert(A.K >= InvalidTy && "ArgType cannot be pointer to invalid/unknown");
     ArgType Res = A;
     Res.Ptr = true;
     return Res;
   }
 
   /// Create an ArgType which corresponds to the size_t/ssize_t type.
   static ArgType makeSizeT(const ArgType &A) {
     ArgType Res = A;
-    Res.IsSizeT = true;
+    Res.TK = TypeKind::SizeT;
+    return Res;
+  }
+
+  /// Create an ArgType which corresponds to the ptrdiff_t/unsigned ptrdiff_t
+  /// type.
+  static ArgType makePtrdiffT(const ArgType &A) {
+    ArgType Res = A;
+    Res.TK = TypeKind::PtrdiffT;
     return Res;
   }
 
   MatchKind matchesType(ASTContext &C, QualType argTy) const;
 
   QualType getRepresentativeType(ASTContext &C) const;
 
   std::string getRepresentativeTypeName(ASTContext &C) const;

lib/Analysis/PrintfFormatString.cpp

switch (LM.getKind()) {
         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");
       case LengthModifier::AsPtrDiff:
-        return ArgType(Ctx.getPointerDiffType(), "ptrdiff_t");
+        return ArgType::makePtrdiffT(
+            ArgType(Ctx.getPointerDiffType(), "ptrdiff_t"));
       case LengthModifier::AsAllocate:
       case LengthModifier::AsMAllocate:
       case LengthModifier::AsWide:
         return ArgType::Invalid();
     }
 
   if (CS.isUIntArg())
     switch (LM.getKind()) {
switch (LM.getKind()) {
         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");
       case LengthModifier::AsPtrDiff:
-        return ArgType(Ctx.getUnsignedPointerDiffType(), "unsigned ptrdiff_t");
+        return ArgType::makePtrdiffT(
+            ArgType(Ctx.getUnsignedPointerDiffType(), "unsigned ptrdiff_t"));
       case LengthModifier::AsAllocate:
       case LengthModifier::AsMAllocate:
       case LengthModifier::AsWide:
         return ArgType::Invalid();
     }
 
   if (CS.isDoubleArg()) {
     if (LM.getKind() == LengthModifier::AsLongDouble)

lib/Sema/SemaChecking.cpp

CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
 
   // Special-case some of Darwin's platform-independence types by suggesting
   // casts to primitive types that are known to be large enough.
   bool ShouldNotPrintDirectly = false; StringRef CastTyName;
   if (S.Context.getTargetInfo().getTriple().isOSDarwin()) {
     QualType CastTy;
     std::tie(CastTy, CastTyName) = shouldNotPrintDirectly(S.Context, IntendedTy, E);
     if (!CastTy.isNull()) {
-      // %zi/%zu are OK to use for NSInteger/NSUInteger of type int
+      // %zi/%zu and %td/%tu are OK to use for NSInteger/NSUInteger of type int
       // (long in ASTContext). Only complain to pedants.
       if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
-          AT.isSizeT() && AT.matchesType(S.Context, CastTy))
+          (AT.isSizeT() || AT.isPtrdiffT()) &&
+          AT.matchesType(S.Context, CastTy))
         Pedantic = true;
       IntendedTy = CastTy;
       ShouldNotPrintDirectly = true;
     }
   }
 
   // We may be able to offer a FixItHint if it is a supported type.
   PrintfSpecifier fixedFS = FS;

test/SemaObjC/format-size-spec-nsinteger.m

 // RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat %s
 // RUN: %clang_cc1 -triple thumbv7-apple-ios -Wno-objc-root-class -fsyntax-only -verify -Wformat-pedantic -DPEDANTIC %s
+// RUN: %clang_cc1 -triple thumbv7k-apple-watchos2.0.0 -fsyntax-only -fblocks -verify %s
+// RUN: %clang_cc1 -triple thumbv7k-apple-watchos2.0.0 -fsyntax-only -fblocks -verify -Wformat-pedantic -DPEDANTIC %s
 
 #if !defined(PEDANTIC)
 // expected-no-diagnostics
 #endif
 
 #if __LP64__
 typedef unsigned long NSUInteger;
 typedef long NSInteger;
+typedef long ptrdiff_t;
 #else
 typedef unsigned int NSUInteger;
 typedef int NSInteger;
+#if __is_target_os(watchos)
+  // Watch ABI uses long for ptrdiff_t.
+  typedef long ptrdiff_t;
+#else
+  typedef int ptrdiff_t;
+#endif
 #endif
 
 @class NSString;
 
 extern void NSLog(NSString *format, ...);
 
 void testSizeSpecifier() {
   NSInteger i = 0;
   NSUInteger j = 0;
   NSLog(@"max NSInteger = %zi", i);
   NSLog(@"max NSUinteger = %zu", j);
 
 #if defined(PEDANTIC)
   // expected-warning@-4 {{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
   // expected-warning@-4 {{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
 #endif
 }
+
+void testPtrdiffSpecifier(ptrdiff_t x) {
+  NSInteger i = 0;
+  NSUInteger j = 0;
+
+  NSLog(@"ptrdiff_t NSUinteger: %tu", j);
+  NSLog(@"ptrdiff_t NSInteger: %td", i);
+  NSLog(@"ptrdiff_t %tu, %td", x, x);
+#if __is_target_os(watchos) && defined(PEDANTIC)
+  // expected-warning@-4 {{values of type 'NSUInteger' should not be used as format arguments; add an explicit cast to 'unsigned long' instead}}
+  // expected-warning@-4 {{values of type 'NSInteger' should not be used as format arguments; add an explicit cast to 'long' instead}}
+#endif
+}