[libc] Fix printf %p format

[michaelrj]

Summary

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.

[jhuber6]

Thanks.

libc/src/stdio/printf_core/ptr_converter.h

23–24

Why do we need to cast nullptr here? Also do we have the raw value itself? We could just check that for zero.

[michaelrj]

libc/src/stdio/printf_core/ptr_converter.h

23–24

I'm not sure why that cast is there. I've removed it.

The reason we check the pointer is because the value is a pointer, so it gets stored in the pointer member.

[michaelrj]

clean up unnecessary cast.

[lntue]

libc/src/stdio/printf_core/ptr_converter.h

32–37

This is now unreachable. You can remove the else from the above and this return statement.

[michaelrj]

remove unnecessary return and simplify conditionals

libc/src/stdio/printf_core/ptr_converter.h

 //===-- Pointer Converter for printf ----------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_PTR_CONVERTER_H
 #define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_PTR_CONVERTER_H
 
 #include "src/__support/CPP/string_view.h"
 #include "src/__support/common.h"
 #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
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_PTR_CONVERTER_H

libc/test/src/stdio/sprintf_test.cpp

TEST(LlvmLibcSPrintfTest, PointerConv) {
   written = __llvm_libc::sprintf(buff, "%p", nullptr);
   EXPECT_EQ(written, 9);
   ASSERT_STREQ(buff, "(nullptr)");
 
   written = __llvm_libc::sprintf(buff, "%p", 0x1a2b3c4d);
   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) {
   char buff[64];
   int written;
 
   // Basic Tests.