[libc] Lock the output stream for the 'puts' call

[jhuber6]

Summary:
The puts function consists of an initial write and then another write
to append the newline. When executing code in parallel, it is possible
for these writes to becomes disjointed. This code adds an explicit lock
call to ensure that the string is always appended by the newline as the
users expects.

Wasn't sure if this required a test as it would be difficult since
reproducing it would be flaky.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
The puts function consists of an initial write and then another write
to append the newline. When executing code in parallel, it is possible
for these writes to becomes disjointed. This code adds an explicit lock
call to ensure that the string is always appended by the newline as the
users expects.

Wasn't sure if this required a test as it would be difficult since
reproducing it would be flaky.

---

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

1 Files Affected:

• (modified) libc/src/stdio/generic/puts.cpp (+14)

diff --git a/libc/src/stdio/generic/puts.cpp b/libc/src/stdio/generic/puts.cpp
index d8d69332566cd0..052977e5620646 100644
--- a/libc/src/stdio/generic/puts.cpp
+++ b/libc/src/stdio/generic/puts.cpp
@@ -15,14 +15,28 @@
 
 namespace LIBC_NAMESPACE {
 
+// Simple helper to unlock the file once destroyed.
+struct ScopedLock {
+  ScopedLock(LIBC_NAMESPACE::File *stream) : stream(stream) { stream->lock(); }
+  ~ScopedLock() { stream->unlock(); }
+
+private:
+  LIBC_NAMESPACE::File *stream;
+};
+
 LLVM_LIBC_FUNCTION(int, puts, (const char *__restrict str)) {
   cpp::string_view str_view(str);
+
+  // We need to lock the stream to ensure the newline is always appended.
+  ScopedLock lock(LIBC_NAMESPACE::stdout);
+
   auto result = LIBC_NAMESPACE::stdout->write(str, str_view.size());
   if (result.has_error())
     libc_errno = result.error;
   size_t written = result.value;
   if (str_view.size() != written) {
     // The stream should be in an error state in this case.
+    LIBC_NAMESPACE::stdout->unlock();
     return EOF;
   }
   result = LIBC_NAMESPACE::stdout->write("\n", 1);

[SchrodingerZhu]

why this is marked as __restrict?

[jhuber6]

I think all of the implementations of string functions are like this. My best guess is that it's giving the same effect as const char const * without changing the signature or something.

[nickdesaulniers]

Sorry, can you please provide more info on this point; it doesn't sound right to me.

My understanding of __restrict is that it has to do with strict aliasing of function parameters. If there's only 1 parameter, there is nothing to be concerned with wrt to aliasing.

So why add __restrict here?

[jhuber6]

ping