[Sema] -Wformat-pedantic only for NSInteger/NSUInteger %zu/%zi on Darwin

[jfb]

Summary

Pick D42933 back up, and make NSInteger/NSUInteger with %zu/%zi specifiers on Darwin warn only in pedantic mode. The default -Wformat recently started warning for the following code because of the added support for analysis for the '%zi' specifier.

NSInteger i = NSIntegerMax;
NSLog(@"max NSInteger = %zi", i);

The problem is that on armv7 %zi is 'long', and NSInteger is typedefed to 'int' in Foundation. We should avoid this warning as it's inconvenient to our users: it's target specific (happens only on armv7 and not arm64), and breaks their existing code. We should also silence the warning for the '%zu' specifier to ensure consistency. This is acceptable because Darwin guarantees that, despite the unfortunate choice of typedef, sizeof(size_t) == sizeof(NS[U]Integer), the warning is therefore noisy for pedantic reasons. Once this is in I'll update public documentation.

Related discussion on cfe-dev:
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058050.html

rdar://36874921&40501559

[alexander-shaposhnikov]

test/FixIt/fixit-format-ios-nopedantic.m

21 ↗

(On Diff #148296)

maybe i'm missing smth, but i don't see any verification in this test.

[jfb]

test/FixIt/fixit-format-ios-nopedantic.m

21 ↗

(On Diff #148296)

-Werror makes the test fail if something is reported.

[ahatanak]

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

18 ↗

(On Diff #148296)

This test looks identical to test/SemaObjC/format-size-spec-nsinteger-nopedantic.m except for the CHECK lines. You can probably merge the two files if you separate the CHECK lines from the lines that call NSLog (see test/Sema/format-strings-darwin.c for example).

[jfb]

• Merge format-size-spec-nsinteger

[jfb]

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

18 ↗

(On Diff #148296)

Cute, I didn't know about this pattern.

[aaron.ballman]

This is relaxing -Wformat and making users instead write -Wformat-pedantic to get the strong guarantees, which is the opposite of what I thought the consensus was. Have I misunderstood something?

include/clang/Analysis/Analyses/FormatString.h

263–265 ↗

(On Diff #148467)

If we're going to update this, can you fix the parameter names to be in line with the coding conventions?

lib/Sema/SemaChecking.cpp

6597–6599 ↗

(On Diff #148467)

These identifiers also need some love to match the coding conventions. Happens elsewhere in this patch as well.

[jfb]

In D47290#1113365, @aaron.ballman wrote:

This is relaxing -Wformat and making users instead write -Wformat-pedantic to get the strong guarantees, which is the opposite of what I thought the consensus was. Have I misunderstood something?

From Shoaib's email:

Based on all the discussion so far, I think the consensus is that we don't want to relax -Wformat by default, but an additional relaxed option would be fine. That works for me (where I can just switch my codebases to use the new warning option), but it wouldn't handle JF Bastien's use case, so he would still want to pursue the relaxations specific to Apple platforms.

This patch is specific to the Darwin platform, as proposed in Alex' original patch and as I promised to do in the cfe-dev thread. Shoaib's follow-up would be different and do what I think you're referring to.

[jfb]

• Fix variable capitalization.

[jfb]

Addressed comments.

[aaron.ballman]

In D47290#1113412, @jfb wrote:

In D47290#1113365, @aaron.ballman wrote:

This is relaxing -Wformat and making users instead write -Wformat-pedantic to get the strong guarantees, which is the opposite of what I thought the consensus was. Have I misunderstood something?

From Shoaib's email:

Based on all the discussion so far, I think the consensus is that we don't want to relax -Wformat by default, but an additional relaxed option would be fine. That works for me (where I can just switch my codebases to use the new warning option), but it wouldn't handle JF Bastien's use case, so he would still want to pursue the relaxations specific to Apple platforms.

This patch is specific to the Darwin platform, as proposed in Alex' original patch and as I promised to do in the cfe-dev thread. Shoaib's follow-up would be different and do what I think you're referring to.

There were several people on the thread asking to not relax -Wformat for any target, but instead wanted a separate warning flag to perform a more relaxed check.

http://lists.llvm.org/pipermail/cfe-dev/2018-May/057938.html
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058030.html
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058039.html

[jfb]

In D47290#1113422, @aaron.ballman wrote:

In D47290#1113412, @jfb wrote:

In D47290#1113365, @aaron.ballman wrote:

This is relaxing -Wformat and making users instead write -Wformat-pedantic to get the strong guarantees, which is the opposite of what I thought the consensus was. Have I misunderstood something?

From Shoaib's email:

Based on all the discussion so far, I think the consensus is that we don't want to relax -Wformat by default, but an additional relaxed option would be fine. That works for me (where I can just switch my codebases to use the new warning option), but it wouldn't handle JF Bastien's use case, so he would still want to pursue the relaxations specific to Apple platforms.

This patch is specific to the Darwin platform, as proposed in Alex' original patch and as I promised to do in the cfe-dev thread. Shoaib's follow-up would be different and do what I think you're referring to.

There were several people on the thread asking to not relax -Wformat for any target, but instead wanted a separate warning flag to perform a more relaxed check.

http://lists.llvm.org/pipermail/cfe-dev/2018-May/057938.html

I don't think this one is talking about platform-specific warnings, I therefore don't think it applies to this patch.

http://lists.llvm.org/pipermail/cfe-dev/2018-May/058030.html
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058039.html

Hans' comment and your +1 are on-topic here, and are the only ones AFAIK (others don't care or explicitly agree with this patch). What I'd like to emphasize is that the new stricter warnings are causing issues for developers on our platform. The warnings are new, and they don't help developers because the platform guarantees that their code is fine. That frustrates developers because they *know* that we know better (they might even find this discussion when searching! 👋), yet we chose to come in and warn them anyways. I have yet to hear a reason why we're doing them a service by warning in this case. It really has the feel of a pedantic warning, and that's what this patch makes it.

I'll quote James slightly out of context:

No, this actually has been a thorn in the side of some people at google,
causing a good deal of angst (only for a very small number of people,
granted). I'd not truly put the blame on the warning, but rather on printf
itself -- and that we're still USING printf (and other functions that
ultimately wrap printf) all over the place. The long-term plan is certainly
to switch to better APIs. But, it'd be nice to be able to reduce the issue
in the meantime.

The problem we have is that Google has an internal "int64" typedef, which
predates the availability of C99 "int64_t". We'd like to eliminate this,
and switch everything over to using the standard int64_t. (Well, really
we'd've liked to have done that years ago...) However, "int64" is defined
as "long long", while "int64_t" is defined as "long" on 64-bit linux. A
bunch of printf specifiers all throughout the codebase use %lld. This
mismatching type-specifier makes it quite difficult to change the
type. Automatically updating all the format strings has not been found to
be easy.

If we could eliminate the warning on mismatch of "%lld" vs "long", on a
platform where they're the same, that could make things a whole lot easier.

It seems Hans' comment came from the sentiment "warning is the right thing to do, and from Google's vast experience it's not a problem". James provides a counter-point in line with ours: we receive exactly that feedback for NSInteger and %z. The code exists, it works fine, the platform guarantees it'll keep working fine, and now a tonne of "useless" warnings come in. That's frustrating because it means developers have these options:

• Useless code churn in code that otherwise won't change, ever
• Silence the warning, potentially silencing useful warnings later (because -Wformat-relaxed could start doing more things)

That's frustrating when you update compilers, especially if your update is incremental because only some developers see the warning. It means people are reluctant to update, because the first thing they see is us being pedants, not the cool new features we bring in. If they saw a flood of really useful warnings instead they'd be happy, but even there they get drowned by these format warnings... Kind of a shit sandwich if you'll pardon the graphic description.

Hopefully this makes sense? I'm not sure I summarize my context very well. I'm happy to talk about it in-person next week too.

[aaron.ballman]

In D47290#1115352, @jfb wrote:

Hopefully this makes sense? I'm not sure I summarize my context very well. I'm happy to talk about it in-person next week too.

I appreciate the in-person conversation, it helped me to understand your goals better.

That said, I still see this as changing the portability aspects of -Wformat (because we'd be changing the behavior based on the target, but this is perhaps different from the portability concerns @rjmccall raised) and I don't see where to get off that train. For instance, do we also silence -Wformat when targeting Windows with %ld and %d because long and int have the same size and alignment requirements for x86 and x86-64 (at least, I don't know about other Windows targets) and that is effectively baked into the platform requirements? If we don't, how is that situation different than the one motivating your changes?

As a user, I very much appreciate that -Wformat gives me the same diagnostics no matter what platform I target, so these changes make me a bit uncomfortable -- those warnings are not useless to me. You (rightly) asked for a concrete explanation of why. My reason is: the user's code here is UB and there is a long history demonstrating that "it's UB, but it works and does what I want!" suddenly changing into "it's not what I want" silently, perhaps many years down the line, because the optimizer suddenly got smarter and it results in exploitable security vulnerabilities. However, I'm no longer as scared that the optimizer is going to start making type-based decisions based on this format specifier string. I can imagine the optimizer making value-based decisions based on a format specifier string (like seeing a value matches a %s specifier and assuming the value cannot be null), but that's not this. However, I have had users for which UB is verboten. Period. Full stop. I doubt these users care at all about Apple's platform, so *this* change isn't likely to impact them, but the *precedent* from this change certainly could. However, perhaps making this pedantic as in this patch will be reasonable (I believe these folks already pass -pedantic when they realize that's a thing). I have to do my homework on that (and can't do that homework this week).

[hans]

What's special about size_t though? If I understand your patch correctly, it would suppress warning about printing NSInteger with %zd, but still warn about %ld even though ssize_t=long on the target? As a user I'd find this confusing.

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Also, I haven't looked at what happens on the scanf side. Does that need an equivalent change?

[aaron.ballman]

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Would you be similarly okay with %ld and %d on Windows platforms when mixing up int and long?

[hans]

In D47290#1124933, @aaron.ballman wrote:

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Would you be similarly okay with %ld and %d on Windows platforms when mixing up int and long?

No, I'm against a general relaxation of -Wformat, but to solve JF's problem I think special-casing NSInteger might be reasonable.

[aaron.ballman]

In D47290#1124956, @hans wrote:

In D47290#1124933, @aaron.ballman wrote:

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Would you be similarly okay with %ld and %d on Windows platforms when mixing up int and long?

No, I'm against a general relaxation of -Wformat, but to solve JF's problem I think special-casing NSInteger might be reasonable.

How is JF's problem different?

[hans]

In D47290#1124964, @aaron.ballman wrote:

In D47290#1124956, @hans wrote:

In D47290#1124933, @aaron.ballman wrote:

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Would you be similarly okay with %ld and %d on Windows platforms when mixing up int and long?

No, I'm against a general relaxation of -Wformat, but to solve JF's problem I think special-casing NSInteger might be reasonable.

How is JF's problem different?

It concerns a vendor-specific type. Of course I personally think it would be better if the code could be fixed, but it doesn't sound like that's an option so then I think special-casing for NSInteger is an acceptable solution.

[aaron.ballman]

In D47290#1124991, @hans wrote:

In D47290#1124964, @aaron.ballman wrote:

In D47290#1124956, @hans wrote:

In D47290#1124933, @aaron.ballman wrote:

In D47290#1124866, @hans wrote:

If we really want to special-case NSInteger, and given that you're targeting a specific wide-spread pattern maybe that's the right thing to do, I think we should make -Wformat accept (move the warning behind -Wformat-pedantic I suppose) printing NSInteger with *any* integral type of the right size, not just size_t.

Would you be similarly okay with %ld and %d on Windows platforms when mixing up int and long?

No, I'm against a general relaxation of -Wformat, but to solve JF's problem I think special-casing NSInteger might be reasonable.

How is JF's problem different?

It concerns a vendor-specific type. Of course I personally think it would be better if the code could be fixed, but it doesn't sound like that's an option so then I think special-casing for NSInteger is an acceptable solution.

Okay, that's fair, but the vendor-specific type for my Windows example is spelled DWORD. I'm really worried that this special case will become a precedent and we'll wind up with -Wformat being relaxed for everything based on the same rationale. If that's how the community wants -Wformat to work, cool, but I'd like to know if we're intending to change (what I see as) the design of this warning.

[aaron.ballman]

In D47290#1125028, @aaron.ballman wrote:

Okay, that's fair, but the vendor-specific type for my Windows example is spelled DWORD. I'm really worried that this special case will become a precedent and we'll wind up with -Wformat being relaxed for everything based on the same rationale. If that's how the community wants -Wformat to work, cool, but I'd like to know if we're intending to change (what I see as) the design of this warning.

I spoke with @jfb offline and am less concerned about this patch now. He's welcome to correct me if I misrepresent anything, but the precedent this sets is that a platform "owner" (someone with authority, not Joe Q Random-User) can relax -Wformat as in this patch, but this is not a precedent for a blanket change to -Wformat just because the UB happens to work and we "know" it.

[jfb]

In D47290#1126443, @aaron.ballman wrote:

In D47290#1125028, @aaron.ballman wrote:

Okay, that's fair, but the vendor-specific type for my Windows example is spelled DWORD. I'm really worried that this special case will become a precedent and we'll wind up with -Wformat being relaxed for everything based on the same rationale. If that's how the community wants -Wformat to work, cool, but I'd like to know if we're intending to change (what I see as) the design of this warning.

I spoke with @jfb offline and am less concerned about this patch now. He's welcome to correct me if I misrepresent anything, but the precedent this sets is that a platform "owner" (someone with authority, not Joe Q Random-User) can relax -Wformat as in this patch, but this is not a precedent for a blanket change to -Wformat just because the UB happens to work and we "know" it.

Thanks for asking these questions Aaron, it's helped answer everyone's concerns and explain our respective positions. You've certainly summarized what I was thinking.

It sounds like you're both OK moving forward with this patch?

[aaron.ballman]

In D47290#1127190, @jfb wrote:

In D47290#1126443, @aaron.ballman wrote:

In D47290#1125028, @aaron.ballman wrote:

Okay, that's fair, but the vendor-specific type for my Windows example is spelled DWORD. I'm really worried that this special case will become a precedent and we'll wind up with -Wformat being relaxed for everything based on the same rationale. If that's how the community wants -Wformat to work, cool, but I'd like to know if we're intending to change (what I see as) the design of this warning.

I spoke with @jfb offline and am less concerned about this patch now. He's welcome to correct me if I misrepresent anything, but the precedent this sets is that a platform "owner" (someone with authority, not Joe Q Random-User) can relax -Wformat as in this patch, but this is not a precedent for a blanket change to -Wformat just because the UB happens to work and we "know" it.

Thanks for asking these questions Aaron, it's helped answer everyone's concerns and explain our respective positions. You've certainly summarized what I was thinking.

Excellent!

It sounds like you're both OK moving forward with this patch?

I am okay moving forward.

[arphaman]

LGTM. Thanks for working on this!

cfe/trunk/include/clang/Analysis/Analyses/FormatString.h

public:
   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;
-  bool Ptr;
+  const char *Name = nullptr;
+  bool Ptr = false, IsSizeT = false;
+
 public:
-  ArgType(Kind k = UnknownTy, const char *n = nullptr)
-      : K(k), Name(n), Ptr(false) {}
-  ArgType(QualType t, const char *n = nullptr)
-      : K(SpecificTy), T(t), Name(n), Ptr(false) {}
-  ArgType(CanQualType t) : K(SpecificTy), T(t), Name(nullptr), Ptr(false) {}
+  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; }
+
   /// 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;
+    return Res;
+  }
+
   MatchKind matchesType(ASTContext &C, QualType argTy) const;
 
   QualType getRepresentativeType(ASTContext &C) const;
 
   std::string getRepresentativeTypeName(ASTContext &C) const;
 };
 
 class OptionalAmount {

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_format_conversion_argument_type_mismatch_pedantic : Extension<
   "format specifies type %0 but the argument has "
   "%select{type|underlying type}2 %1">,
   InGroup<FormatPedantic>;
 def warn_format_argument_needs_cast : Warning<
   "%select{values of type|enum values with underlying type}2 '%0' should not "
   "be used as format arguments; add an explicit cast to %1 instead">,
   InGroup<Format>;
+def warn_format_argument_needs_cast_pedantic : Warning<
+  "%select{values of type|enum values with underlying type}2 '%0' should not "
+  "be used as format arguments; add an explicit cast to %1 instead">,
+  InGroup<FormatPedantic>, DefaultIgnore;
 def warn_printf_positional_arg_exceeds_data_args : Warning <
   "data argument position '%0' exceeds the number of data arguments (%1)">,
   InGroup<Format>;
 def warn_format_zero_positional_specifier : Warning<
   "position arguments in format strings start counting at 1 (not 0)">,
   InGroup<Format>;
 def warn_format_invalid_positional_specifier : Warning<
   "invalid position specified for %select{field width|field precision}0">,

cfe/trunk/lib/Analysis/PrintfFormatString.cpp

switch (LM.getKind()) {
       case LengthModifier::AsLongLong:
       case LengthModifier::AsQuad:
         return Ctx.LongLongTy;
       case LengthModifier::AsInt64:
         return ArgType(Ctx.LongLongTy, "__int64");
       case LengthModifier::AsIntMax:
         return ArgType(Ctx.getIntMaxType(), "intmax_t");
       case LengthModifier::AsSizeT:
-        return ArgType(Ctx.getSignedSizeType(), "ssize_t");
+        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");
       case LengthModifier::AsAllocate:
       case LengthModifier::AsMAllocate:
switch (LM.getKind()) {
       case LengthModifier::AsLongLong:
       case LengthModifier::AsQuad:
         return Ctx.UnsignedLongLongTy;
       case LengthModifier::AsInt64:
         return ArgType(Ctx.UnsignedLongLongTy, "unsigned __int64");
       case LengthModifier::AsIntMax:
         return ArgType(Ctx.getUIntMaxType(), "uintmax_t");
       case LengthModifier::AsSizeT:
-        return ArgType(Ctx.getSizeType(), "size_t");
+        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");
       case LengthModifier::AsAllocate:
       case LengthModifier::AsMAllocate:

cfe/trunk/lib/Sema/SemaChecking.cpp

CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
   if (!AT.isValid())
     return true;
 
   QualType ExprTy = E->getType();
   while (const TypeOfExprType *TET = dyn_cast<TypeOfExprType>(ExprTy)) {
     ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  analyze_printf::ArgType::MatchKind match = AT.matchesType(S.Context, ExprTy);
-
-  if (match == analyze_printf::ArgType::Match) {
+  const analyze_printf::ArgType::MatchKind Match =
+      AT.matchesType(S.Context, ExprTy);
+  bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic;
+  if (Match == analyze_printf::ArgType::Match)
     return true;
-  }
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
   // and function pointer decay; seeing that an argument intended to be a
   // string has type 'char [6]' is probably more confusing than 'char *'.
   if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
     if (ICE->getCastKind() == CK_IntegralCast ||
         ICE->getCastKind() == CK_FloatingCast) {
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
+      // (long in ASTContext). Only complain to pedants.
+      if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
+          AT.isSizeT() && 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;
-  bool success =
+  bool Success =
       fixedFS.fixType(IntendedTy, S.getLangOpts(), S.Context, isObjCContext());
 
-  if (success) {
+  if (Success) {
     // Get the fix string from the fixed format specifier
     SmallString<16> buf;
     llvm::raw_svector_ostream os(buf);
     fixedFS.toString(os);
 
     CharSourceRange SpecRange = getSpecifierRange(StartSpecifier, SpecifierLen);
 
     if (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
-      unsigned diag = diag::warn_format_conversion_argument_type_mismatch;
-      if (match == analyze_format_string::ArgType::NoMatchPedantic) {
-        diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
-      }
+      unsigned Diag =
+          Pedantic
+              ? diag::warn_format_conversion_argument_type_mismatch_pedantic
+              : diag::warn_format_conversion_argument_type_mismatch;
       // In this case, the specifier is wrong and should be changed to match
       // the argument.
-      EmitFormatDiagnostic(S.PDiag(diag)
+      EmitFormatDiagnostic(S.PDiag(Diag)
                                << AT.getRepresentativeTypeName(S.Context)
                                << IntendedTy << IsEnum << E->getSourceRange(),
                            E->getLocStart(),
                            /*IsStringLocation*/ false, SpecRange,
                            FixItHint::CreateReplacement(SpecRange, os.str()));
     } else {
       // The canonical type for formatting this value is different from the
       // actual type of the expression. (This occurs, for example, with Darwin's
if (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
         // The expression has a type that should not be printed directly.
         // We extract the name from the typedef because we don't want to show
         // the underlying type in the diagnostic.
         StringRef Name;
         if (const TypedefType *TypedefTy = dyn_cast<TypedefType>(ExprTy))
           Name = TypedefTy->getDecl()->getName();
         else
           Name = CastTyName;
-        EmitFormatDiagnostic(S.PDiag(diag::warn_format_argument_needs_cast)
-                               << Name << IntendedTy << IsEnum
+        unsigned Diag = Pedantic
+                            ? diag::warn_format_argument_needs_cast_pedantic
+                            : diag::warn_format_argument_needs_cast;
+        EmitFormatDiagnostic(S.PDiag(Diag) << Name << IntendedTy << IsEnum
                                            << E->getSourceRange(),
                              E->getLocStart(), /*IsStringLocation=*/false,
                              SpecRange, Hints);
       } else {
         // In this case, the expression could be printed using a different
         // specifier, but we've decided that the specifier is probably correct 
         // and we should cast instead. Just use the normal warning message.
         EmitFormatDiagnostic(
           S.PDiag(diag::warn_format_conversion_argument_type_mismatch)
             << AT.getRepresentativeTypeName(S.Context) << ExprTy << IsEnum
             << E->getSourceRange(),
           E->getLocStart(), /*IsStringLocation*/false,
           SpecRange, Hints);
       }
     }
   } else {
     const CharSourceRange &CSR = getSpecifierRange(StartSpecifier,
                                                    SpecifierLen);
     // Since the warning for passing non-POD types to variadic functions
     // was deferred until now, we emit a warning for non-POD
     // arguments here.
     switch (S.isValidVarArgType(ExprTy)) {
     case Sema::VAK_Valid:
     case Sema::VAK_ValidInCXX11: {
-      unsigned diag = diag::warn_format_conversion_argument_type_mismatch;
-      if (match == analyze_printf::ArgType::NoMatchPedantic) {
-        diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
-      }
+      unsigned Diag =
+          Pedantic
+              ? diag::warn_format_conversion_argument_type_mismatch_pedantic
+              : diag::warn_format_conversion_argument_type_mismatch;
 
       EmitFormatDiagnostic(
-          S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context) << ExprTy
+          S.PDiag(Diag) << AT.getRepresentativeTypeName(S.Context) << ExprTy
                         << IsEnum << CSR << E->getSourceRange(),
           E->getLocStart(), /*IsStringLocation*/ false, CSR);
       break;
     }
     case Sema::VAK_Undefined:
     case Sema::VAK_MSVCUndefined:
       EmitFormatDiagnostic(
         S.PDiag(diag::warn_non_pod_vararg_with_format_string)
if (!Ex)
     return true;
 
   const analyze_format_string::ArgType &AT = FS.getArgType(S.Context);
 
   if (!AT.isValid()) {
     return true;
   }
 
-  analyze_format_string::ArgType::MatchKind match =
+  analyze_format_string::ArgType::MatchKind Match =
       AT.matchesType(S.Context, Ex->getType());
-  if (match == analyze_format_string::ArgType::Match) {
+  bool Pedantic = Match == analyze_format_string::ArgType::NoMatchPedantic;
+  if (Match == analyze_format_string::ArgType::Match)
     return true;
-  }
 
   ScanfSpecifier fixedFS = FS;
-  bool success = fixedFS.fixType(Ex->getType(), Ex->IgnoreImpCasts()->getType(),
+  bool Success = fixedFS.fixType(Ex->getType(), Ex->IgnoreImpCasts()->getType(),
                                  S.getLangOpts(), S.Context);
 
-  unsigned diag = diag::warn_format_conversion_argument_type_mismatch;
-  if (match == analyze_format_string::ArgType::NoMatchPedantic) {
-    diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
-  }
+  unsigned Diag =
+      Pedantic ? diag::warn_format_conversion_argument_type_mismatch_pedantic
+               : diag::warn_format_conversion_argument_type_mismatch;
 
-  if (success) {
+  if (Success) {
     // Get the fix string from the fixed format specifier.
     SmallString<128> buf;
     llvm::raw_svector_ostream os(buf);
     fixedFS.toString(os);
 
     EmitFormatDiagnostic(
-        S.PDiag(diag) << AT.getRepresentativeTypeName(S.Context)
+        S.PDiag(Diag) << AT.getRepresentativeTypeName(S.Context)
                       << Ex->getType() << false << Ex->getSourceRange(),
         Ex->getLocStart(),
         /*IsStringLocation*/ false,
         getSpecifierRange(startSpecifier, specifierLen),
         FixItHint::CreateReplacement(
             getSpecifierRange(startSpecifier, specifierLen), os.str()));
   } else {
-    EmitFormatDiagnostic(S.PDiag(diag)
+    EmitFormatDiagnostic(S.PDiag(Diag)
                              << AT.getRepresentativeTypeName(S.Context)
                              << Ex->getType() << false << Ex->getSourceRange(),
                          Ex->getLocStart(),
                          /*IsStringLocation*/ false,
                          getSpecifierRange(startSpecifier, specifierLen));
   }
 
   return true;

cfe/trunk/test/FixIt/fixit-format-ios-nopedantic.m

+// RUN: cp %s %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -Werror -fixit %t
+
+int printf(const char *restrict, ...);
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+NSUInteger getNSUInteger();
+NSInteger getNSInteger();
+
+void test() {
+  // For thumbv7-apple-ios8.0.0 the underlying type of ssize_t is long
+  // and the underlying type of size_t is unsigned long.
+
+  printf("test 1: %zu", getNSUInteger());
+
+  printf("test 2: %zu %zu", getNSUInteger(), getNSUInteger());
+
+  printf("test 3: %zd", getNSInteger());
+
+  printf("test 4: %zd %zd", getNSInteger(), getNSInteger());
+}

cfe/trunk/test/FixIt/fixit-format-ios.m

 // RUN: cp %s %t
-// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat -fixit %t
+// RUN: %clang_cc1 -triple thumbv7-apple-ios8.0.0 -fsyntax-only -Wformat-pedantic -fixit %t
 // RUN: grep -v CHECK %t | FileCheck %s
 
 int printf(const char * restrict, ...);
 typedef unsigned int NSUInteger;
 typedef int NSInteger;
 NSUInteger getNSUInteger();
 NSInteger getNSInteger();
 

cfe/trunk/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
+
+#if !defined(PEDANTIC)
+// expected-no-diagnostics
+#endif
+
+#if __LP64__
+typedef unsigned long NSUInteger;
+typedef long NSInteger;
+#else
+typedef unsigned int NSUInteger;
+typedef int NSInteger;
+#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
+}