[libc-commits] [libc] dd51ae8 - [libc] Fix printf %p format

[Michael Jones via libc-commits]

Author: Michael Jones
Date: 2023-09-07T14:13:35-07:00
New Revision: dd51ae81d8d3a24fbe3839686a1e18569fbd5330

URL: https://github.com/llvm/llvm-project/commit/dd51ae81d8d3a24fbe3839686a1e18569fbd5330
DIFF: https://github.com/llvm/llvm-project/commit/dd51ae81d8d3a24fbe3839686a1e18569fbd5330.diff

LOG: [libc] Fix printf %p format

The %p format wasn't correctly passing along flags and modifiers to the
integer conversion behind the scenes. This patch fixes that behavior, as
well as changing the nullptr behavior to be a string conversion behind
the scenes.

Reviewed By: lntue, jhuber6

Differential Revision: https://reviews.llvm.org/D159458

Added: 
    

Modified: 
    libc/src/stdio/printf_core/ptr_converter.h
    libc/test/src/stdio/sprintf_test.cpp

Removed: 
    


################################################################################
diff  --git a/libc/src/stdio/printf_core/ptr_converter.h b/libc/src/stdio/printf_core/ptr_converter.h
index e76e0f2613b8227..6304abcdb6ddafb 100644
--- a/libc/src/stdio/printf_core/ptr_converter.h
+++ b/libc/src/stdio/printf_core/ptr_converter.h
@@ -14,23 +14,27 @@
 #include "src/stdio/printf_core/converter_utils.h"
 #include "src/stdio/printf_core/core_structs.h"
 #include "src/stdio/printf_core/int_converter.h"
+#include "src/stdio/printf_core/string_converter.h"
 #include "src/stdio/printf_core/writer.h"
 
 namespace __llvm_libc {
 namespace printf_core {
 
 LIBC_INLINE int convert_pointer(Writer *writer, const FormatSection &to_conv) {
-  if (to_conv.conv_val_ptr == (void *)(nullptr)) {
-    RET_IF_RESULT_NEGATIVE(writer->write("(nullptr)"));
-  } else {
-    FormatSection hex_conv;
-    hex_conv.has_conv = true;
-    hex_conv.conv_name = 'x';
-    hex_conv.flags = FormatFlags::ALTERNATE_FORM;
-    hex_conv.conv_val_raw = reinterpret_cast<uintptr_t>(to_conv.conv_val_ptr);
-    return convert_int(writer, hex_conv);
+  FormatSection new_conv = to_conv;
+
+  if (to_conv.conv_val_ptr == nullptr) {
+    constexpr char NULLPTR_STR[] = "(nullptr)";
+    new_conv.conv_name = 's';
+    new_conv.conv_val_ptr = const_cast<char *>(NULLPTR_STR);
+    return convert_string(writer, new_conv);
   }
-  return WRITE_OK;
+  new_conv.conv_name = 'x';
+  new_conv.flags =
+      static_cast<FormatFlags>(to_conv.flags | FormatFlags::ALTERNATE_FORM);
+  new_conv.length_modifier = LengthModifier::t;
+  new_conv.conv_val_raw = reinterpret_cast<uintptr_t>(to_conv.conv_val_ptr);
+  return convert_int(writer, new_conv);
 }
 
 } // namespace printf_core

diff  --git a/libc/test/src/stdio/sprintf_test.cpp b/libc/test/src/stdio/sprintf_test.cpp
index 42aa85b2df15905..68fbe6e460207f7 100644
--- a/libc/test/src/stdio/sprintf_test.cpp
+++ b/libc/test/src/stdio/sprintf_test.cpp
@@ -394,8 +394,54 @@ TEST(LlvmLibcSPrintfTest, PointerConv) {
   EXPECT_EQ(written, 10);
   ASSERT_STREQ(buff, "0x1a2b3c4d");
 
+  if constexpr (sizeof(void *) > 4) {
+    written = __llvm_libc::sprintf(buff, "%p", 0x1a2b3c4d5e6f7081);
+    EXPECT_EQ(written, 18);
+    ASSERT_STREQ(buff, "0x1a2b3c4d5e6f7081");
+  }
+
   written = __llvm_libc::sprintf(buff, "%p", buff);
   EXPECT_GT(written, 0);
+
+  // Width tests:
+
+  written = __llvm_libc::sprintf(buff, "%20p", nullptr);
+  EXPECT_EQ(written, 20);
+  ASSERT_STREQ(buff, "           (nullptr)");
+
+  written = __llvm_libc::sprintf(buff, "%20p", 0x1a2b3c4d);
+  EXPECT_EQ(written, 20);
+  ASSERT_STREQ(buff, "          0x1a2b3c4d");
+
+  // Flag tests:
+
+  written = __llvm_libc::sprintf(buff, "%-20p", nullptr);
+  EXPECT_EQ(written, 20);
+  ASSERT_STREQ(buff, "(nullptr)           ");
+
+  written = __llvm_libc::sprintf(buff, "%-20p", 0x1a2b3c4d);
+  EXPECT_EQ(written, 20);
+  ASSERT_STREQ(buff, "0x1a2b3c4d          ");
+
+  // Using the 0 flag is technically undefined, but here we're following the
+  // convention of matching the behavior of %#x.
+  written = __llvm_libc::sprintf(buff, "%020p", 0x1a2b3c4d);
+  EXPECT_EQ(written, 20);
+  ASSERT_STREQ(buff, "0x00000000001a2b3c4d");
+
+  // Precision tests:
+  // These are all undefined behavior. The precision option is undefined for %p.
+
+  // Precision specifies the number of characters for a string conversion.
+  written = __llvm_libc::sprintf(buff, "%.5p", nullptr);
+  EXPECT_EQ(written, 5);
+  ASSERT_STREQ(buff, "(null");
+
+  // Precision specifies the number of digits to be written for %x conversions,
+  // and the "0x" doesn't count as part of the digits.
+  written = __llvm_libc::sprintf(buff, "%.20p", 0x1a2b3c4d);
+  EXPECT_EQ(written, 22);
+  ASSERT_STREQ(buff, "0x0000000000001a2b3c4d");
 }
 
 TEST(LlvmLibcSPrintfTest, OctConv) {