Make -Wformat fix-its preserve original conversion specifiers.

This commit makes PrintfSpecifier::fixType() and ScanfSpecifier::fixType()
only fix a conversion specification enough that Clang wouldn't warn about it,
as opposed to always changing it to use the "canonical" conversion specifier.
(PR11975)

This preserves the user's choice of conversion specifier in cases like:

printf("%a", (long double)1);
where we previously suggested "%Lf", we now suggest "%La"

printf("%x", (long)1);
where we previously suggested "%ld", we now suggest "%lx".

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

diff --git a/include/clang/Analysis/Analyses/FormatString.h b/include/clang/Analysis/Analyses/FormatString.h
index 49221bbc9b1157febaa1de79bf66bedb43c07a91..f3fe32474c760f1f027bc555c51f52cdedc4064d 100644 (file)
--- a/include/clang/Analysis/Analyses/FormatString.h
+++ b/include/clang/Analysis/Analyses/FormatString.h
@@ -467,10 +467,11 @@ public:
   const OptionalFlag &hasSpacePrefix() const { return HasSpacePrefix; }
   bool usesPositionalArg() const { return 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, const LangOptions &LangOpt);
+  /// 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, const LangOptions &LangOpt, ASTContext &Ctx,
+               bool IsObjCLiteral);
 
   void toString(raw_ostream &os) const;
 
@@ -567,7 +568,7 @@ public:
 
   ScanfArgTypeResult getArgType(ASTContext &Ctx) const;
 
-  bool fixType(QualType QT, const LangOptions &LangOpt);
+  bool fixType(QualType QT, const LangOptions &LangOpt, ASTContext &Ctx);
 
   void toString(raw_ostream &os) const;
 

diff --git a/lib/Analysis/PrintfFormatString.cpp b/lib/Analysis/PrintfFormatString.cpp
index 6da37fc44aba27588f98695bfb396610ddfe1be8..ff0174e3c3e57d091c36ae5e7b9981aa61b91a51 100644 (file)
--- a/lib/Analysis/PrintfFormatString.cpp
+++ b/lib/Analysis/PrintfFormatString.cpp
@@ -336,7 +336,8 @@ ArgTypeResult PrintfSpecifier::getArgType(ASTContext &Ctx,
   return ArgTypeResult();
 }
 
-bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt) {
+bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt,
+                              ASTContext &Ctx, bool IsObjCLiteral) {
   // Handle strings first (char *, wchar_t *)
   if (QT->isPointerType() && (QT->getPointeeType()->isAnyCharacterType())) {
     CS.setKind(ConversionSpecifier::sArg);
@@ -432,6 +433,11 @@ bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt) {
     }
   }
 
