[libc-commits] [libc] f9868aa - [libc][NFC] Use operator delete to cleanup a File object.
Siva Chandra Reddy via libc-commits
libc-commits at lists.llvm.org
Thu Dec 22 14:31:27 PST 2022
Author: Siva Chandra Reddy
Date: 2022-12-22T22:31:06Z
New Revision: f9868aa7292a0821f4ed29048d4d4214b17cbb92
URL: https://github.com/llvm/llvm-project/commit/f9868aa7292a0821f4ed29048d4d4214b17cbb92
DIFF: https://github.com/llvm/llvm-project/commit/f9868aa7292a0821f4ed29048d4d4214b17cbb92.diff
LOG: [libc][NFC] Use operator delete to cleanup a File object.
The File API has been refactored to allow cleanup using operator delete.
Reviewed By: lntue
Differential Revision: https://reviews.llvm.org/D140574
Added:
Modified:
libc/src/__support/File/file.cpp
libc/src/__support/File/file.h
libc/src/__support/File/linux_file.cpp
libc/src/stdio/fclose.cpp
libc/src/stdio/fopencookie.cpp
libc/test/src/__support/File/file_test.cpp
libc/test/src/__support/File/platform_file_test.cpp
Removed:
################################################################################
diff --git a/libc/src/__support/File/file.cpp b/libc/src/__support/File/file.cpp
index 1493633718e6d..53fa06b10c8aa 100644
--- a/libc/src/__support/File/file.cpp
+++ b/libc/src/__support/File/file.cpp
@@ -338,26 +338,6 @@ int File::flush_unlocked() {
return 0;
}
-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;
- }
- }
- int result = platform_close(this);
- if (result != 0)
- return result;
- if (own_buf)
- free(buf);
- }
- free(this);
- return 0;
-}
-
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 3828694cbf032..2299584c3d64e 100644
--- a/libc/src/__support/File/file.h
+++ b/libc/src/__support/File/file.h
@@ -9,6 +9,7 @@
#ifndef LLVM_LIBC_SRC_SUPPORT_OSUTIL_FILE_H
#define LLVM_LIBC_SRC_SUPPORT_OSUTIL_FILE_H
+#include "src/__support/CPP/new.h"
#include "src/__support/error_or.h"
#include "src/__support/threads/mutex.h"
@@ -46,6 +47,16 @@ class File {
using SeekFunc = ErrorOr<long>(File *, long, int);
using CloseFunc = int(File *);
using FlushFunc = int(File *);
+ // CleanupFunc is a function which does the equivalent of this:
+ //
+ // void my_file_cleanup(File *f) {
+ // MyFile *file = reinterpret_cast<MyFile *>(f);
+ // delete file;
+ // }
+ //
+ // Essentially, it a function which calls the delete operator on the
+ // platform file object to cleanup resources held by it.
+ using CleanupFunc = void(File *);
using ModeFlags = uint32_t;
@@ -83,6 +94,7 @@ class File {
SeekFunc *platform_seek;
CloseFunc *platform_close;
FlushFunc *platform_flush;
+ CleanupFunc *platform_cleanup;
Mutex mutex;
@@ -130,6 +142,26 @@ class File {
FileLock(FileLock &&) = delete;
};
+ // This is private function and is not to be called by the users of
+ // File and its derived classes. The correct way to close a file is
+ // to call the File::cleanup function.
+ 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;
+ }
+ }
+ int result = platform_close(this);
+ if (result != 0)
+ return result;
+ }
+ return 0;
+ }
+
protected:
constexpr bool write_allowed() const {
return mode & (static_cast<ModeFlags>(OpenMode::WRITE) |
@@ -142,6 +174,11 @@ class File {
static_cast<ModeFlags>(OpenMode::PLUS));
}
+ ~File() {
+ if (own_buf)
+ delete buf;
+ }
+
public:
// We want this constructor to be constexpr so that global file objects
// like stdout do not require invocation of the constructor which can
@@ -151,16 +188,29 @@ class File {
// is zero. This way, we will not have to employ the semantics of
// the set_buffer method and allocate a buffer.
constexpr File(WriteFunc *wf, ReadFunc *rf, SeekFunc *sf, CloseFunc *cf,
- FlushFunc *ff, uint8_t *buffer, size_t buffer_size,
- int buffer_mode, bool owned, ModeFlags modeflags)
+ FlushFunc *ff, CleanupFunc *clf, uint8_t *buffer,
+ size_t buffer_size, int buffer_mode, bool owned,
+ ModeFlags modeflags)
: platform_write(wf), platform_read(rf), platform_seek(sf),
- platform_close(cf), platform_flush(ff), mutex(false, false, false),
- ungetc_buf(0), buf(buffer), bufsize(buffer_size), bufmode(buffer_mode),
- own_buf(owned), mode(modeflags), pos(0), prev_op(FileOp::NONE),
- read_limit(0), eof(false), err(false) {
+ platform_close(cf), platform_flush(ff), platform_cleanup(clf),
+ mutex(false, false, false), ungetc_buf(0), buf(buffer),
+ bufsize(buffer_size), bufmode(buffer_mode), own_buf(owned),
+ mode(modeflags), pos(0), prev_op(FileOp::NONE), read_limit(0),
+ eof(false), err(false) {
adjust_buf();
}
+ // Close |f| and cleanup resources held by it.
+ // Returns the non-zero error value if an error occurs when closing the
+ // file.
+ static constexpr int cleanup(File *f) {
+ int close_result = f->close();
+ if (close_result != 0)
+ return close_result;
+ f->platform_cleanup(f);
+ return 0;
+ }
+
// Buffered write of |len| bytes from |data| without the file lock.
FileIOResult write_unlocked(const void *data, size_t len);
@@ -214,9 +264,6 @@ class File {
// is ENOMEM.
int set_buffer(void *buffer, size_t size, int buffer_mode);
- // Closes the file stream and frees up all resources owned by it.
- int close();
-
void lock() { mutex.lock(); }
void unlock() { mutex.unlock(); }
@@ -273,6 +320,15 @@ class File {
}
};
+// Platform specific file implementations can simply pass a pointer to a
+// a specialization of this function as the CleanupFunc argument to the
+// File constructor. The template type argument FileType should replaced
+// with the type of the platform specific file implementation.
+template <typename FileType> void cleanup_file(File *f) {
+ auto *file = reinterpret_cast<FileType *>(f);
+ delete file;
+}
+
// The implementaiton of this function is provided by the platfrom_file
// library.
ErrorOr<File *> openfile(const char *path, const char *mode);
diff --git a/libc/src/__support/File/linux_file.cpp b/libc/src/__support/File/linux_file.cpp
index 13de7c46994e2..be6c61b58ecfb 100644
--- a/libc/src/__support/File/linux_file.cpp
+++ b/libc/src/__support/File/linux_file.cpp
@@ -35,7 +35,8 @@ class LinuxFile : public File {
constexpr LinuxFile(int file_descriptor, uint8_t *buffer, size_t buffer_size,
int buffer_mode, bool owned, File::ModeFlags modeflags)
: File(&write_func, &read_func, &seek_func, &close_func, flush_func,
- buffer, buffer_size, buffer_mode, owned, modeflags),
+ &cleanup_file<LinuxFile>, buffer, buffer_size, buffer_mode, owned,
+ modeflags),
fd(file_descriptor) {}
int get_fd() const { return fd; }
diff --git a/libc/src/stdio/fclose.cpp b/libc/src/stdio/fclose.cpp
index c287442dc0010..b4a366af1443f 100644
--- a/libc/src/stdio/fclose.cpp
+++ b/libc/src/stdio/fclose.cpp
@@ -15,7 +15,8 @@
namespace __llvm_libc {
LLVM_LIBC_FUNCTION(int, fclose, (::FILE * stream)) {
- int result = reinterpret_cast<__llvm_libc::File *>(stream)->close();
+ auto *file = reinterpret_cast<__llvm_libc::File *>(stream);
+ int result = File::cleanup(file);
if (result != 0) {
errno = result;
return EOF;
diff --git a/libc/src/stdio/fopencookie.cpp b/libc/src/stdio/fopencookie.cpp
index a48e28f1324e9..2e3cce7e6672c 100644
--- a/libc/src/stdio/fopencookie.cpp
+++ b/libc/src/stdio/fopencookie.cpp
@@ -32,8 +32,9 @@ class CookieFile : public __llvm_libc::File {
CookieFile(void *c, cookie_io_functions_t cops, uint8_t *buffer,
size_t bufsize, File::ModeFlags mode)
: File(&cookie_write, &cookie_read, &CookieFile::cookie_seek,
- &cookie_close, &cookie_flush, buffer, bufsize,
- 0 /* default buffering mode */, true /* File owns buffer */, mode),
+ &cookie_close, &cookie_flush, &cleanup_file<CookieFile>, buffer,
+ bufsize, 0 /* default buffering mode */,
+ true /* File owns buffer */, mode),
cookie(c), ops(cops) {}
};
diff --git a/libc/test/src/__support/File/file_test.cpp b/libc/test/src/__support/File/file_test.cpp
index f3690e492534e..6dbf2c0278fce 100644
--- a/libc/test/src/__support/File/file_test.cpp
+++ b/libc/test/src/__support/File/file_test.cpp
@@ -18,9 +18,10 @@
using ModeFlags = __llvm_libc::File::ModeFlags;
using MemoryView = __llvm_libc::memory::testing::MemoryView;
using __llvm_libc::ErrorOr;
+using __llvm_libc::File;
using __llvm_libc::FileIOResult;
-class StringFile : public __llvm_libc::File {
+class StringFile : public File {
static constexpr size_t SIZE = 512;
size_t pos;
char str[SIZE] = {0};
@@ -38,8 +39,9 @@ class StringFile : public __llvm_libc::File {
explicit StringFile(char *buffer, size_t buflen, int bufmode, bool owned,
ModeFlags modeflags)
: __llvm_libc::File(&str_write, &str_read, &str_seek, &str_close,
- &str_flush, reinterpret_cast<uint8_t *>(buffer),
- buflen, bufmode, owned, modeflags),
+ &str_flush, &__llvm_libc::cleanup_file<StringFile>,
+ reinterpret_cast<uint8_t *>(buffer), buflen, bufmode,
+ owned, modeflags),
pos(0), eof_marker(0), write_append(false) {
if (modeflags & static_cast<ModeFlags>(__llvm_libc::File::OpenMode::APPEND))
write_append = true;
@@ -144,7 +146,7 @@ TEST(LlvmLibcFileTest, WriteOnly) {
EXPECT_TRUE(result.has_error());
}
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, WriteLineBuffered) {
@@ -203,8 +205,8 @@ TEST(LlvmLibcFileTest, WriteLineBuffered) {
EXPECT_MEM_EQ(src3, dst_line_final);
EXPECT_MEM_EQ(src3, dst_full_final);
- ASSERT_EQ(f_line->close(), 0);
- ASSERT_EQ(f_full->close(), 0);
+ ASSERT_EQ(File::cleanup(f_line), 0);
+ ASSERT_EQ(File::cleanup(f_full), 0);
}
TEST(LlvmLibcFileTest, WriteUnbuffered) {
@@ -219,7 +221,7 @@ TEST(LlvmLibcFileTest, WriteUnbuffered) {
sizeof(data)); // no buffering means this is written immediately.
EXPECT_STREQ(f->get_str(), data);
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, ReadOnly) {
@@ -272,7 +274,7 @@ TEST(LlvmLibcFileTest, ReadOnly) {
EXPECT_TRUE(result.has_error());
}
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, ReadSeekCurAndRead) {
@@ -294,7 +296,7 @@ TEST(LlvmLibcFileTest, ReadSeekCurAndRead) {
ASSERT_EQ(f->seek(-5, SEEK_CUR).value(), 0);
ASSERT_EQ(f->read(data, READ_SIZE - 1).value, READ_SIZE - 1);
ASSERT_STREQ(data, "9098");
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, AppendOnly) {
@@ -324,7 +326,7 @@ TEST(LlvmLibcFileTest, AppendOnly) {
EXPECT_EQ(f->flush(), int(0));
EXPECT_EQ(f->get_pos(), sizeof(write_data) + sizeof(initial_content));
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, WriteUpdate) {
@@ -344,7 +346,7 @@ TEST(LlvmLibcFileTest, WriteUpdate) {
ASSERT_EQ(f->read(read_data, sizeof(data)).value, sizeof(data));
EXPECT_STREQ(read_data, data);
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, ReadUpdate) {
@@ -377,7 +379,7 @@ TEST(LlvmLibcFileTest, ReadUpdate) {
src2(write_data, sizeof(write_data));
EXPECT_MEM_EQ(src2, dst2);
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, AppendUpdate) {
@@ -419,7 +421,7 @@ TEST(LlvmLibcFileTest, AppendUpdate) {
MemoryView src4(initial_content, READ_SIZE), dst4(read_data, READ_SIZE);
EXPECT_MEM_EQ(src4, dst4);
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, SmallBuffer) {
@@ -436,7 +438,7 @@ TEST(LlvmLibcFileTest, SmallBuffer) {
EXPECT_EQ(f->get_pos(), sizeof(WRITE_DATA));
ASSERT_STREQ(f->get_str(), WRITE_DATA);
- ASSERT_EQ(f->close(), 0);
+ ASSERT_EQ(File::cleanup(f), 0);
}
TEST(LlvmLibcFileTest, ZeroLengthBuffer) {
@@ -458,9 +460,9 @@ TEST(LlvmLibcFileTest, ZeroLengthBuffer) {
ASSERT_STREQ(f_lbf->get_str(), WRITE_DATA);
ASSERT_STREQ(f_nbf->get_str(), WRITE_DATA);
- ASSERT_EQ(f_fbf->close(), 0);
- ASSERT_EQ(f_lbf->close(), 0);
- ASSERT_EQ(f_nbf->close(), 0);
+ ASSERT_EQ(File::cleanup(f_fbf), 0);
+ ASSERT_EQ(File::cleanup(f_lbf), 0);
+ ASSERT_EQ(File::cleanup(f_nbf), 0);
}
TEST(LlvmLibcFileTest, WriteNothing) {
@@ -485,7 +487,7 @@ TEST(LlvmLibcFileTest, WriteNothing) {
ASSERT_FALSE(f_lbf->error_unlocked());
ASSERT_FALSE(f_nbf->error_unlocked());
- ASSERT_EQ(f_fbf->close(), 0);
- ASSERT_EQ(f_lbf->close(), 0);
- ASSERT_EQ(f_nbf->close(), 0);
+ ASSERT_EQ(File::cleanup(f_fbf), 0);
+ ASSERT_EQ(File::cleanup(f_lbf), 0);
+ ASSERT_EQ(File::cleanup(f_nbf), 0);
}
diff --git a/libc/test/src/__support/File/platform_file_test.cpp b/libc/test/src/__support/File/platform_file_test.cpp
index 51f7f9aeea03d..abbd8113846ac 100644
--- a/libc/test/src/__support/File/platform_file_test.cpp
+++ b/libc/test/src/__support/File/platform_file_test.cpp
@@ -20,7 +20,7 @@ TEST(LlvmLibcPlatformFileTest, CreateWriteCloseAndReadBack) {
File *file = __llvm_libc::openfile(FILENAME, "w");
ASSERT_FALSE(file == nullptr);
ASSERT_EQ(file->write(TEXT, TEXT_SIZE).value, TEXT_SIZE);
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
file = __llvm_libc::openfile(FILENAME, "r");
ASSERT_FALSE(file == nullptr);
@@ -33,7 +33,7 @@ TEST(LlvmLibcPlatformFileTest, CreateWriteCloseAndReadBack) {
ASSERT_EQ(file->read(data, TEXT_SIZE).value, size_t(0));
ASSERT_TRUE(file->iseof());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
}
TEST(LlvmLibcPlatformFileTest, CreateWriteSeekAndReadBack) {
@@ -53,7 +53,7 @@ TEST(LlvmLibcPlatformFileTest, CreateWriteSeekAndReadBack) {
ASSERT_EQ(file->read(data, TEXT_SIZE).value, size_t(0));
ASSERT_TRUE(file->iseof());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
}
TEST(LlvmLibcPlatformFileTest, CreateAppendCloseAndReadBack) {
@@ -61,14 +61,14 @@ TEST(LlvmLibcPlatformFileTest, CreateAppendCloseAndReadBack) {
File *file = __llvm_libc::openfile(FILENAME, "w");
ASSERT_FALSE(file == nullptr);
ASSERT_EQ(file->write(TEXT, TEXT_SIZE).value, TEXT_SIZE);
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
file = __llvm_libc::openfile(FILENAME, "a");
ASSERT_FALSE(file == nullptr);
constexpr char APPEND_TEXT[] = " Append Text";
constexpr size_t APPEND_TEXT_SIZE = sizeof(APPEND_TEXT) - 1;
ASSERT_EQ(file->write(APPEND_TEXT, APPEND_TEXT_SIZE).value, APPEND_TEXT_SIZE);
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
file = __llvm_libc::openfile(FILENAME, "r");
ASSERT_FALSE(file == nullptr);
@@ -82,7 +82,7 @@ TEST(LlvmLibcPlatformFileTest, CreateAppendCloseAndReadBack) {
ASSERT_EQ(file->read(data, READ_SIZE).value, size_t(0));
ASSERT_TRUE(file->iseof());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
}
TEST(LlvmLibcPlatformFileTest, CreateAppendSeekAndReadBack) {
@@ -90,7 +90,7 @@ TEST(LlvmLibcPlatformFileTest, CreateAppendSeekAndReadBack) {
File *file = __llvm_libc::openfile(FILENAME, "w");
ASSERT_FALSE(file == nullptr);
ASSERT_EQ(file->write(TEXT, TEXT_SIZE).value, TEXT_SIZE);
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
file = __llvm_libc::openfile(FILENAME, "a+");
ASSERT_FALSE(file == nullptr);
@@ -108,7 +108,7 @@ TEST(LlvmLibcPlatformFileTest, CreateAppendSeekAndReadBack) {
ASSERT_EQ(file->read(data, APPEND_TEXT_SIZE).value, size_t(0));
ASSERT_TRUE(file->iseof());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
}
TEST(LlvmLibcPlatformFileTest, LargeFile) {
@@ -126,7 +126,7 @@ TEST(LlvmLibcPlatformFileTest, LargeFile) {
for (int i = 0; i < REPEAT; ++i) {
ASSERT_EQ(file->write(write_data, DATA_SIZE).value, DATA_SIZE);
}
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
file = __llvm_libc::openfile(FILENAME, "r");
ASSERT_FALSE(file == nullptr);
@@ -141,7 +141,7 @@ TEST(LlvmLibcPlatformFileTest, LargeFile) {
ASSERT_EQ(file->read(data, 1).value, size_t(0));
ASSERT_TRUE(file->iseof());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
}
TEST(LlvmLibcPlatformFileTest, ReadSeekCurAndRead) {
@@ -151,7 +151,7 @@ TEST(LlvmLibcPlatformFileTest, ReadSeekCurAndRead) {
constexpr char CONTENT[] = "1234567890987654321";
ASSERT_EQ(sizeof(CONTENT) - 1,
file->write(CONTENT, sizeof(CONTENT) - 1).value);
- ASSERT_EQ(0, file->close());
+ ASSERT_EQ(0, File::cleanup(file));
file = __llvm_libc::openfile(FILENAME, "r");
ASSERT_FALSE(file == nullptr);
@@ -168,7 +168,7 @@ TEST(LlvmLibcPlatformFileTest, ReadSeekCurAndRead) {
ASSERT_EQ(file->read(data, READ_SIZE - 1).value, READ_SIZE - 1);
ASSERT_STREQ(data, "9098");
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
}
TEST(LlvmLibcPlatformFileTest, IncorrectOperation) {
@@ -180,21 +180,21 @@ TEST(LlvmLibcPlatformFileTest, IncorrectOperation) {
ASSERT_EQ(file->read(data, 1).value, size_t(0)); // Cannot read
ASSERT_FALSE(file->iseof());
ASSERT_TRUE(file->error());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
file = __llvm_libc::openfile(FILENAME, "r");
ASSERT_FALSE(file == nullptr);
ASSERT_EQ(file->write(data, 1).value, size_t(0)); // Cannot write
ASSERT_FALSE(file->iseof());
ASSERT_TRUE(file->error());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
file = __llvm_libc::openfile(FILENAME, "a");
ASSERT_FALSE(file == nullptr);
ASSERT_EQ(file->read(data, 1).value, size_t(0)); // Cannot read
ASSERT_FALSE(file->iseof());
ASSERT_TRUE(file->error());
- ASSERT_EQ(file->close(), 0);
+ ASSERT_EQ(File::cleanup(file), 0);
}
TEST(LlvmLibcPlatformFileTest, StdOutStdErrSmokeTest) {
More information about the libc-commits
mailing list