Add compiler-specific "examine printf format" attributes to printfs.

Functions that take a printf-style format get a new annotation, which
produces a bunch of compiler warnings when you use printf impoperly.
This change adds the annotations and fixes the warnings.

We now must use PRId64 for 64-bit numbers and the PRIsz for size_t.

Review URL: http://codereview.chromium.org/339059

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@32600 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 103 insertions, 43 deletions

diff --git a/base/compiler_specific.h b/base/compiler_specific.h
index 50dc6f3..23b9f124 100644
--- a/base/compiler_specific.h
+++ b/base/compiler_specific.h
@@ -67,11 +67,32 @@
 
 
 #if defined(COMPILER_GCC)
+
 #define ALLOW_UNUSED __attribute__((unused))
 #define WARN_UNUSED_RESULT __attribute__((warn_unused_result))
+
+// Tell the compiler a function is using a printf-style format string.
+// |format_param| is the one-based index of the format string parameter;
+// |dots_param| is the one-based index of the "..." parameter.
+// For v*printf functions (which take a va_list), pass 0 for dots_param.
+// (This is undocumented but matches what the system C headers do.)
+#define PRINTF_FORMAT(format_param, dots_param) \
+    __attribute__((format(printf, format_param, dots_param)))
+
+// WPRINTF_FORMAT is the same, but for wide format strings.
+// This doesn't appear to yet be implemented.
+// See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38308 .
+#define WPRINTF_FORMAT(format_param, dots_param)
+// If available, it would look like:
+//   __attribute__((format(wprintf, format_param, dots_param)))
+
 #else  // Not GCC
+
 #define ALLOW_UNUSED
 #define WARN_UNUSED_RESULT
+#define PRINTF_FORMAT(x, y)
+#define WPRINTF_FORMAT(x, y)
+
 #endif
 
 #endif  // BASE_COMPILER_SPECIFIC_H_
diff --git a/base/format_macros.h b/base/format_macros.h
index 383579f..d218f48 100644
--- a/base/format_macros.h
+++ b/base/format_macros.h
@@ -5,14 +5,21 @@
 #ifndef BASE_FORMAT_MACROS_H_
 #define BASE_FORMAT_MACROS_H_
 
-// This file defines the C99 format macros for 64-bit values. If you wish to
-// print a 64-bit value in a portable way do:
+// This file defines the format macros for some integer types.
+
+// To print a 64-bit value in a portable way:
 //   int64_t value;
 //   printf("xyz:%" PRId64, value);
+// The "d" in the macro corresponds to %d; you can also use PRIu64 etc.
 //
 // For wide strings, prepend "Wide" to the macro:
 //   int64_t value;
 //   StringPrintf(L"xyz: %" WidePRId64, value);
+//
+// To print a size_t value in a portable way:
+//   size_t size;
+//   printf("xyz: %" PRIuS, size);
+// The "u" in the macro corresponds to %u, and S is for "size".
 
 #include "build/build_config.h"
 
@@ -35,6 +42,8 @@
 #define WidePRIu64 PRIu64
 #define WidePRIx64 PRIx64
 
+#define PRIuS "zu"
+
 #else  // OS_WIN
 
 #if !defined(PRId64)
@@ -53,6 +62,8 @@
 #define WidePRIu64 L"I64u"
 #define WidePRIx64 L"I64x"
 
+#define PRIuS "Iu"
+
 #endif
 
 #endif  // !BASE_FORMAT_MACROS_H_
