Added FixIt support to printf format string checking.

- Refactored LengthModifier to be a class.
- Added toString methods in all member classes of FormatSpecifier.
- FixIt suggestions keep user specified flags unless incorrect.

Limitations:
- The suggestions are not conversion specifier sensitive. For example, if we have a 'pad with zeroes' flag, and the correction is a string conversion specifier, we do not remove the flag. Clang will warn us on the next compilation.

A    test/Sema/format-strings-fixit.c
M    include/clang/Analysis/Analyses/PrintfFormatString.h
M    lib/Analysis/PrintfFormatString.cpp
M    lib/Sema/SemaChecking.cpp


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@105680 91177308-0d34-0410-b5e6-96231b3b80d8

4 files changed, 352 insertions, 54 deletions

diff --git a/include/clang/Analysis/Analyses/PrintfFormatString.h b/include/clang/Analysis/Analyses/PrintfFormatString.h
index e4f7c57061..4e23e7c768 100644
--- a/include/clang/Analysis/Analyses/PrintfFormatString.h
+++ b/include/clang/Analysis/Analyses/PrintfFormatString.h
@@ -124,45 +124,85 @@ public:
   bool isUIntArg() const { return kind >= oArg && kind <= XArg; }
   bool isDoubleArg() const { return kind >= fArg && kind <= AArg; }
   Kind getKind() const { return kind; }
+  void setKind(Kind k) { kind = k; }
   unsigned getLength() const {
     // Conversion specifiers currently only are represented by
     // single characters, but we be flexible.
     return 1;
   }
+  const char *toString() const;
 
 private:
   const char *Position;
   Kind kind;
 };
 
