[sanitizer] Support padding with spaces in Printf.

[samsonov]

lib/sanitizer_common/sanitizer_printf.cc

43

I would rename "num" to smth. like "absolute_value"

69–77

RAW_CHECK(pos > 0);

78

Oh, this is ugly. Can you write
while (pos >= 0 && num_buffer[pos] == 0) {

char c = (pad_with_zero || pos == 0) ? '0' : ' ');
result += AppendChar(buff, buff_end, c);
pos--;

}
instead of while + if?

95

Does negation works properly for INT_MIN here?

lib/sanitizer_common/tests/sanitizer_printf_test.cc

107

And now the function name doesn't make any sense. Please either rename it, or use different helper in Padding test.

145

Add tests for both 2-digit and 4-digit numbers with %3d / %03d.

[earthdok]

• addressed samsonov's comments

lib/sanitizer_common/sanitizer_printf.cc

43

done

69–77

done

78

I guess so.

95

Yeah, we have tests for that. It's just 2's complement.

lib/sanitizer_common/tests/sanitizer_printf_test.cc

107

done

145

done

[samsonov]

LGTM

lib/sanitizer_common/sanitizer_printf.cc

71

I'd prefer
for (;pos >= 0 && num_buffer[pos] == 0; pos--) { ... }
here and below.

lib/sanitizer_common/sanitizer_printf.cc

static int AppendChar(char **buff, const char *buff_end, char c) {
   if (*buff < buff_end) {
     **buff = c;
     (*buff)++;
   }
   return 1;
 }
 
 // Appends number in a given base to buffer. If its length is less than