diff --git a/base/histogram.cc b/base/histogram.cc
index 3d2cd9e..55af96d 100644
--- a/base/histogram.cc
+++ b/base/histogram.cc
@@ -124,7 +124,10 @@ void Histogram::WriteAscii(bool graph_it, const std::string& newline,
     if (!current && !PrintEmptyBucket(i))
       continue;
     remaining -= current;
-    StringAppendF(output, "%#*s ", print_width, GetAsciiBucketRange(i).c_str());
+    std::string range = GetAsciiBucketRange(i);
+    output->append(range);
+    for (size_t j = 0; range.size() + j < print_width + 1; ++j)
+      output->push_back(' ');
     if (0 == current && i < bucket_count() - 1 && 0 == snapshot.counts(i + 1)) {
       while (i < bucket_count() - 1 && 0 == snapshot.counts(i + 1))
         ++i;
@@ -287,7 +290,7 @@ void Histogram::WriteAsciiHeader(const SampleSet& snapshot,
                                  Count sample_count,
                                  std::string* output) const {
   StringAppendF(output,
-                "Histogram: %s recorded %ld samples",
+                "Histogram: %s recorded %d samples",
                 histogram_name().c_str(),
                 sample_count);
   if (0 == sample_count) {
diff --git a/base/i18n/number_formatting.cc b/base/i18n/number_formatting.cc
index fef1b7d..7a69294 100644
--- a/base/i18n/number_formatting.cc
+++ b/base/i18n/number_formatting.cc
@@ -4,6 +4,7 @@
 
 #include "base/i18n/number_formatting.h"
 
+#include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/singleton.h"
 #include "base/string_util.h"
@@ -37,7 +38,7 @@ string16 FormatNumber(int64 number) {
 
   if (!number_format) {
     // As a fallback, just return the raw number in a string.
-    return UTF8ToUTF16(StringPrintf("%lld", number));
+    return UTF8ToUTF16(StringPrintf("%" PRId64, number));
   }
   icu::UnicodeString ustr;
   number_format->format(number, ustr);
diff --git a/base/process_util.h b/base/process_util.h
index a6f63ac..cd26c2a 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -43,12 +43,12 @@ struct ProcessEntry {
 };
 
 struct IoCounters {
-  unsigned long long ReadOperationCount;
-  unsigned long long WriteOperationCount;
-  unsigned long long OtherOperationCount;
-  unsigned long long ReadTransferCount;
-  unsigned long long WriteTransferCount;
-  unsigned long long OtherTransferCount;
+  uint64_t ReadOperationCount;
+  uint64_t WriteOperationCount;
+  uint64_t OtherOperationCount;
+  uint64_t ReadTransferCount;
+  uint64_t WriteTransferCount;
+  uint64_t OtherTransferCount;
 };
 
 #include "base/file_descriptor_shuffle.h"
diff --git a/base/string_util.h b/base/string_util.h
index 89caf4f..6d4a4d9 100644
--- a/base/string_util.h
+++ b/base/string_util.h
@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/string16.h"
 #include "base/string_piece.h"  // For implicit conversions.
 
@@ -42,17 +43,22 @@ int strncasecmp(const char* s1, const char* s2, size_t count);
 // Wrapper for vsnprintf that always null-terminates and always returns the
 // number of characters that would be in an untruncated formatted
 // string, even when truncation occurs.
-int vsnprintf(char* buffer, size_t size, const char* format, va_list arguments);
+int vsnprintf(char* buffer, size_t size, const char* format, va_list arguments)
+    PRINTF_FORMAT(3, 0);
 
 // vswprintf always null-terminates, but when truncation occurs, it will either
 // return -1 or the number of characters that would be in an untruncated
 // formatted string.  The actual return value depends on the underlying
 // C library's vswprintf implementation.
 int vswprintf(wchar_t* buffer, size_t size,
-              const wchar_t* format, va_list arguments);
+              const wchar_t* format, va_list arguments) WPRINTF_FORMAT(3, 0);
 
 // Some of these implementations need to be inlined.
 
+// We separate the declaration from the implementation of this inline
+// function just so the PRINTF_FORMAT works.
+inline int snprintf(char* buffer, size_t size, const char* format, ...)
+    PRINTF_FORMAT(3, 4);
 inline int snprintf(char* buffer, size_t size, const char* format, ...) {
   va_list arguments;
   va_start(arguments, format);
@@ -61,6 +67,10 @@ inline int snprintf(char* buffer, size_t size, const char* format, ...) {
   return result;
 }
 
+// We separate the declaration from the implementation of this inline
+// function just so the WPRINTF_FORMAT works.
+inline int swprintf(wchar_t* buffer, size_t size, const wchar_t* format, ...)
+    WPRINTF_FORMAT(3, 4);
 inline int swprintf(wchar_t* buffer, size_t size, const wchar_t* format, ...) {
   va_list arguments;
   va_start(arguments, format);
@@ -438,22 +448,28 @@ double StringToDouble(const std::string& value);
 double StringToDouble(const string16& value);
 
 // Return a C++ string given printf-like input.
-std::string StringPrintf(const char* format, ...);
-std::wstring StringPrintf(const wchar_t* format, ...);
+std::string StringPrintf(const char* format, ...) PRINTF_FORMAT(1, 2);
+std::wstring StringPrintf(const wchar_t* format, ...) WPRINTF_FORMAT(1, 2);
 
 // Store result into a supplied string and return it
-const std::string& SStringPrintf(std::string* dst, const char* format, ...);
+const std::string& SStringPrintf(std::string* dst, const char* format, ...)
+    PRINTF_FORMAT(2, 3);
 const std::wstring& SStringPrintf(std::wstring* dst,
-                                  const wchar_t* format, ...);
+                                  const wchar_t* format, ...)
+    WPRINTF_FORMAT(2, 3);
 
 // Append result to a supplied string
-void StringAppendF(std::string* dst, const char* format, ...);
-void StringAppendF(std::wstring* dst, const wchar_t* format, ...);
+void StringAppendF(std::string* dst, const char* format, ...)
+    PRINTF_FORMAT(2, 3);
+void StringAppendF(std::wstring* dst, const wchar_t* format, ...)
+    WPRINTF_FORMAT(2, 3);
 
 // Lower-level routine that takes a va_list and appends to a specified
 // string.  All other routines are just convenience wrappers around it.
-void StringAppendV(std::string* dst, const char* format, va_list ap);
-void StringAppendV(std::wstring* dst, const wchar_t* format, va_list ap);
+void StringAppendV(std::string* dst, const char* format, va_list ap)
+    PRINTF_FORMAT(2, 0);
+void StringAppendV(std::wstring* dst, const wchar_t* format, va_list ap)
+    WPRINTF_FORMAT(2, 0);
 
 // This is mpcomplete's pattern for saving a string copy when dealing with
 // a function that writes results into a wchar_t[] and wanting the result to
diff --git a/base/string_util_unittest.cc b/base/string_util_unittest.cc
index d691003..c586ff4 100644
--- a/base/string_util_unittest.cc
+++ b/base/string_util_unittest.cc
@@ -827,9 +827,7 @@ TEST(StringUtilTest, VAList) {
   VariableArgsFunc("%d %d %s %lf", 45, 92, "This is interesting", 9.21);
 }
 
-TEST(StringUtilTest, StringPrintfEmptyFormat) {
-  const char* empty = "";
-  EXPECT_EQ("", StringPrintf(empty));
+TEST(StringUtilTest, StringPrintfEmpty) {
   EXPECT_EQ("", StringPrintf("%s", ""));
 }
 
@@ -838,16 +836,6 @@ TEST(StringUtilTest, StringPrintfMisc) {
   EXPECT_EQ(L"123hello w", StringPrintf(L"%3d%2ls %1lc", 123, L"hello", 'w'));
 }
 
-TEST(StringUtilTest, StringAppendfStringEmptyParam) {
-  std::string value("Hello");
-  StringAppendF(&value, "");
-  EXPECT_EQ("Hello", value);
-
-  std::wstring valuew(L"Hello");
-  StringAppendF(&valuew, L"");
-  EXPECT_EQ(L"Hello", valuew);
-}
-
 TEST(StringUtilTest, StringAppendfEmptyString) {
   std::string value("Hello");
   StringAppendF(&value, "%s", "");
@@ -926,6 +914,25 @@ TEST(StringUtilTest, Grow) {
   delete[] ref;
 }
 
+// A helper for the StringAppendV test that follows.
+// Just forwards its args to StringAppendV.
+static void StringAppendVTestHelper(std::string* out,
+                                    const char* format,
+                                    ...) PRINTF_FORMAT(2, 3);
+
+static void StringAppendVTestHelper(std::string* out, const char* format, ...) {
+  va_list ap;
+  va_start(ap, format);
+  StringAppendV(out, format, ap);
+  va_end(ap);
+}
+
+TEST(StringUtilTest, StringAppendV) {
+  std::string out;
+  StringAppendVTestHelper(&out, "%d foo %s", 1, "bar");
+  EXPECT_EQ("1 foo bar", out);
+}
+
 // Test the boundary condition for the size of the string_util's
 // internal buffer.
 TEST(StringUtilTest, GrowBoundary) {
diff --git a/base/trace_event.cc b/base/trace_event.cc
index be2fbaa..13c0c2c 100644
--- a/base/trace_event.cc
+++ b/base/trace_event.cc
@@ -133,10 +133,10 @@ void TraceLog::Trace(const std::string& name,
   int64 usec = delta.InMicroseconds();
   std::string msg =
     StringPrintf("{'pid':'0x%lx', 'tid':'0x%lx', 'type':'%s', "
-                 "'name':'%s', 'id':'0x%lx', 'extra':'%s', 'file':'%s', "
+                 "'name':'%s', 'id':'%p', 'extra':'%s', 'file':'%s', "
                  "'line_number':'%d', 'usec_begin': %" PRId64 "},\n",
-                 base::GetCurrentProcId(),
-                 PlatformThread::CurrentId(),
+                 static_cast<unsigned long>(base::GetCurrentProcId()),
+                 static_cast<unsigned long>(PlatformThread::CurrentId()),
                  kEventTypeNames[type],
                  name.c_str(),
                  id,
diff --git a/base/tracked_objects.cc b/base/tracked_objects.cc
index 1e7d293..0d68703 100644
--- a/base/tracked_objects.cc
+++ b/base/tracked_objects.cc
@@ -6,6 +6,7 @@
 
 #include <math.h>
 
+#include "base/format_macros.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
 
@@ -551,22 +552,23 @@ void Aggregation::Write(std::string* output) const {
   if (locations_.size() == 1) {
     locations_.begin()->first.Write(true, true, output);
   } else {
-    StringAppendF(output, "%d Locations. ", locations_.size());
+    StringAppendF(output, "%" PRIuS " Locations. ", locations_.size());
     if (birth_files_.size() > 1)
-      StringAppendF(output, "%d Files. ", birth_files_.size());
+      StringAppendF(output, "%" PRIuS " Files. ", birth_files_.size());
     else
       StringAppendF(output, "All born in %s. ",
                     birth_files_.begin()->first.c_str());
   }
 
   if (birth_threads_.size() > 1)
-    StringAppendF(output, "%d BirthingThreads. ", birth_threads_.size());
+    StringAppendF(output, "%" PRIuS " BirthingThreads. ",
+                  birth_threads_.size());
   else
     StringAppendF(output, "All born on %s. ",
                   birth_threads_.begin()->first->ThreadName().c_str());
 
   if (death_threads_.size() > 1) {
-    StringAppendF(output, "%d DeathThreads. ", death_threads_.size());
+    StringAppendF(output, "%" PRIuS " DeathThreads. ", death_threads_.size());
   } else {
     if (death_threads_.begin()->first)
       StringAppendF(output, "All deleted on %s. ",
diff --git a/base/worker_pool_linux.cc b/base/worker_pool_linux.cc
index 31fcef1..b9c85b3 100644
--- a/base/worker_pool_linux.cc
+++ b/base/worker_pool_linux.cc
@@ -67,8 +67,7 @@ class WorkerThread : public PlatformThread::Delegate {
 
 void WorkerThread::ThreadMain() {
   const std::string name =
-      StringPrintf("%s/%d", name_prefix_.c_str(),
-                   IntToString(PlatformThread::CurrentId()).c_str());
+      StringPrintf("%s/%d", name_prefix_.c_str(), PlatformThread::CurrentId());
   PlatformThread::SetName(name.c_str());
 
   for (;;) {