+  // If fixing the length modifier was enough, we are done.
+  const analyze_printf::ArgTypeResult &ATR = getArgType(Ctx, IsObjCLiteral);
+  if (hasValidLengthModifier() && ATR.isValid() && ATR.matchesType(Ctx, QT))
+    return true;
+
   // Set conversion specifier and disable any flags which do not apply to it.
   // Let typedefs to char fall through to int, as %c is silly for uint8_t.
   if (isa<TypedefType>(QT) && QT->isAnyCharacterType()) {
@@ -451,9 +457,7 @@ bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt) {
     HasAlternativeForm = 0;
   }
   else if (QT->isUnsignedIntegerType()) {
-    // Preserve the original formatting, e.g. 'X', 'o'.
-    if (!cast<PrintfConversionSpecifier>(CS).isUIntArg())
-      CS.setKind(ConversionSpecifier::uArg);
+    CS.setKind(ConversionSpecifier::uArg);
     HasAlternativeForm = 0;
     HasPlusPrefix = 0;
   } else {

diff --git a/lib/Analysis/ScanfFormatString.cpp b/lib/Analysis/ScanfFormatString.cpp
index c1cdef8632792a2b0cb8ddd3e8d71ea6e26e5232..5990a56c35c6c000e17a65268f0c9752b043b2f2 100644 (file)
--- a/lib/Analysis/ScanfFormatString.cpp
+++ b/lib/Analysis/ScanfFormatString.cpp
@@ -307,8 +307,8 @@ ScanfArgTypeResult ScanfSpecifier::getArgType(ASTContext &Ctx) const {
   return ScanfArgTypeResult();
 }
 
-bool ScanfSpecifier::fixType(QualType QT, const LangOptions &LangOpt)
-{
+bool ScanfSpecifier::fixType(QualType QT, const LangOptions &LangOpt,
+                             ASTContext &Ctx) {
   if (!QT->isPointerType())
     return false;
 
@@ -390,17 +390,19 @@ bool ScanfSpecifier::fixType(QualType QT, const LangOptions &LangOpt)
     }
   }
 
+  // If fixing the length modifier was enough, we are done.
+  const analyze_scanf::ScanfArgTypeResult &ATR = getArgType(Ctx);
+  if (hasValidLengthModifier() && ATR.isValid() && ATR.matchesType(Ctx, QT))
+    return true;
+
   // Figure out the conversion specifier.
   if (PT->isRealFloatingType())
     CS.setKind(ConversionSpecifier::fArg);
   else if (PT->isSignedIntegerType())
     CS.setKind(ConversionSpecifier::dArg);
-  else if (PT->isUnsignedIntegerType()) {
-    // Preserve the original formatting, e.g. 'X', 'o'.
-    if (!CS.isUIntArg()) {
-      CS.setKind(ConversionSpecifier::uArg);
-    }
-  } else
+  else if (PT->isUnsignedIntegerType())
+    CS.setKind(ConversionSpecifier::uArg);
+  else
     llvm_unreachable("Unexpected type");
 
   return true;

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 3393cf73f1c00378acdfc78342e67f4a7a142309..1d75ef6e6f5eac2136c4cf01ae8ccd4ee5ef6038 100644 (file)
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -2181,7 +2181,8 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier
 
     // We may be able to offer a FixItHint if it is a supported type.
     PrintfSpecifier fixedFS = FS;
-    bool success = fixedFS.fixType(Ex->getType(), S.getLangOptions());
+    bool success = fixedFS.fixType(Ex->getType(), S.getLangOptions(),
+                                   S.Context, IsObjCLiteral);
 
     if (success) {
       // Get the fix string from the fixed format specifier
@@ -2340,7 +2341,8 @@ bool CheckScanfHandler::HandleScanfSpecifier(
   const analyze_scanf::ScanfArgTypeResult &ATR = FS.getArgType(S.Context);
   if (ATR.isValid() && !ATR.matchesType(S.Context, Ex->getType())) {
     ScanfSpecifier fixedFS = FS;
-    bool success = fixedFS.fixType(Ex->getType(), S.getLangOptions());
+    bool success = fixedFS.fixType(Ex->getType(), S.getLangOptions(),
+                                   S.Context);
 
     if (success) {
       // Get the fix string from the fixed format specifier.

diff --git a/test/Sema/format-strings-fixit.c b/test/Sema/format-strings-fixit.c
index 4fb6d75d76d5780c36cc5287f9c0922d610734d6..80b1be05787bcd4b6b03df2048a8c5da536a8353 100644 (file)
--- a/test/Sema/format-strings-fixit.c
+++ b/test/Sema/format-strings-fixit.c
@@ -48,7 +48,7 @@ void test() {
   printf("%hhs", "foo");
   printf("%1$zp", (void *)0);
 
-  // Perserve the original formatting for unsigned integers.
+  // Preserve the original formatting for unsigned integers.
   unsigned long val = 42;
   printf("%X", val);
 
@@ -60,6 +60,21 @@ void test() {
 
   // string
   printf("%ld", "foo");
+
+  // Preserve the original choice of conversion specifier.
+  printf("%o", (long) 42);
+  printf("%u", (long) 42);
+  printf("%x", (long) 42);
+  printf("%X", (long) 42);
+  printf("%i", (unsigned long) 42);
+  printf("%d", (unsigned long) 42);
+  printf("%F", (long double) 42);
+  printf("%e", (long double) 42);
+  printf("%E", (long double) 42);
+  printf("%g", (long double) 42);
+  printf("%G", (long double) 42);
+  printf("%a", (long double) 42);
+  printf("%A", (long double) 42);
 }
 
 int scanf(char const *, ...);
@@ -101,20 +116,30 @@ void test2() {
   scanf("%f", &uIntmaxVar);
   scanf("%f", &ptrdiffVar);
 
-  // Perserve the original formatting for unsigned integers.
-  scanf("%o", &uLongVar);
-  scanf("%x", &uLongVar);
-  scanf("%X", &uLongVar);
+  // Preserve the original formatting.
+  scanf("%o", &longVar);
+  scanf("%u", &longVar);
+  scanf("%x", &longVar);
+  scanf("%X", &longVar);
+  scanf("%i", &uLongVar);
+  scanf("%d", &uLongVar);
+  scanf("%F", &longDoubleVar);
+  scanf("%e", &longDoubleVar);
+  scanf("%E", &longDoubleVar);
+  scanf("%g", &longDoubleVar);
+  scanf("%G", &longDoubleVar);
+  scanf("%a", &longDoubleVar);
+  scanf("%A", &longDoubleVar);
 }
 
-// Validate the fixes...
+// Validate the fixes.
 // CHECK: printf("%d", (int) 123);
 // CHECK: printf("abc%s", "testing testing 123");
-// CHECK: printf("%ld", (long) -12);
+// CHECK: printf("%lu", (long) -12);
 // CHECK: printf("%d", 123);
 // CHECK: printf("%s\n", "x");
 // CHECK: printf("%f\n", 1.23);
-// CHECK: printf("%.2llu", (unsigned long long) 123456);
+// CHECK: printf("%+.2lld", (unsigned long long) 123456);
 // CHECK: printf("%1Lf", (long double) 1.23);
 // CHECK: printf("%0u", (unsigned) 31337);
 // CHECK: printf("%p", (void *) 0);
@@ -132,6 +157,19 @@ void test2() {
 // CHECK: printf("%ju", (uintmax_t) 42);
 // CHECK: printf("%td", (ptrdiff_t) 42);
 // CHECK: printf("%s", "foo");
+// CHECK: printf("%lo", (long) 42);
+// CHECK: printf("%lu", (long) 42);
+// CHECK: printf("%lx", (long) 42);
+// CHECK: printf("%lX", (long) 42);
+// CHECK: printf("%li", (unsigned long) 42);
+// CHECK: printf("%ld", (unsigned long) 42);
+// CHECK: printf("%LF", (long double) 42);
+// CHECK: printf("%Le", (long double) 42);
+// CHECK: printf("%LE", (long double) 42);
+// CHECK: printf("%Lg", (long double) 42);
+// CHECK: printf("%LG", (long double) 42);
+// CHECK: printf("%La", (long double) 42);
+// CHECK: printf("%LA", (long double) 42);
 
 // CHECK: scanf("%s", str);
 // CHECK: scanf("%hd", &shortVar);
@@ -149,6 +187,16 @@ void test2() {
 // CHECK: scanf("%jd", &intmaxVar);
 // CHECK: scanf("%ju", &uIntmaxVar);
 // CHECK: scanf("%td", &ptrdiffVar);
-// CHECK: scanf("%lo", &uLongVar);
-// CHECK: scanf("%lx", &uLongVar);
-// CHECK: scanf("%lX", &uLongVar);
+// CHECK: scanf("%lo", &longVar);
+// CHECK: scanf("%lu", &longVar);
+// CHECK: scanf("%lx", &longVar);
+// CHECK: scanf("%lX", &longVar);
+// CHECK: scanf("%li", &uLongVar);
+// CHECK: scanf("%ld", &uLongVar);
+// CHECK: scanf("%LF", &longDoubleVar);
+// CHECK: scanf("%Le", &longDoubleVar);
+// CHECK: scanf("%LE", &longDoubleVar);
+// CHECK: scanf("%Lg", &longDoubleVar);
+// CHECK: scanf("%LG", &longDoubleVar);
+// CHECK: scanf("%La", &longDoubleVar);
+// CHECK: scanf("%LA", &longDoubleVar);