-enum LengthModifier {
- None,
- AsChar,      // 'hh'
- AsShort,     // 'h'
- AsLong,      // 'l'
- AsLongLong,  // 'll', 'q' (BSD, deprecated)
- AsIntMax,    // 'j'
- AsSizeT,     // 'z'
- AsPtrDiff,   // 't'
- AsLongDouble, // 'L'
- AsWideChar = AsLong // for '%ls'
+class LengthModifier {
+public:
+  enum Kind {
+   None,
+   AsChar,       // 'hh'
+   AsShort,      // 'h'
+   AsLong,       // 'l'
+   AsLongLong,   // 'll', 'q' (BSD, deprecated)
+   AsIntMax,     // 'j'
+   AsSizeT,      // 'z'
+   AsPtrDiff,    // 't'
+   AsLongDouble, // 'L'
+   AsWideChar = AsLong // for '%ls'
+  };
+
+  LengthModifier()
+    : Position(0), kind(None) {}
+  LengthModifier(const char *pos, Kind k)
+    : Position(pos), kind(k) {}
+
+  const char *getStart() const {
+    return Position;
+  }
+
+  unsigned getLength() const {
+    switch (kind) {
+    default:
+      return 1;
+    case AsLongLong:
+      return 2;
+    case None:
+      return 0;
+    }
+  }
+
+  Kind getKind() const { return kind; }
+  void setKind(Kind k) { kind = k; }
+
+  const char *toString() const;
+
+private:
+  const char *Position;
+  Kind kind;
 };
 
 class OptionalAmount {
 public:
   enum HowSpecified { NotSpecified, Constant, Arg, Invalid };
 
-  OptionalAmount(HowSpecified h, unsigned i, const char *st)
-    : start(st), hs(h), amt(i) {}
+  OptionalAmount(HowSpecified howSpecified,
+                 unsigned amount,
+                 const char *amountStart,
+                 bool usesPositionalArg)
+    : start(amountStart), hs(howSpecified), amt(amount),
+      UsesPositionalArg(usesPositionalArg) {}
 
-  OptionalAmount(bool b = true)
-    : start(0), hs(b ? NotSpecified : Invalid), amt(0) {}
+  OptionalAmount(bool valid = true)
+    : start(0), hs(valid ? NotSpecified : Invalid), amt(0),
+      UsesPositionalArg(0) {}
 
   bool isInvalid() const {
     return hs == Invalid;
   }
 
   HowSpecified getHowSpecified() const { return hs; }
+  void setHowSpecified(HowSpecified h) { hs = h; }
 
   bool hasDataArgument() const { return hs == Arg; }
 
@@ -182,10 +222,19 @@ public:
 
   ArgTypeResult getArgType(ASTContext &Ctx) const;
 
+  void toString(llvm::raw_ostream &os) const;
+
+  bool usesPositionalArg() const { return (bool) UsesPositionalArg; }
+  unsigned getPositionalArgIndex() const {
+    assert(hasDataArgument());
+    return amt + 1;
+  }
+
 private:
   const char *start;
   HowSpecified hs;
   unsigned amt;
+  bool UsesPositionalArg : 1;
 };
 
 class FormatSpecifier {
@@ -204,7 +253,7 @@ class FormatSpecifier {
   OptionalAmount FieldWidth;
   OptionalAmount Precision;
 public:
-  FormatSpecifier() : LM(None),
+  FormatSpecifier() :
     IsLeftJustified(0), HasPlusPrefix(0), HasSpacePrefix(0),
     HasAlternativeForm(0), HasLeadingZeroes(0), UsesPositionalArg(0),
     argIndex(0) {}
@@ -235,6 +284,11 @@ public:
     return argIndex;
   }
 
+  unsigned getPositionalArgIndex() const {
+    assert(CS.consumesDataArgument());
+    return argIndex + 1;
+  }
+
   // Methods for querying the format specifier.
 
   const ConversionSpecifier &getConversionSpecifier() const {
@@ -274,6 +328,13 @@ public:
   bool hasLeadingZeros() const { return (bool) HasLeadingZeroes; }
   bool hasSpacePrefix() const { return (bool) HasSpacePrefix; }
   bool usesPositionalArg() const { return (bool) UsesPositionalArg; }
+
+  /// Changes the specifier and length according to a QualType, retaining any
+  /// flags or options. Returns true on success, or false when a conversion
+  /// was not successful.
+  bool fixType(QualType QT);
+
+  void toString(llvm::raw_ostream &os) const;
 };
 
 enum PositionContext { FieldWidthPos = 0, PrecisionPos = 1 };
diff --git a/lib/Analysis/PrintfFormatString.cpp b/lib/Analysis/PrintfFormatString.cpp
index c38aae3476..381b15ddc7 100644
--- a/lib/Analysis/PrintfFormatString.cpp
+++ b/lib/Analysis/PrintfFormatString.cpp
@@ -14,12 +14,16 @@
 
 #include "clang/Analysis/Analyses/PrintfFormatString.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Type.h"
+#include "llvm/Support/raw_ostream.h"
 
 using clang::analyze_printf::ArgTypeResult;
 using clang::analyze_printf::FormatSpecifier;
 using clang::analyze_printf::FormatStringHandler;
 using clang::analyze_printf::OptionalAmount;
 using clang::analyze_printf::PositionContext;
+using clang::analyze_printf::ConversionSpecifier;
+using clang::analyze_printf::LengthModifier;
 
 using namespace clang;
 
@@ -80,7 +84,7 @@ static OptionalAmount ParseAmount(const char *&Beg, const char *E) {
     }
 
     if (hasDigits)
-      return OptionalAmount(OptionalAmount::Constant, accumulator, Beg);
+      return OptionalAmount(OptionalAmount::Constant, accumulator, Beg, 0);
 
     break;
   }
@@ -92,7 +96,7 @@ static OptionalAmount ParseNonPositionAmount(const char *&Beg, const char *E,
                                              unsigned &argIndex) {
   if (*Beg == '*') {
     ++Beg;
-    return OptionalAmount(OptionalAmount::Arg, argIndex++, Beg);
+    return OptionalAmount(OptionalAmount::Arg, argIndex++, Beg, 0);
   }
 
   return ParseAmount(Beg, E);
@@ -120,6 +124,8 @@ static OptionalAmount ParsePositionAmount(FormatStringHandler &H,
     assert(Amt.getHowSpecified() == OptionalAmount::Constant);
 
     if (*I == '$') {
+      // Handle positional arguments
+
       // Special case: '*0$', since this is an easy mistake.
       if (Amt.getConstantAmount() == 0) {
         H.HandleZeroPosition(Beg, I - Beg + 1);
@@ -130,7 +136,7 @@ static OptionalAmount ParsePositionAmount(FormatStringHandler &H,
       Beg = ++I;
 
       return OptionalAmount(OptionalAmount::Arg, Amt.getConstantAmount() - 1,
-                            Tmp);
+                            Tmp, 1);
     }
 
     H.HandleInvalidPosition(Beg, I - Beg, p);
@@ -304,24 +310,28 @@ static FormatSpecifierResult ParseFormatSpecifier(FormatStringHandler &H,
   }
 
   // Look for the length modifier.
-  LengthModifier lm = None;
+  LengthModifier::Kind lmKind = LengthModifier::None;
+  const char *lmPosition = I;
   switch (*I) {
     default:
       break;
     case 'h':
       ++I;
-      lm = (I != E && *I == 'h') ? ++I, AsChar : AsShort;
+      lmKind = (I != E && *I == 'h') ?
+          ++I, LengthModifier::AsChar : LengthModifier::AsShort;
       break;
     case 'l':
       ++I;
-      lm = (I != E && *I == 'l') ? ++I, AsLongLong : AsLong;
+      lmKind = (I != E && *I == 'l') ?
+          ++I, LengthModifier::AsLongLong : LengthModifier::AsLong;
       break;
-    case 'j': lm = AsIntMax;     ++I; break;
-    case 'z': lm = AsSizeT;      ++I; break;
-    case 't': lm = AsPtrDiff;    ++I; break;
-    case 'L': lm = AsLongDouble; ++I; break;
-    case 'q': lm = AsLongLong;   ++I; break;
+    case 'j': lmKind = LengthModifier::AsIntMax;     ++I; break;
+    case 'z': lmKind = LengthModifier::AsSizeT;      ++I; break;
+    case 't': lmKind = LengthModifier::AsPtrDiff;    ++I; break;
+    case 'L': lmKind = LengthModifier::AsLongDouble; ++I; break;
+    case 'q': lmKind = LengthModifier::AsLongLong;   ++I; break;
   }
+  LengthModifier lm(lmPosition, lmKind);
   FS.setLengthModifier(lm);
 
   if (I == E) {
@@ -515,6 +525,94 @@ ArgTypeResult OptionalAmount::getArgType(ASTContext &Ctx) const {
 }
 
 //===----------------------------------------------------------------------===//
+// Methods on ConversionSpecifier.
+//===----------------------------------------------------------------------===//
+const char *ConversionSpecifier::toString() const {
+  switch (kind) {
+  case dArg: return "d";
+  case iArg: return "i";
+  case oArg: return "o";
+  case uArg: return "u";
+  case xArg: return "x";
+  case XArg: return "X";
+  case fArg: return "f";
+  case FArg: return "F";
+  case eArg: return "e";
+  case EArg: return "E";
+  case gArg: return "g";
+  case GArg: return "G";
+  case aArg: return "a";
+  case AArg: return "A";
+  case IntAsCharArg:     return "c";
+  case CStrArg:          return "s";
+  case VoidPtrArg:       return "p";
+  case OutIntPtrArg:     return "n";
+  case PercentArg:       return "%";
+  case InvalidSpecifier: return NULL;
+
+  // MacOS X unicode extensions.
+  case CArg:          return "C";
+  case UnicodeStrArg: return "S";
+
+  // Objective-C specific specifiers.
+  case ObjCObjArg: return "@";
+
+  // GlibC specific specifiers.
+  case PrintErrno: return "m";
+  }
+  return NULL;
+}
+
+//===----------------------------------------------------------------------===//
+// Methods on LengthModifier.
+//===----------------------------------------------------------------------===//
+
+const char *LengthModifier::toString() const {
+  switch (kind) {
+  case AsChar:
+    return "hh";
+  case AsShort:
+    return "h";
+  case AsLong: // or AsWideChar
+    return "l";
+  case AsLongLong:
+    return "ll";
+  case AsIntMax:
+    return "j";
+  case AsSizeT:
+    return "z";
+  case AsPtrDiff:
+    return "t";
+  case AsLongDouble:
+    return "L";
+  case None:
+    return "";
+  }
+  return NULL;
+}
+
+//===----------------------------------------------------------------------===//
+// Methods on OptionalAmount.
+//===----------------------------------------------------------------------===//
+
+void OptionalAmount::toString(llvm::raw_ostream &os) const {
+  switch (hs) {
+  case Invalid:
+  case NotSpecified:
+    return;
+  case Arg:
+    if (usesPositionalArg())
+      os << ".*" << getPositionalArgIndex() << "$";
+    else
+      os << ".*";
+    break;
+  case Constant:
+    os << "." << amt;
+    break;
+  }
+}
+
+//===----------------------------------------------------------------------===//
 // Methods on FormatSpecifier.
 //===----------------------------------------------------------------------===//
 
@@ -523,52 +621,53 @@ ArgTypeResult FormatSpecifier::getArgType(ASTContext &Ctx) const {
     return ArgTypeResult::Invalid();
 
   if (CS.isIntArg())
-    switch (LM) {
-      case AsLongDouble:
+    switch (LM.getKind()) {
+      case LengthModifier::AsLongDouble:
         return ArgTypeResult::Invalid();
-      case None: return Ctx.IntTy;
-      case AsChar: return Ctx.SignedCharTy;
-      case AsShort: return Ctx.ShortTy;
-      case AsLong: return Ctx.LongTy;
-      case AsLongLong: return Ctx.LongLongTy;
-      case AsIntMax:
+      case LengthModifier::None: return Ctx.IntTy;
+      case LengthModifier::AsChar: return Ctx.SignedCharTy;
+      case LengthModifier::AsShort: return Ctx.ShortTy;
+      case LengthModifier::AsLong: return Ctx.LongTy;
+      case LengthModifier::AsLongLong: return Ctx.LongLongTy;
+      case LengthModifier::AsIntMax:
         // FIXME: Return unknown for now.
         return ArgTypeResult();
-      case AsSizeT: return Ctx.getSizeType();
-      case AsPtrDiff: return Ctx.getPointerDiffType();
+      case LengthModifier::AsSizeT: return Ctx.getSizeType();
+      case LengthModifier::AsPtrDiff: return Ctx.getPointerDiffType();
     }
 
   if (CS.isUIntArg())
-    switch (LM) {
-      case AsLongDouble:
+    switch (LM.getKind()) {
+      case LengthModifier::AsLongDouble:
         return ArgTypeResult::Invalid();
-      case None: return Ctx.UnsignedIntTy;
-      case AsChar: return Ctx.UnsignedCharTy;
-      case AsShort: return Ctx.UnsignedShortTy;
-      case AsLong: return Ctx.UnsignedLongTy;
-      case AsLongLong: return Ctx.UnsignedLongLongTy;
-      case AsIntMax:
+      case LengthModifier::None: return Ctx.UnsignedIntTy;
+      case LengthModifier::AsChar: return Ctx.UnsignedCharTy;
+      case LengthModifier::AsShort: return Ctx.UnsignedShortTy;
+      case LengthModifier::AsLong: return Ctx.UnsignedLongTy;
+      case LengthModifier::AsLongLong: return Ctx.UnsignedLongLongTy;
+      case LengthModifier::AsIntMax:
         // FIXME: Return unknown for now.
         return ArgTypeResult();
-      case AsSizeT:
+      case LengthModifier::AsSizeT:
         // FIXME: How to get the corresponding unsigned
         // version of size_t?
         return ArgTypeResult();
-      case AsPtrDiff:
+      case LengthModifier::AsPtrDiff:
         // FIXME: How to get the corresponding unsigned
         // version of ptrdiff_t?
         return ArgTypeResult();
     }
 
   if (CS.isDoubleArg()) {
-    if (LM == AsLongDouble)
+    if (LM.getKind() == LengthModifier::AsLongDouble)
       return Ctx.LongDoubleTy;
     return Ctx.DoubleTy;
   }
 
   switch (CS.getKind()) {
     case ConversionSpecifier::CStrArg:
-      return ArgTypeResult(LM == AsWideChar ? ArgTypeResult::WCStrTy                                            : ArgTypeResult::CStrTy);
+      return ArgTypeResult(LM.getKind() == LengthModifier::AsWideChar ?
+          ArgTypeResult::WCStrTy : ArgTypeResult::CStrTy);
     case ConversionSpecifier::UnicodeStrArg:
       // FIXME: This appears to be Mac OS X specific.
       return ArgTypeResult::WCStrTy;
@@ -582,3 +681,99 @@ ArgTypeResult FormatSpecifier::getArgType(ASTContext &Ctx) const {
   return ArgTypeResult();
 }
 
+bool FormatSpecifier::fixType(QualType QT) {
+  // Handle strings first (char *, wchar_t *)
+  if (QT->isPointerType() && (QT->getPointeeType()->isAnyCharacterType())) {
+    CS.setKind(ConversionSpecifier::CStrArg);
+
+    // Set the long length modifier for wide characters
+    if (QT->getPointeeType()->isWideCharType())
+      LM.setKind(LengthModifier::AsWideChar);
+
+    return true;
+  }
+
+  // We can only work with builtin types.
+  if (!QT->isBuiltinType())
+    return false;
+
+  // Everything else should be a base type
+  const BuiltinType *BT = QT->getAs<BuiltinType>();
+  // Set length modifier
+  switch (BT->getKind()) {
+  default:
+    break;
+  case BuiltinType::WChar:
+  case BuiltinType::Long:
+  case BuiltinType::ULong:
+    LM.setKind(LengthModifier::AsLong);
+    break;
+
+  case BuiltinType::LongLong:
+  case BuiltinType::ULongLong:
+    LM.setKind(LengthModifier::AsLongLong);
+    break;
+
+  case BuiltinType::LongDouble:
+    LM.setKind(LengthModifier::AsLongDouble);
+    break;
+  }
+
+  // Set conversion specifier and disable any flags which do not apply to it.
+  if (QT->isAnyCharacterType()) {
+    CS.setKind(ConversionSpecifier::IntAsCharArg);
+    Precision.setHowSpecified(OptionalAmount::NotSpecified);
+    HasAlternativeForm = 0;
+    HasLeadingZeroes = 0;
+  }
+  // Test for Floating type first as LongDouble can pass isUnsignedIntegerType
+  else if (QT->isFloatingType()) {
+    CS.setKind(ConversionSpecifier::fArg);
+  }
+  else if (QT->isPointerType()) {
+    CS.setKind(ConversionSpecifier::VoidPtrArg);
+    Precision.setHowSpecified(OptionalAmount::NotSpecified);
+    HasAlternativeForm = 0;
+    HasLeadingZeroes = 0;
+  }
+  else if (QT->isSignedIntegerType()) {
+    CS.setKind(ConversionSpecifier::dArg);
+    HasAlternativeForm = 0;
+  }
+  else if (QT->isUnsignedIntegerType) {
+    CS.setKind(ConversionSpecifier::uArg);
+    HasAlternativeForm = 0;
+  }
+  else {
+    return false;
+  }
+
+  return true;
+}
+
+void FormatSpecifier::toString(llvm::raw_ostream &os) const {
+  // Whilst some features have no defined order, we are using the order
+  // appearing in the C99 standard (ISO/IEC 9899:1999 (E) ¤7.19.6.1)
+  os << "%";
+
+  // Positional args
+  if (usesPositionalArg()) {
+    os << getPositionalArgIndex() << "$";
+  }
+
+  // Conversion flags
+  if (IsLeftJustified)    os << "-";
+  if (HasPlusPrefix)      os << "+";
+  if (HasSpacePrefix)     os << " ";
+  if (HasAlternativeForm) os << "#";
+  if (HasLeadingZeroes)   os << "0";
+
+  // Minimum field width
+  FieldWidth.toString(os);
+  // Precision
+  Precision.toString(os);
+  // Length modifier
+  os << LM.toString();
+  // Conversion specifier
+  os << CS.toString();
+}
diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 6e54dab113..dbc33430fb 100644
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -26,6 +26,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/raw_ostream.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include <limits>
@@ -1404,11 +1405,32 @@ CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier
         if (ATR.matchesType(S.Context, ICE->getSubExpr()->getType()))
           return true;
 
-    S.Diag(getLocationOfByte(CS.getStart()),
-           diag::warn_printf_conversion_argument_type_mismatch)
-      << ATR.getRepresentativeType(S.Context) << Ex->getType()
-      << getFormatSpecifierRange(startSpecifier, specifierLen)
-      << Ex->getSourceRange();
+    // We may be able to offer a FixItHint if it is a supported type.
+    FormatSpecifier fixedFS = FS;
+    bool success = fixedFS.fixType(Ex->getType());
+
+    if (success) {
+      // Get the fix string from the fixed format specifier
+      llvm::SmallString<128> buf;
+      llvm::raw_svector_ostream os(buf);
+      fixedFS.toString(os);
+
+      S.Diag(getLocationOfByte(CS.getStart()),
+          diag::warn_printf_conversion_argument_type_mismatch)
+        << ATR.getRepresentativeType(S.Context) << Ex->getType()
+        << getFormatSpecifierRange(startSpecifier, specifierLen)
+        << Ex->getSourceRange()
+        << FixItHint::CreateReplacement(
+            getFormatSpecifierRange(startSpecifier, specifierLen),
+            os.str());
+    }
+    else {
+      S.Diag(getLocationOfByte(CS.getStart()),
+             diag::warn_printf_conversion_argument_type_mismatch)
+        << ATR.getRepresentativeType(S.Context) << Ex->getType()
+        << getFormatSpecifierRange(startSpecifier, specifierLen)
+        << Ex->getSourceRange();
+    }
   }
 
   return true;
diff --git a/test/Sema/format-strings-fixit.c b/test/Sema/format-strings-fixit.c
new file mode 100644
index 0000000000..ba38973049
--- /dev/null
+++ b/test/Sema/format-strings-fixit.c
@@ -0,0 +1,20 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -pedantic -Wall -fixit %t || true
+// RUN: %clang_cc1 -fsyntax-only -pedantic -Wall -Werror %t
+
+/* This is a test of the various code modification hints that are
+   provided as part of warning or extension diagnostics. All of the
+   warnings will be fixed by -fixit, and the resulting file should
+   compile cleanly with -Werror -pedantic. */
+
+int printf(char const *, ...);
+
+void test() {
+  printf("%0s", (int) 123);
+  printf("abc%f", "testing testing 123");
+  printf("%u", (long) -12);
+  printf("%+.2d", (unsigned long long) 123456);
+  printf("%1d", (long double) 1.23);
+  printf("%Ld", (long double) -4.56);
+  printf("%1$f:%2$.*3$f:%4$.*3$f\n", 1, 2, 3, 4);
+}