[cfe-commits] r105680 - in /cfe/trunk: include/clang/Analysis/Analyses/PrintfFormatString.h lib/Analysis/PrintfFormatString.cpp lib/Sema/SemaChecking.cpp test/Sema/format-strings-fixit.c
Tom Care
tcare at apple.com
Tue Jun 8 21:11:11 PDT 2010
Author: tcare
Date: Tue Jun 8 23:11:11 2010
New Revision: 105680
URL: http://llvm.org/viewvc/llvm-project?rev=105680&view=rev
Log:
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
Added:
cfe/trunk/test/Sema/format-strings-fixit.c
Modified:
cfe/trunk/include/clang/Analysis/Analyses/PrintfFormatString.h
cfe/trunk/lib/Analysis/PrintfFormatString.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
Modified: cfe/trunk/include/clang/Analysis/Analyses/PrintfFormatString.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/PrintfFormatString.h?rev=105680&r1=105679&r2=105680&view=diff
==============================================================================
--- cfe/trunk/include/clang/Analysis/Analyses/PrintfFormatString.h (original)
+++ cfe/trunk/include/clang/Analysis/Analyses/PrintfFormatString.h Tue Jun 8 23:11:11 2010
@@ -124,45 +124,85 @@
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(bool b = true)
- : start(0), hs(b ? NotSpecified : Invalid), amt(0) {}
+ OptionalAmount(HowSpecified howSpecified,
+ unsigned amount,
+ const char *amountStart,
+ bool usesPositionalArg)
+ : start(amountStart), hs(howSpecified), amt(amount),
+ UsesPositionalArg(usesPositionalArg) {}
+
+ 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 @@
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 @@
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 @@
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 @@
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 };
Modified: cfe/trunk/lib/Analysis/PrintfFormatString.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/PrintfFormatString.cpp?rev=105680&r1=105679&r2=105680&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/PrintfFormatString.cpp (original)
+++ cfe/trunk/lib/Analysis/PrintfFormatString.cpp Tue Jun 8 23:11:11 2010
@@ -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 @@
}
if (hasDigits)
- return OptionalAmount(OptionalAmount::Constant, accumulator, Beg);
+ return OptionalAmount(OptionalAmount::Constant, accumulator, Beg, 0);
break;
}
@@ -92,7 +96,7 @@
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 @@
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 @@
Beg = ++I;
return OptionalAmount(OptionalAmount::Arg, Amt.getConstantAmount() - 1,
- Tmp);
+ Tmp, 1);
}
H.HandleInvalidPosition(Beg, I - Beg, p);
@@ -304,24 +310,28 @@
}
// 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 @@
}
//===----------------------------------------------------------------------===//
+// 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 @@
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 @@
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();
+}
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=105680&r1=105679&r2=105680&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Tue Jun 8 23:11:11 2010
@@ -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 @@
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;
Added: cfe/trunk/test/Sema/format-strings-fixit.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/format-strings-fixit.c?rev=105680&view=auto
==============================================================================
--- cfe/trunk/test/Sema/format-strings-fixit.c (added)
+++ cfe/trunk/test/Sema/format-strings-fixit.c Tue Jun 8 23:11:11 2010
@@ -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);
+}
More information about the cfe-commits
mailing list