Bug 7377: printf checking fails to flag some undefined behavior

http://llvm.org/bugs/show_bug.cgi?id=7377

Updated format string highlighting and fixits to take advantage of the new CharSourceRange class.
- Change HighlightRange to allow highlighting whitespace only in a CharSourceRange (for warnings about the ' ' (space) flag)
- Change format specifier range helper function to allow for half-open ranges (+1 to end)
- Enabled previously failing tests (FIXMEs/XFAILs removed)
- Small fixes and additions to format string test cases

M       test/Sema/format-strings.c
M       test/Sema/format-strings-fixit.c
M       lib/Frontend/TextDiagnosticPrinter.cpp
M       lib/Sema/SemaChecking.cpp

llvm-svn: 106480

clang/lib/Frontend/TextDiagnosticPrinter.cpp

@@ -123,21 +123,24 @@ void TextDiagnosticPrinter::HighlightRange(const CharSourceRange &R,
 
   assert(StartColNo <= EndColNo && "Invalid range!");
 
-  // Pick the first non-whitespace column.
-  while (StartColNo < SourceLine.size() &&
-         (SourceLine[StartColNo] == ' ' || SourceLine[StartColNo] == '\t'))
-    ++StartColNo;
+  // Check that a token range does not highlight only whitespace.
+  if (R.isTokenRange()) {
+    // Pick the first non-whitespace column.
+    while (StartColNo < SourceLine.size() &&
+           (SourceLine[StartColNo] == ' ' || SourceLine[StartColNo] == '\t'))
+      ++StartColNo;
 
-  // Pick the last non-whitespace column.
-  if (EndColNo > SourceLine.size())
-    EndColNo = SourceLine.size();
-  while (EndColNo-1 &&
-         (SourceLine[EndColNo-1] == ' ' || SourceLine[EndColNo-1] == '\t'))
-    --EndColNo;
+    // Pick the last non-whitespace column.
+    if (EndColNo > SourceLine.size())
+      EndColNo = SourceLine.size();
+    while (EndColNo-1 &&
+           (SourceLine[EndColNo-1] == ' ' || SourceLine[EndColNo-1] == '\t'))
+      --EndColNo;
 
-  // If the start/end passed each other, then we are trying to highlight a range
-  // that just exists in whitespace, which must be some sort of other bug.
-  assert(StartColNo <= EndColNo && "Trying to highlight whitespace??");
+    // If the start/end passed each other, then we are trying to highlight a range
+    // that just exists in whitespace, which must be some sort of other bug.
+    assert(StartColNo <= EndColNo && "Trying to highlight whitespace??");
+  }
 
   // Fill the range with ~'s.
   for (unsigned i = StartColNo; i < EndColNo; ++i)

clang/lib/Sema/SemaChecking.cpp

@@ -1194,8 +1194,8 @@ public:
                              unsigned specifierLen);
 private:
   SourceRange getFormatStringRange();
