sanitizer_common: prepare for enabling format string checking

[dvyukov]

Summary

The attribute((format)) was added somewhere in 2012,
the lost during refactoring, then re-added in 2014 but
to te source files, which is a no-op.
Move it back to header files so that it actually takes effect.
But over the past 7 years we've accumulated whole lot of
format string bugs of different types, so disable the warning
with -Wno-format for now for incremental clean up.

Among the bugs that it warns about are all kinds of bad things:

• wrong sizes of arguments
• missing/excessive arguments
• printing wrong things (e.g. *ptr instead of ptr)
• completely messed up format strings
• security issues where external string is used as format

[dvyukov]

There is a nasty issue: it seems we can't print uptr at all.
%zu give this warning on 32-bit platforms:

/b/sanitizer-x86_64-linux/build/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_tls_get_addr.cpp:140:22: error: format specifies type 'size_t' (aka 'unsigned int') but the argument has type '__sanitizer::uptr' (aka 'unsigned long') [-Werror,-Wformat]
          tls_beg, tls_size);
                   ^~~~~~~~

https://lab.llvm.org/buildbot/#/builders/37/builds/6061

But we can't use %lu either because on windows long is 32-bits and we define uptr to long long. And we obviously can't use %llu for uptr as well.

Maybe we should define uptr to unsigned int on 32-bits to match size_t? But I am not sure if it will cause more problems somewhere else...

Or we can disable -Wformat on 32-bits? Or on windows?

[melver]

In D107977#2943768, @dvyukov wrote:

But we can't use %lu either because on windows long is 32-bits and we define uptr to long long. And we obviously can't use %llu for uptr as well.

Maybe we should define uptr to unsigned int on 32-bits to match size_t? But I am not sure if it will cause more problems somewhere else...

Or we can disable -Wformat on 32-bits? Or on windows?

If it's impossible to have one print-format, probably the only way around is to make a define like

#ifdef ...special builds...
#define UPTR_FMT ...
#elif ....
... 
#else
#define UPTR_FMT "%zu"
#endif

And then just

Printf("Foo bar " UPTR_FMT " baz", ...);

It's a bit more verbose, but would allow not having to disable warnings.

However, the simple and pragmatic solution would probably just be disabling these warnings on 32-bit. Unless they actually are bugs?

[dvyukov]

In D107977#2943785, @melver wrote:

In D107977#2943768, @dvyukov wrote:

But we can't use %lu either because on windows long is 32-bits and we define uptr to long long. And we obviously can't use %llu for uptr as well.

Maybe we should define uptr to unsigned int on 32-bits to match size_t? But I am not sure if it will cause more problems somewhere else...

Or we can disable -Wformat on 32-bits? Or on windows?

If it's impossible to have one print-format, probably the only way around is to make a define like

#ifdef ...special builds...
#define UPTR_FMT ...
#elif ....
... 
#else
#define UPTR_FMT "%zu"
#endif

And then just

Printf("Foo bar " UPTR_FMT " baz", ...);

I don't want to live in this world.

It's a bit more verbose, but would allow not having to disable warnings.

However, the simple and pragmatic solution would probably just be disabling these warnings on 32-bit. Unless they actually are bugs?

Disabling the warning for 32-bits is the only practical solution I see...

[dvyukov]

FTR @fmayer sent D108042 to fix this.

compiler-rt/CMakeLists.txt

if(LLVM_ENABLE_MODULES)
   # don't include system headers, which is incompatible with modules.
   list(APPEND SANITIZER_COMMON_CFLAGS -fno-modules)
 endif()
 
 # Turn off several warnings.
 append_list_if(COMPILER_RT_HAS_WGNU_FLAG -Wno-gnu SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WVARIADIC_MACROS_FLAG -Wno-variadic-macros SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WC99_EXTENSIONS_FLAG -Wno-c99-extensions SANITIZER_COMMON_CFLAGS)
+# Too many existing bugs, needs cleanup.
+append_list_if(COMPILER_RT_HAS_WNO_FORMAT -Wno-format SANITIZER_COMMON_CFLAGS)
+# format-pedantic warns about passing T* for %p, which is not useful.
+append_list_if(COMPILER_RT_HAS_WNO_FORMAT_PEDANTIC -Wno-format-pedantic SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4146_FLAG /wd4146 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4291_FLAG /wd4291 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4391_FLAG /wd4391 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4722_FLAG /wd4722 SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WD4800_FLAG /wd4800 SANITIZER_COMMON_CFLAGS)
 
 append_list_if(MINGW -fms-extensions SANITIZER_COMMON_CFLAGS)
 

compiler-rt/cmake/config-ix.cmake

 check_cxx_compiler_flag("-Werror -Wc99-extensions"     COMPILER_RT_HAS_WC99_EXTENSIONS_FLAG)
 check_cxx_compiler_flag("-Werror -Wgnu"                COMPILER_RT_HAS_WGNU_FLAG)
 check_cxx_compiler_flag("-Werror -Wnon-virtual-dtor"   COMPILER_RT_HAS_WNON_VIRTUAL_DTOR_FLAG)
 check_cxx_compiler_flag("-Werror -Wvariadic-macros"    COMPILER_RT_HAS_WVARIADIC_MACROS_FLAG)
 check_cxx_compiler_flag("-Werror -Wunused-parameter"   COMPILER_RT_HAS_WUNUSED_PARAMETER_FLAG)
 check_cxx_compiler_flag("-Werror -Wcovered-switch-default" COMPILER_RT_HAS_WCOVERED_SWITCH_DEFAULT_FLAG)
 check_cxx_compiler_flag("-Werror -Wsuggest-override"   COMPILER_RT_HAS_WSUGGEST_OVERRIDE_FLAG)
 check_cxx_compiler_flag(-Wno-pedantic COMPILER_RT_HAS_WNO_PEDANTIC)