-// "minimal_num_length", it is padded with leading zeroes.
-static int AppendUnsigned(char **buff, const char *buff_end, u64 num,
-                          u8 base, u8 minimal_num_length) {
+// |minimal_num_length|, it is padded with leading zeroes or spaces, depending
+// on the value of |pad_with_zero|.
+static int AppendNumber(char **buff, const char *buff_end, u64 absolute_value,
+                        u8 base, u8 minimal_num_length, bool pad_with_zero,
+                        bool negative) {
   uptr const kMaxLen = 30;
   RAW_CHECK(base == 10 || base == 16);
+  RAW_CHECK(base == 10 || !negative);
+  RAW_CHECK(absolute_value || !negative);
   RAW_CHECK(minimal_num_length < kMaxLen);
+  int result = 0;
+  if (negative && minimal_num_length)
+    --minimal_num_length;
+  if (negative && pad_with_zero)
+    result += AppendChar(buff, buff_end, '-');
   uptr num_buffer[kMaxLen];
-  uptr pos = 0;
+  int pos = 0;
   do {
-    RAW_CHECK_MSG(pos < kMaxLen, "appendNumber buffer overflow");
-    num_buffer[pos++] = num % base;
-    num /= base;
-  } while (num > 0);
+    RAW_CHECK_MSG((uptr)pos < kMaxLen, "AppendNumber buffer overflow");
+    num_buffer[pos++] = absolute_value % base;
+    absolute_value /= base;
+  } while (absolute_value > 0);
   if (pos < minimal_num_length) {
     // Make sure compiler doesn't insert call to memset here.
     internal_memset(&num_buffer[pos], 0,
                     sizeof(num_buffer[0]) * (minimal_num_length - pos));
     pos = minimal_num_length;
   }
-  int result = 0;
-  while (pos-- > 0) {
+  RAW_CHECK(pos > 0);
+  pos--;
+  while (pos >= 0 && num_buffer[pos] == 0) {
+    char c = (pad_with_zero || pos == 0) ? '0' : ' ';
+    result += AppendChar(buff, buff_end, c);
+    pos--;
+  }
+  if (negative && !pad_with_zero) result += AppendChar(buff, buff_end, '-');
+  while (pos >= 0) {
     char digit = static_cast<char>(num_buffer[pos]);
     result += AppendChar(buff, buff_end, (digit < 10) ? '0' + digit
                                                       : 'a' + digit - 10);
+    pos--;
   }
   return result;
 }
 
-static int AppendSignedDecimal(char **buff, const char *buff_end, s64 num,
-                               u8 minimal_num_length) {
-  int result = 0;
-  if (num < 0) {
-    result += AppendChar(buff, buff_end, '-');
-    num = -num;
-    if (minimal_num_length)
-      --minimal_num_length;
-  }
-  result += AppendUnsigned(buff, buff_end, (u64)num, 10, minimal_num_length);
-  return result;
+static int AppendUnsigned(char **buff, const char *buff_end, u64 num, u8 base,
+                          u8 minimal_num_length, bool pad_with_zero) {
+  return AppendNumber(buff, buff_end, num, base, minimal_num_length,
+                      pad_with_zero, false /* negative */);
+
+static int AppendSignedDecimal(char **buff, const char *buff_end, s64 num,
+                               u8 minimal_num_length, bool pad_with_zero) {
+  bool negative = (num < 0);
+  return AppendNumber(buff, buff_end, (u64)(negative ? -num : num), 10,
+                      minimal_num_length, pad_with_zero, negative);
 }
 
 static int AppendString(char **buff, const char *buff_end, const char *s) {
   if (s == 0)
     s = "<null>";
   int result = 0;
   for (; *s; s++) {
     result += AppendChar(buff, buff_end, *s);
   }
   return result;
 }
 
 static int AppendPointer(char **buff, const char *buff_end, u64 ptr_value) {
   int result = 0;
   result += AppendString(buff, buff_end, "0x");
   result += AppendUnsigned(buff, buff_end, ptr_value, 16,
-                           (SANITIZER_WORDSIZE == 64) ? 12 : 8);
+                           (SANITIZER_WORDSIZE == 64) ? 12 : 8, true);
   return result;
 }
 
 int VSNPrintf(char *buff, int buff_length,
               const char *format, va_list args) {
   static const char *kPrintfFormatsHelp =
-    "Supported Printf formats: %(0[0-9]*)?(z|ll)?{d,u,x}; %p; %s; %c\n";
+    "Supported Printf formats: %([0-9]*)?(z|ll)?{d,u,x}; %p; %s; %c\n";
   RAW_CHECK(format);
   RAW_CHECK(buff_length > 0);
   const char *buff_end = &buff[buff_length - 1];
   const char *cur = format;
   int result = 0;
   for (; *cur; cur++) {
     if (*cur != '%') {
       result += AppendChar(&buff, buff_end, *cur);
       continue;
     }
     cur++;
-    bool have_width = (*cur == '0');
+    bool have_width = (*cur >= '0' && *cur <= '9');
+    bool pad_with_zero = (*cur == '0');
     int width = 0;
     if (have_width) {
       while (*cur >= '0' && *cur <= '9') {
-        have_width = true;
         width = width * 10 + *cur++ - '0';
       }
     }
     bool have_z = (*cur == 'z');
     cur += have_z;
     bool have_ll = !have_z && (cur[0] == 'l' && cur[1] == 'l');
     cur += have_ll * 2;
     s64 dval;
     u64 uval;
     bool have_flags = have_width | have_z | have_ll;
     switch (*cur) {
       case 'd': {
         dval = have_ll ? va_arg(args, s64)
              : have_z ? va_arg(args, sptr)
              : va_arg(args, int);
-        result += AppendSignedDecimal(&buff, buff_end, dval, width);
+        result += AppendSignedDecimal(&buff, buff_end, dval, width,
+                                      pad_with_zero);
         break;
       }
       case 'u':
       case 'x': {
         uval = have_ll ? va_arg(args, u64)
              : have_z ? va_arg(args, uptr)
              : va_arg(args, unsigned);
         result += AppendUnsigned(&buff, buff_end, uval,
-                                 (*cur == 'u') ? 10 : 16, width);
+                                 (*cur == 'u') ? 10 : 16, width, pad_with_zero);
         break;
       }
       case 'p': {
         RAW_CHECK_MSG(!have_flags, kPrintfFormatsHelp);
         result += AppendPointer(&buff, buff_end, va_arg(args, uptr));
         break;
       }
       case 's': {

lib/sanitizer_common/tests/sanitizer_printf_test.cc

TEST(Printf, OverflowPtr) {
   EXPECT_EQ(buf[5], '6');
   EXPECT_EQ(buf[6], '7');
   EXPECT_EQ(buf[7], '8');
   EXPECT_EQ(buf[8], '9');
   EXPECT_EQ(buf[9], 0);
 }
 
 template<typename T>
-static void TestMinMax(const char *fmt, T min, T max) {
+static void TestAgainstLibc(const char *fmt, T arg1, T arg2) {
   char buf[1024];
-  uptr len = internal_snprintf(buf, sizeof(buf), fmt, min, max);
+  uptr len = internal_snprintf(buf, sizeof(buf), fmt, arg1, arg2);
   char buf2[1024];
-  snprintf(buf2, sizeof(buf2), fmt, min, max);
+  snprintf(buf2, sizeof(buf2), fmt, arg1, arg2);
   EXPECT_EQ(len, strlen(buf));
   EXPECT_STREQ(buf2, buf);
 }
 
 TEST(Printf, MinMax) {
-  TestMinMax<int>("%d-%d", INT_MIN, INT_MAX);  // NOLINT
-  TestMinMax<long>("%zd-%zd", LONG_MIN, LONG_MAX);  // NOLINT
-  TestMinMax<unsigned>("%u-%u", 0, UINT_MAX);  // NOLINT
-  TestMinMax<unsigned long>("%zu-%zu", 0, ULONG_MAX);  // NOLINT
-  TestMinMax<unsigned>("%x-%x", 0, UINT_MAX);  // NOLINT
-  TestMinMax<unsigned long>("%zx-%zx", 0, ULONG_MAX);  // NOLINT
+  TestAgainstLibc<int>("%d-%d", INT_MIN, INT_MAX);  // NOLINT
+  TestAgainstLibc<long>("%zd-%zd", LONG_MIN, LONG_MAX);  // NOLINT
+  TestAgainstLibc<unsigned>("%u-%u", 0, UINT_MAX);  // NOLINT
+  TestAgainstLibc<unsigned long>("%zu-%zu", 0, ULONG_MAX);  // NOLINT
+  TestAgainstLibc<unsigned>("%x-%x", 0, UINT_MAX);  // NOLINT
+  TestAgainstLibc<unsigned long>("%zx-%zx", 0, ULONG_MAX);  // NOLINT
+  Report("%zd\n", LONG_MIN);
+}
+
+TEST(Printf, Padding) {
+  TestAgainstLibc<int>("%3d - %3d", 1, 0);
+  TestAgainstLibc<int>("%3d - %3d", -1, 123);
+  TestAgainstLibc<int>("%3d - %3d", -1, -123);
+  TestAgainstLibc<int>("%3d - %3d", 12, 1234);
+  TestAgainstLibc<int>("%3d - %3d", -12, -1234);
+  TestAgainstLibc<int>("%03d - %03d", 1, 0);
+  TestAgainstLibc<int>("%03d - %03d", -1, 123);
+  TestAgainstLibc<int>("%03d - %03d", -1, -123);
+  TestAgainstLibc<int>("%03d - %03d", 12, 1234);
+  TestAgainstLibc<int>("%03d - %03d", -12, -1234);
 }
 
 }  // namespace __sanitizer