[libc-commits] [libc] [libc] Move File::close out of line to file.cpp (PR #224901)

[via libc-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libc

Author: Jeff Bailey (kaladron)

<details>
<summary>Changes</summary>

Moved File::close from src/__support/File/file.h into src/__support/File/file.cpp and removed #include
"src/__support/CPP/new.h" from file.h, matching Dir::close in src/__support/File/dir.h.

src/__support/CPP/new.h renames global operator delete via __asm__ to __llvm_libc_delete (which calls free) for the entire translation unit without renaming operator new. Including new.h in file.h leaked this replacement operator delete into unit test translation units that include file.h, causing an AddressSanitizer alloc-dealloc-mismatch in Test::createCallable during death tests.

Assisted-by: Automated tooling, human reviewed.

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


2 Files Affected:

- (modified) libc/src/__support/File/file.cpp (+35-1) 
- (modified) libc/src/__support/File/file.h (+8-30) 


``````````diff
diff --git a/libc/src/__support/File/file.cpp b/libc/src/__support/File/file.cpp
index d0366100525ac..c296b99e2ffe4 100644
--- a/libc/src/__support/File/file.cpp
+++ b/libc/src/__support/File/file.cpp
@@ -1,10 +1,15 @@
-//===--- Implementation of a platform independent file data structure -----===//
+//===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
+///
+/// \file
+/// Implementation of a platform independent file data structure.
+///
+//===----------------------------------------------------------------------===//
 
 #include "file.h"
 
@@ -459,6 +464,35 @@ int File::flush_unlocked() {
   return 0;
 }
 
+// Does the following:
+// 1. If in write mode, Write out any data present in the buffer.
+// 2. Call platform_close.
+// platform_close is expected to cleanup the complete file object.
+int File::close() {
+  {
+    FileLock lock(this);
+    if (prev_op == FileOp::WRITE && pos > 0) {
+      auto buf_result = platform_write(this, buf, pos);
+      if (buf_result.has_error() || buf_result.value < pos) {
+        err = true;
+        return buf_result.error;
+      }
+    }
+  }
+
+  // If we own the buffer, delete it before calling the platform close
+  // implementation. The platform close should not need to access the buffer
+  // and we need to clean it up before the entire structure is removed.
+  if (own_buf)
+    delete buf;
+
+  // Platform close is expected to cleanup the file data structure which
+  // includes the file mutex. Hence, we call platform_close after releasing
+  // the file lock. Another thread doing file operations while a thread is
+  // closing the file is undefined behavior as per POSIX.
+  return platform_close(this);
+}
+
 int File::set_buffer(void *buffer, size_t size, int buffer_mode) {
   // We do not need to lock the file as this method should be called before
   // other operations are performed on the file.
diff --git a/libc/src/__support/File/file.h b/libc/src/__support/File/file.h
index 072f5d38c7c95..f5e262ece2587 100644
--- a/libc/src/__support/File/file.h
+++ b/libc/src/__support/File/file.h
@@ -1,10 +1,15 @@
-//===--- A platform independent file data structure -------------*- 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
 //
 //===----------------------------------------------------------------------===//
+///
+/// \file
+/// Platform independent file data structure.
+///
+//===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_SRC___SUPPORT_FILE_FILE_H
 #define LLVM_LIBC_SRC___SUPPORT_FILE_FILE_H
@@ -15,7 +20,6 @@
 #include "hdr/types/off_t.h"
 #include "hdr/types/wchar_t.h"
 #include "hdr/types/wint_t.h"
-#include "src/__support/CPP/new.h"
 #include "src/__support/error_or.h"
 #include "src/__support/macros/config.h"
 #include "src/__support/macros/properties/architectures.h"
@@ -228,34 +232,8 @@ class File {
     return ungetwc_unlocked(wc);
   }
 
-  // Does the following:
-  // 1. If in write mode, Write out any data present in the buffer.
-  // 2. Call platform_close.
-  // platform_close is expected to cleanup the complete file object.
-  int close() {
-    {
-      FileLock lock(this);
-      if (prev_op == FileOp::WRITE && pos > 0) {
-        auto buf_result = platform_write(this, buf, pos);
-        if (buf_result.has_error() || buf_result.value < pos) {
-          err = true;
-          return buf_result.error;
-        }
-      }
-    }
-
-    // If we own the buffer, delete it before calling the platform close
-    // implementation. The platform close should not need to access the buffer
-    // and we need to clean it up before the entire structure is removed.
-    if (own_buf)
-      delete buf;
-
-    // Platform close is expected to cleanup the file data structure which
-    // includes the file mutex. Hence, we call platform_close after releasing
-    // the file lock. Another thread doing file operations while a thread is
-    // closing the file is undefined behavior as per POSIX.
-    return platform_close(this);
-  }
+  // Closes the file stream and frees up all resources owned by it.
+  int close();
 
   // Sets the internal buffer to |buffer| with buffering mode |mode|.
   // |size| is the size of |buffer|. If |size| is non-zero, but |buffer|

``````````

</details>


https://github.com/llvm/llvm-project/pull/224901