-  SourceRange getFormatSpecifierRange(const char *startSpecifier,
-                                      unsigned specifierLen);
+  CharSourceRange getFormatSpecifierRange(const char *startSpecifier,
+                                          unsigned specifierLen);
   SourceLocation getLocationOfByte(const char *x);
 
   bool HandleAmount(const analyze_printf::OptionalAmount &Amt, unsigned k,
@@ -1220,10 +1220,15 @@ SourceRange CheckPrintfHandler::getFormatStringRange() {
   return OrigFormatExpr->getSourceRange();
 }
 
-SourceRange CheckPrintfHandler::
+CharSourceRange CheckPrintfHandler::
 getFormatSpecifierRange(const char *startSpecifier, unsigned specifierLen) {
-  return SourceRange(getLocationOfByte(startSpecifier),
-                     getLocationOfByte(startSpecifier+specifierLen-1));
+  SourceLocation Start = getLocationOfByte(startSpecifier);
+  SourceLocation End   = getLocationOfByte(startSpecifier + specifierLen - 1);
+
+  // Advance the end SourceLocation by one due to half-open ranges.
+  End = End.getFileLocWithOffset(1);
+
+  return CharSourceRange::getCharRange(Start, End);
 }
 
 SourceLocation CheckPrintfHandler::getLocationOfByte(const char *x) {
@@ -1451,7 +1456,7 @@ CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier
 
   // Check for invalid use of field width
   if (!FS.hasValidFieldWidth()) {
-    HandleInvalidAmount(FS, FS.getFieldWidth(), /* field width */ 1,
+    HandleInvalidAmount(FS, FS.getFieldWidth(), /* field width */ 0,
         startSpecifier, specifierLen);
   }
 
@@ -1466,21 +1471,17 @@ CheckPrintfHandler::HandleFormatSpecifier(const analyze_printf::FormatSpecifier
     HandleFlag(FS, FS.hasLeadingZeros(), startSpecifier, specifierLen);
   if (!FS.hasValidPlusPrefix())
     HandleFlag(FS, FS.hasPlusPrefix(), startSpecifier, specifierLen);
-  // FIXME: the following lines are disabled due to clang assertions on
-  // highlights containing spaces.
-  // if (!FS.hasValidSpacePrefix())
-  //   HandleFlag(FS, FS.hasSpacePrefix(), startSpecifier, specifierLen);
+  if (!FS.hasValidSpacePrefix())
+    HandleFlag(FS, FS.hasSpacePrefix(), startSpecifier, specifierLen);
   if (!FS.hasValidAlternativeForm())
     HandleFlag(FS, FS.hasAlternativeForm(), startSpecifier, specifierLen);
   if (!FS.hasValidLeftJustified())
     HandleFlag(FS, FS.isLeftJustified(), startSpecifier, specifierLen);
 
   // Check that flags are not ignored by another flag
-  // FIXME: the following lines are disabled due to clang assertions on
-  // highlights containing spaces.
-  //if (FS.hasSpacePrefix() && FS.hasPlusPrefix()) // ' ' ignored by '+'
-  //  HandleIgnoredFlag(FS, FS.hasSpacePrefix(), FS.hasPlusPrefix(),
-  //      startSpecifier, specifierLen);
+  if (FS.hasSpacePrefix() && FS.hasPlusPrefix()) // ' ' ignored by '+'
+    HandleIgnoredFlag(FS, FS.hasSpacePrefix(), FS.hasPlusPrefix(),
+        startSpecifier, specifierLen);
   if (FS.hasLeadingZeros() && FS.isLeftJustified()) // '0' ignored by '-'
     HandleIgnoredFlag(FS, FS.hasLeadingZeros(), FS.isLeftJustified(),
             startSpecifier, specifierLen);

clang/test/Sema/format-strings-fixit.c

@@ -1,9 +1,6 @@
 // RUN: cp %s %t
 // RUN: %clang_cc1 -pedantic -Wall -fixit %t || true
 // RUN: %clang_cc1 -fsyntax-only -pedantic -Wall -Werror %t
-// XFAIL: *
-// FIXME: Some of these tests currently fail due to a bug in the HighlightRange
-// function in lib/Frontend/TextDiagnosticPrinter.cpp.
 
 /* This is a test of the various code modification hints that are
    provided as part of warning or extension diagnostics. All of the
@@ -26,9 +23,10 @@ void test() {
   printf("%1d", (long double) 1.23);
 
   // Flag handling
-  printf("%0+s", (unsigned) 31337); // flags should stay
-  printf("%0f", "test"); // flag should be removed
+  printf("%0+s", (unsigned) 31337); // 0 flag should stay
   printf("%#p", (void *) 0);
+  printf("% +f", 1.23); // + flag should stay
+  printf("%0-f", 1.23); // - flag should stay
 
   // Positional arguments
   printf("%1$f:%2$.*3$f:%4$.*3$f\n", 1, 2, 3, 4);

clang/test/Sema/format-strings.c

@@ -1,7 +1,4 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral %s
-// XFAIL: *
-// FIXME: temporarily marking as expected fail until whitespace highlighting
-// is fixed.
 
 #include <stdarg.h>
 typedef __typeof(sizeof(int)) size_t;
@@ -277,7 +274,8 @@ void bug7377_bad_length_mod_usage() {
 
   // Bad optional amount use
   printf("%.2c", 'a'); // expected-warning{{precision used with 'c' conversion specifier, resulting in undefined behavior}}
-  printf("%1n", (void *) 0); // expected-warning{{precision used with 'n' conversion specifier, resulting in undefined behavior}} expected-warning{{use of '%n' in format string discouraged (potentially insecure)}}
+  printf("%1n", (void *) 0); // expected-warning{{field width used with 'n' conversion specifier, resulting in undefined behavior}} expected-warning{{use of '%n' in format string discouraged (potentially insecure)}}
+  printf("%.9n", (void *) 0); // expected-warning{{precision used with 'n' conversion specifier, resulting in undefined behavior}} expected-warning{{use of '%n' in format string discouraged (potentially insecure)}}
 
   // Ignored flags
   printf("% +f", 1.23); // expected-warning{{flag ' ' is ignored when flag '+' is present}}