+check_cxx_compiler_flag(-Wno-format COMPILER_RT_HAS_WNO_FORMAT)
+check_cxx_compiler_flag(-Wno-format-pedantic COMPILER_RT_HAS_WNO_FORMAT_PEDANTIC)
 
 check_cxx_compiler_flag(/W4 COMPILER_RT_HAS_W4_FLAG)
 check_cxx_compiler_flag(/WX COMPILER_RT_HAS_WX_FLAG)
 check_cxx_compiler_flag(/wd4146 COMPILER_RT_HAS_WD4146_FLAG)
 check_cxx_compiler_flag(/wd4291 COMPILER_RT_HAS_WD4291_FLAG)
 check_cxx_compiler_flag(/wd4221 COMPILER_RT_HAS_WD4221_FLAG)
 check_cxx_compiler_flag(/wd4391 COMPILER_RT_HAS_WD4391_FLAG)
 check_cxx_compiler_flag(/wd4722 COMPILER_RT_HAS_WD4722_FLAG)

compiler-rt/lib/sanitizer_common/sanitizer_common.h

 // Passing NULL removes the callback.
 void SetLowLevelAllocateCallback(LowLevelAllocateCallback callback);
 
 // IO
 void CatastrophicErrorWrite(const char *buffer, uptr length);
 void RawWrite(const char *buffer);
 bool ColorizeReports();
 void RemoveANSIEscapeSequencesFromString(char *buffer);
-void Printf(const char *format, ...);
-void Report(const char *format, ...);
+void Printf(const char *format, ...) FORMAT(1, 2);
+void Report(const char *format, ...) FORMAT(1, 2);
 void SetPrintfAndReportCallback(void (*callback)(const char *));
 #define VReport(level, ...)                                              \
   do {                                                                   \
     if ((uptr)Verbosity() >= (level)) Report(__VA_ARGS__); \
   } while (0)
 #define VPrintf(level, ...)                                              \
   do {                                                                   \
     if ((uptr)Verbosity() >= (level)) Printf(__VA_ARGS__); \

  public:
   InternalScopedString() : buffer_(1) { buffer_[0] = '\0'; }
 
   uptr length() const { return buffer_.size() - 1; }
   void clear() {
     buffer_.resize(1);
     buffer_[0] = '\0';
   }
-  void append(const char *format, ...);
+  void append(const char *format, ...) FORMAT(2, 3);
   const char *data() const { return buffer_.data(); }
   char *data() { return buffer_.data(); }
 
  private:
   InternalMmapVector<char> buffer_;
 };
 
 template <class T>

compiler-rt/lib/sanitizer_common/sanitizer_libc.h

 int internal_strncmp(const char *s1, const char *s2, uptr n);
 uptr internal_strlcpy(char *dst, const char *src, uptr maxlen);
 char *internal_strncpy(char *dst, const char *src, uptr n);
 uptr internal_strnlen(const char *s, uptr maxlen);
 char *internal_strrchr(const char *s, int c);
 char *internal_strstr(const char *haystack, const char *needle);
 // Works only for base=10 and doesn't set errno.
 s64 internal_simple_strtoll(const char *nptr, const char **endptr, int base);
-int internal_snprintf(char *buffer, uptr length, const char *format, ...);
+int internal_snprintf(char *buffer, uptr length, const char *format, ...)
+    FORMAT(3, 4);
 
 // Return true if all bytes in [mem, mem+size) are zero.
 // Optimized for the case when the result is true.
 bool mem_is_zero(const char *mem, uptr size);
 
 // I/O
 // Define these as macros so we can use them in linker initialized global
 // structs without dynamic initialization.

compiler-rt/lib/sanitizer_common/sanitizer_printf.cpp

static void NOINLINE SharedPrintfCode(bool append_pid, const char *format,
   // the stack limit enforced by TSan (-Wframe-larger-than=512). On the other
   // hand, the bigger the buffer is, the more the chance the error report will
   // fit into it.
   char local_buffer[400];
   SharedPrintfCodeNoBuffer(append_pid, local_buffer, ARRAY_SIZE(local_buffer),
                            format, args);
 }
 
-FORMAT(1, 2)
 void Printf(const char *format, ...) {
   va_list args;
   va_start(args, format);
   SharedPrintfCode(false, format, args);
   va_end(args);
 }
 
 // Like Printf, but prints the current PID before the output string.
-FORMAT(1, 2)
 void Report(const char *format, ...) {
   va_list args;
   va_start(args, format);
   SharedPrintfCode(true, format, args);
   va_end(args);
 }
 
 // Writes at most "length" symbols to "buffer" (including trailing '\0').
 // Returns the number of symbols that should have been written to buffer
 // (not including trailing '\0'). Thus, the string is truncated
 // iff return value is not less than "length".
-FORMAT(3, 4)
 int internal_snprintf(char *buffer, uptr length, const char *format, ...) {
   va_list args;
   va_start(args, format);
   int needed_length = VSNPrintf(buffer, length, format, args);
   va_end(args);
   return needed_length;
 }
 
-FORMAT(2, 3)
 void InternalScopedString::append(const char *format, ...) {
   uptr prev_len = length();
 
   while (true) {
     buffer_.resize(buffer_.capacity());
 
     va_list args;
     va_start(args, format);