[libc] Change default behaviour of baremetal/printf to use stdout

[saturn691]

In #94078, write_to_stdout had not been fully implemented. However, now that it has been implemented, to conform with the C standard (7.23.6.3. The printf function, specifically point 2), we use stdout. This issue is tracked in #94685.

• Also prefer static constexpr
• Made it explicit that we are writing to stdout

[llvmbot]

@llvm/pr-subscribers-libc

Author: William (saturn691)

Changes

In #94078, write_to_stdout had not been fully implemented. However, now that it has been implemented, to conform with the C standard (7.23.6.3. The printf function, specifically point 2), we use printf. This issue is tracked in #94685.

• Also prefer static constexpr
• Made it explicit that we are writing to stdout

---

Full diff: https://github.com/llvm/llvm-project/pull/143703.diff

4 Files Affected:

• (modified) libc/src/stdio/baremetal/printf.cpp (+4-4)
• (modified) libc/src/stdio/baremetal/putchar.cpp (+1-1)
• (modified) libc/src/stdio/baremetal/puts.cpp (+2-2)
• (modified) libc/src/stdio/baremetal/vprintf.cpp (+4-4)

diff --git a/libc/src/stdio/baremetal/printf.cpp b/libc/src/stdio/baremetal/printf.cpp
index c94698ec02953..9814edb556faf 100644
--- a/libc/src/stdio/baremetal/printf.cpp
+++ b/libc/src/stdio/baremetal/printf.cpp
@@ -21,8 +21,8 @@ namespace LIBC_NAMESPACE_DECL {
 
 namespace {
 
-LIBC_INLINE int raw_write_hook(cpp::string_view new_str, void *) {
-  write_to_stderr(new_str);
+LIBC_INLINE int raw_write_stdout_hook(cpp::string_view new_str, void *) {
+  write_to_stdout(new_str);
   return printf_core::WRITE_OK;
 }
 
@@ -35,11 +35,11 @@ LLVM_LIBC_FUNCTION(int, printf, (const char *__restrict format, ...)) {
                                  // and pointer semantics, as well as handling
                                  // destruction automatically.
   va_end(vlist);
-  constexpr size_t BUFF_SIZE = 1024;
+  static constexpr size_t BUFF_SIZE = 1024;
   char buffer[BUFF_SIZE];
 
   printf_core::WriteBuffer<printf_core::WriteMode::FLUSH_TO_STREAM> wb(
-      buffer, BUFF_SIZE, &raw_write_hook, nullptr);
+      buffer, BUFF_SIZE, &raw_write_stdout_hook, nullptr);
   printf_core::Writer<printf_core::WriteMode::FLUSH_TO_STREAM> writer(wb);
 
   int retval = printf_core::printf_main(&writer, format, args);
diff --git a/libc/src/stdio/baremetal/putchar.cpp b/libc/src/stdio/baremetal/putchar.cpp
index 0ba46a5ade6c9..ac21e6e783b01 100644
--- a/libc/src/stdio/baremetal/putchar.cpp
+++ b/libc/src/stdio/baremetal/putchar.cpp
@@ -16,7 +16,7 @@ namespace LIBC_NAMESPACE_DECL {
 LLVM_LIBC_FUNCTION(int, putchar, (int c)) {
   char uc = static_cast<char>(c);
 
-  write_to_stderr(cpp::string_view(&uc, 1));
+  write_to_stdout(cpp::string_view(&uc, 1));
 
   return 0;
 }
diff --git a/libc/src/stdio/baremetal/puts.cpp b/libc/src/stdio/baremetal/puts.cpp
index 5062efda1c0dc..fcd3aa086b2bf 100644
--- a/libc/src/stdio/baremetal/puts.cpp
+++ b/libc/src/stdio/baremetal/puts.cpp
@@ -17,8 +17,8 @@ LLVM_LIBC_FUNCTION(int, puts, (const char *__restrict str)) {
   cpp::string_view str_view(str);
 
   // TODO: Can we combine these to avoid needing two writes?
-  write_to_stderr(str_view);
-  write_to_stderr("\n");
+  write_to_stdout(str_view);
+  write_to_stdout("\n");
 
   return 0;
 }
diff --git a/libc/src/stdio/baremetal/vprintf.cpp b/libc/src/stdio/baremetal/vprintf.cpp
index 3e8631abd90d9..4fa1ae00dd61c 100644
--- a/libc/src/stdio/baremetal/vprintf.cpp
+++ b/libc/src/stdio/baremetal/vprintf.cpp
@@ -21,8 +21,8 @@ namespace LIBC_NAMESPACE_DECL {
 
 namespace {
 
-LIBC_INLINE int raw_write_hook(cpp::string_view new_str, void *) {
-  write_to_stderr(new_str);
+LIBC_INLINE int raw_write_stdout_hook(cpp::string_view new_str, void *) {
+  write_to_stdout(new_str);
   return printf_core::WRITE_OK;
 }
 
@@ -33,11 +33,11 @@ LLVM_LIBC_FUNCTION(int, vprintf,
   internal::ArgList args(vlist); // This holder class allows for easier copying
                                  // and pointer semantics, as well as handling
                                  // destruction automatically.
-  constexpr size_t BUFF_SIZE = 1024;
+  static constexpr size_t BUFF_SIZE = 1024;
   char buffer[BUFF_SIZE];
 
   printf_core::WriteBuffer<printf_core::WriteMode::FLUSH_TO_STREAM> wb(
-      buffer, BUFF_SIZE, &raw_write_hook, nullptr);
+      buffer, BUFF_SIZE, &raw_write_stdout_hook, nullptr);
   printf_core::Writer<printf_core::WriteMode::FLUSH_TO_STREAM> writer(wb);
 
   int retval = printf_core::printf_main(&writer, format, args);

[michaelrj-google]

LGTM, @petrhosek will this break anything for you downstream?

[saturn691]

Hi Petr, I've amended it. I don't have commit access yet, please commit this on behalf of me (if you're happy), thanks!

[petrhosek]

What's the motivation for using static constexpr instead of constexpr for BUFF_SIZE? We don't use it as l-value so it shouldn't make a difference.

[saturn691]

Hi Petr,

Sorry for the long read, but to answer your question, TL;DR, it's faster.

---

In LLVM-libc, as far as I can see, we try to prefer static constexpr whenever possible. So I did a little investigation, to see whether it would make a difference. Here is a test case that mimics what the function does.

Test case:

// test1.cpp
#include <string.h>

int f()
{
  static constexpr int SIZE = 1024;
  char buffer[SIZE];
  memset(buffer, 1, SIZE);

  return buffer[0];
}

int main()
{
  return f() != 1;
}

I have created an identical test case, test2.cpp without static. Note I have built clang from 0eeabd4b302cf52c4a585664ed9bc4a81ef91105. Also note I do not care about higher levels of optimisation, as it will optimise f out, which could be seen in the disassembly.

❯ clang test1.cpp -O0 -o test1
❯ clang test2.cpp -O0 -o test2
❯ perf stat -r 10000 ./test1
# Details omitted but it takes 0.000362472 +- 0.000000342 seconds (time elapsed)
❯ perf stat -r 10000 ./test2
# Details omitted but it takes 0.000363506 +- 0.000000338 seconds (time elapsed)

Which is almost identical. So we look at the disassembly:

# test1
❯ llvm-objdump -D test1 | grep -A12 "fv>" 
0000000000001140 <_Z1fv>:
    1140: 55                           	        pushq	%rbp
    1141: 48 89 e5                     	movq	%rsp, %rbp
    1144: 48 81 ec 00 04 00 00       subq	$0x400, %rsp            # imm = 0x400
    114b: 48 8d bd 00 fc ff ff         	leaq	        -0x400(%rbp), %rdi
    1152: be 01 00 00 00               	movl	$0x1, %esi
    1157: ba 00 04 00 00               	movl	$0x400, %edx            # imm = 0x400
    115c: e8 cf fe ff ff               	        callq	         0x1030 <memset@plt>
    ...
    
# test2
0000000000001140 <_Z1fv>:
    1140: 55                           	        pushq	%rbp
    1141: 48 89 e5                     	movq	%rsp, %rbp
    1144: 48 81 ec 10 04 00 00        subq	$0x410, %rsp            # imm = 0x410
    114b: c7 45 fc 00 04 00 00        movl	$0x400, -0x4(%rbp)      # imm = 0x400
    1152: 48 8d bd f0 fb ff ff         	leaq	        -0x410(%rbp), %rdi
    1159: be 01 00 00 00               	movl	$0x1, %esi
    115e: ba 00 04 00 00               	movl	$0x400, %edx            # imm = 0x400
    1163: e8 c8 fe ff ff               	callq	0x1030 <memset@plt>
    ...

Notice the non-static variable requires extra stack space (0x410 vs 0x400) and an extra instruction (movl at 114b). This is consistent with the observation here: https://stackoverflow.com/questions/70711580/static-constexpr-vs-constexpr-in-function-body

You're right at higher levels of optimisation, we get the same thing. But I think it's worth the change, even if it's for good practice.

[petrhosek]

Thanks for the comprehensive answer! Looking through the codebase we seem to be using both constexpr and static constexpr. I'd also argue that for the baremetal configuration reducing binary size is more important than saving a stack slot (using static constexpr will place the object in .rodata section) but I also doubt anyone would be using the unoptimized build so this is mostly theoretical.