Nick Kledzik
2012-May-17 22:25 UTC
[LLVMdev] [RFC] llvm/include/Support/FileOutputBuffer.h
I now have an implementation of FileOutputBuffer (OutputBuffer was already taken). The patch supports the functionality listed below and I've tested that it works for lld. -------------- next part -------------- A non-text attachment was scrubbed... Name: FileOutputBuffer.patch Type: application/octet-stream Size: 25308 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120517/9ab7084f/attachment.obj> -------------- next part -------------- To implement the FileOutputBuffer, I needed to add some more functions to llvm/Support/FileSystem.h, including: is_writable_file() is_executable_file() set_file_executable() unique_file_sized() map_file_pages() unmap_file_pages() I've implemented the unix side for all these, but have the windows side just stubbed out. I've also added a test case (unittests/Support/FileOutputBufferTest.cpp) which exercises FileOutputBuffer. -Nick>>>> >>>> On May 3, 2012, at 6:10 PM, Nick Kledzik wrote: >>>> >>>> Existing llvm code tends to use raw_ostream for writing files. But >>>> raw_ostream is not a good match for a linker for a couple of reasons: >>>> >>>> 1) When the linker creates an executable, the file needs the 'x' bit set. >>>> Currently raw_fd_ostream has no way to set that. >>>> >>>> 2) The Unix conformance suite actually has some test cases where the linker >>>> is run and the output file does exists but is not writable, or is not >>>> writable but is in a writable directory, or with funky umask values. >>>> raw_fd_ostream interface has no way to match those semantics. >>>> >>>> 3) On darwin we have found the linker performs better if it opens the output >>>> file, truncates it to the output size, then mmaps in the file, then writes >>>> directly into that memory buffer. This avoids the memory copy from the >>>> private buffer to the OS file system buffer in the write() syscall. >>>> >>>> 4) In the model we are using for lld, a streaming output interface is not >>>> optimal. Currently, lld copies chunks of code from the (read-only) input >>>> files, to a temporary buffer, then applies any fixups (relocations), then >>>> streams out that temporary buffer. If instead we had a big output buffer, >>>> the linker could copy the code chunks directly to the output buffer and >>>> apply the fixups there, avoiding an extra copy. >>>>
Michael Spencer
2012-May-18 22:07 UTC
[LLVMdev] [RFC] llvm/include/Support/FileOutputBuffer.h
On Thu, May 17, 2012 at 3:25 PM, Nick Kledzik <kledzik at apple.com> wrote:> I now have an implementation of FileOutputBuffer (OutputBuffer was already taken). The patch supports the functionality listed below and I've tested that it works for lld. > > To implement the FileOutputBuffer, I needed to add some more functions to llvm/Support/FileSystem.h, including: > is_writable_file() > is_executable_file() > set_file_executable()For these parts I would like to follow http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3365.html#TOC which is what the rest of the fs library is modeled after. So we would use: error_code permissions(const Twine &path, perms p); If the name is confusing (it says nothing about modification) I'm fine with modify_permissions or something similar.> unique_file_sized()Instead of adding this I would much rather add resize_file and call it after unique_file.> map_file_pages() > unmap_file_pages()These are good.> I've implemented the unix side for all these, but have the windows side just stubbed out. > > I've also added a test case (unittests/Support/FileOutputBufferTest.cpp) which exercises FileOutputBuffer. > > -NickAs a generally comment there is lots of trailing white space and tabs and white space only changes. Inline comments:> Index: include/llvm/Support/FileSystem.h > ==================================================================> --- include/llvm/Support/FileSystem.h (revision 157010) > +++ include/llvm/Support/FileSystem.h (working copy) > @@ -426,12 +454,35 @@ > /// @param result_path Set to the opened file's absolute path. > /// @param makeAbsolute If true and @model is not an absolute path, a temp > /// directory will be prepended. > +/// @param private_file If true file will have permissions set to only > +/// be accessible by the current user. If false, then the file > +/// permissions are set to be globally readable. > /// @results errc::success if result_{fd,path} have been successfully set, > /// otherwise a platform specific error_code. > error_code unique_file(const Twine &model, int &result_fd, > - SmallVectorImpl<char> &result_path, > - bool makeAbsolute = true, unsigned mode = 0600); > + SmallVectorImpl<char> &result_path, > + bool makeAbsolute = true, > + bool private_file = true);Incorrect whitespace only change and tabs.> Index: include/llvm/Support/FileOutputBuffer.h > ==================================================================> --- include/llvm/Support/FileOutputBuffer.h (revision 0) > +++ include/llvm/Support/FileOutputBuffer.h (revision 0) > @@ -0,0 +1,97 @@ > +//=== FileOutputBuffer.h - File Output Buffer -------------------*- C++ -*-===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > +//===----------------------------------------------------------------------===// > +// > +// Utility for creating a in-memory buffer that will be written to a file. > +// > +//===----------------------------------------------------------------------===// > + > +#ifndef LLVM_SUPPORT_FILEOUTPUTBUFFER_H > +#define LLVM_SUPPORT_FILEOUTPUTBUFFER_H > + > +#include "llvm/Support/DataTypes.h" > +#include "llvm/ADT/StringRef.h" > +#include "llvm/ADT/SmallString.h" > + > +namespace llvm { > + > +class error_code; > +template<class T> class OwningPtr; > + > +/// FileOutputBuffer - This interface provides simple way to create an in-memory > +/// buffer which will be written to a file. During the lifetime of these > +/// objects, the content or existence of the specified file is undefined. That > +/// is, creating an OutputBuffer for a file may immediately remove the file. > +/// If the FileOutputBuffer is committed, the target file's content will become > +/// the buffer content at the time of the commit. If the FileOutputBuffer is > +/// not committed, the file will be deleted in the FileOutputBuffer destructor. > +class FileOutputBuffer { > +public: > + > + enum { > + F_executable = 1 /// set the 'x' bit on the resulting file > + }; > + > + /// Factory method to create an OutputBuffer object which manages a read/write > + /// buffer of the specified size. When committed, the buffer will be written > + /// to the file at the specified path. > + static error_code create(StringRef filePath, size_t size, > + OwningPtr<FileOutputBuffer> &result, > + unsigned flags=0);Arguments should start with an upper case letter. (same for all of these)> + /// Returns a pointer to the start of the buffer. > + uint8_t *getBufferStart() const { > + return bufferStart; > + } > + > + /// Returns a pointer to the end of the buffer. > + uint8_t *getBufferEnd() const { > + return bufferEnd; > + } > + > + /// Returns size of the buffer. > + size_t getBufferSize() const { > + return bufferEnd - bufferStart; > + } > + > + /// Returns path where file will show up if buffer is committed. > + StringRef getPath() const { > + return finalPath; > + } > + > + /// Flushes the content of the buffer to its file and deallocates the > + /// buffer. If commit() is not called before this object's destructor > + /// is called, the file is deleted in the destructor. The optional parameter > + /// is used if it turns out you want the file size to be smaller than > + /// initially requested. > + virtual error_code commit(int64_t newSmallerSize = -1);Why is this virtual? I don't ever see a need to subclass FileOutputBuffer.> + /// If this object was previously committed, the destructor just deletes > + /// this object. If this object was not committed, the destructor > + /// deallocates the buffer and the target file is never written. > + virtual ~FileOutputBuffer();Same.> + > + > +protected: > + FileOutputBuffer(const FileOutputBuffer &); // DO NOT IMPLEMENT > + FileOutputBuffer &operator=(const FileOutputBuffer &); // DO NOT IMPLEMENT > + FileOutputBuffer(uint8_t *start, uint8_t *end, > + StringRef path, StringRef tempPath); > + > + uint8_t *bufferStart; > + uint8_t *bufferEnd; > + SmallString<128> finalPath; > + SmallString<128> tempPath;These should all start with an uppercase letter.> +}; > + > + > + > +} // end namespace llvm > + > +#endif > Index: unittests/Support/FileOutputBufferTest.cpp > ==================================================================> --- unittests/Support/FileOutputBufferTest.cpp (revision 0) > +++ unittests/Support/FileOutputBufferTest.cpp (revision 0) > @@ -0,0 +1,131 @@ > +//===- llvm/unittest/Support/FileOutputBuffer.cpp - unit tests ------------===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > +//===----------------------------------------------------------------------===// > + > +#include "llvm/Support/FileSystem.h" > +#include "llvm/Support/PathV2.h" > +#include "llvm/Support/ErrorHandling.h" > +#include "llvm/Support/raw_ostream.h" > +#include "llvm/Support/FileOutputBuffer.h" > +#include "llvm/ADT/OwningPtr.h" > + > +#include "gtest/gtest.h" > + > +using namespace llvm; > +using namespace llvm::sys; > + > +#define ASSERT_NO_ERROR(x) \ > + if (error_code ASSERT_NO_ERROR_ec = x) { \ > + errs() << #x ": did not return errc::success.\n" \ > + << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n" \ > + << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n"; \ > + } else {} > + > +namespace { > + > + > +TEST(FileOutputBuffer, Test) { > + // Create unique temporary directory for these tests > + SmallString<128> TestDirectory; > + { > + int fd; > + ASSERT_NO_ERROR( > + fs::unique_file("FileOutputBuffer-test-%%-%%-%%-%%/dir", fd, > + TestDirectory)); > + ::close(fd); > + TestDirectory = path::parent_path(TestDirectory); > + } > + > + // TEST 1: Verify commit case. > + SmallString<128> File1(TestDirectory); > + File1.append("/file1"); > + { > + OwningPtr<FileOutputBuffer> Buffer; > + ASSERT_NO_ERROR(FileOutputBuffer::create(File1, 8192, Buffer)); > + // Start buffer with special header. > + memcpy(Buffer->getBufferStart(), "AABBCCDDEEFFGGHHIIJJ", 20); > + // Write to end of buffer to verify it is writable. > + memcpy(Buffer->getBufferEnd() - 20, "AABBCCDDEEFFGGHHIIJJ", 20); > + // Commit buffer. > + ASSERT_NO_ERROR(Buffer->commit()); > + } > + // Verify file exists and starts with special header. > + bool MagicMatches = false; > + ASSERT_NO_ERROR(fs::has_magic(Twine(File1), Twine("AABBCCDDEEFFGGHHIIJJ"), > + MagicMatches)); > + EXPECT_TRUE(MagicMatches); > + // Verify file is correct size. > + uint64_t File1Size; > + ASSERT_NO_ERROR(fs::file_size(Twine(File1), File1Size)); > + ASSERT_EQ(File1Size, 8192ULL); > + > + // TEST 2: Verify abort case. > + SmallString<128> File2(TestDirectory); > + File2.append("/file2"); > + { > + OwningPtr<FileOutputBuffer> Buffer2; > + ASSERT_NO_ERROR(FileOutputBuffer::create(File2, 8192, Buffer2)); > + // Fill buffer with special header. > + memcpy(Buffer2->getBufferStart(), "AABBCCDDEEFFGGHHIIJJ", 20); > + // Do *not* commit buffer. > + } > + // Verify file does not exist (because buffer not commited). > + bool Exists = false; > + ASSERT_NO_ERROR(fs::exists(Twine(File2), Exists)); > + EXPECT_FALSE(Exists); > + > + > + // TEST 3: Verify sizing down case. > + SmallString<128> File3(TestDirectory); > + File3.append("/file3"); > + { > + OwningPtr<FileOutputBuffer> Buffer; > + ASSERT_NO_ERROR(FileOutputBuffer::create(File3, 8192000, Buffer)); > + // Start buffer with special header. > + memcpy(Buffer->getBufferStart(), "AABBCCDDEEFFGGHHIIJJ", 20); > + // Write to end of buffer to verify it is writable. > + memcpy(Buffer->getBufferEnd() - 20, "AABBCCDDEEFFGGHHIIJJ", 20); > + // Commit buffer, but size down to smaller size > + ASSERT_NO_ERROR(Buffer->commit(5000)); > + } > + // Verify file exists and starts with special header. > + bool MagicMatches3 = false; > + ASSERT_NO_ERROR(fs::has_magic(Twine(File3), Twine("AABBCCDDEEFFGGHHIIJJ"), > + MagicMatches3)); > + EXPECT_TRUE(MagicMatches3); > + // Verify file is correct size. > + uint64_t File3Size; > + ASSERT_NO_ERROR(fs::file_size(Twine(File3), File3Size)); > + ASSERT_EQ(File3Size, 5000ULL); > + > + > + // TEST 4: Verify file can be made executable. > + SmallString<128> File4(TestDirectory); > + File4.append("/file4"); > + { > + OwningPtr<FileOutputBuffer> Buffer; > + ASSERT_NO_ERROR(FileOutputBuffer::create(File4, 8192, Buffer, > + FileOutputBuffer::F_executable)); > + // Start buffer with special header. > + memcpy(Buffer->getBufferStart(), "AABBCCDDEEFFGGHHIIJJ", 20); > + // Commit buffer. > + ASSERT_NO_ERROR(Buffer->commit()); > + } > + // Verify file exists and is executable. > + bool IsExecutable; > + ASSERT_NO_ERROR(fs::is_executable_file(Twine(File4), IsExecutable)); > + EXPECT_TRUE(IsExecutable); > + > + > + // Clean up. > + uint32_t RemovedCount; > + ASSERT_NO_ERROR(fs::remove_all(TestDirectory.str(), RemovedCount)); > +} > + > + > +} // anonymous namespace > Index: unittests/CMakeLists.txt > ==================================================================> --- unittests/CMakeLists.txt (revision 157010) > +++ unittests/CMakeLists.txt (working copy) > @@ -165,6 +165,7 @@ > Support/CommandLineTest.cpp > Support/ConstantRangeTest.cpp > Support/EndianTest.cpp > + Support/FileOutputBufferTest.cpp > Support/LeakDetectorTest.cpp > Support/MathExtrasTest.cpp > Support/Path.cpp > Index: lib/Support/Unix/PathV2.inc > ==================================================================> --- lib/Support/Unix/PathV2.inc (revision 157010) > +++ lib/Support/Unix/PathV2.inc (working copy) > @@ -24,6 +24,9 @@ > #if HAVE_FCNTL_H > #include <fcntl.h> > #endif > +#ifdef HAVE_SYS_MMAN_H > +#include <sys/mman.h> > +#endif > #if HAVE_DIRENT_H > # include <dirent.h> > # define NAMLEN(dirent) strlen((dirent)->d_name) > @@ -52,6 +55,7 @@ > # define PATH_MAX 4096 > #endif > > + > using namespace llvm; > > namespace { > @@ -347,10 +351,19 @@ > return error_code::success(); > } > > -// Since this is most often used for temporary files, mode defaults to 0600. > +static mode_t current_umask() { > + // There is no posix way to just get current umask. You > + // have to set it to something else, and get the "old" value. > + // So, get and reset to get the current umask value; > + mode_t mask = ::umask(0); > + ::umask(mask); > + return mask; > +} > + > + > error_code unique_file(const Twine &model, int &result_fd, > - SmallVectorImpl<char> &result_path, > - bool makeAbsolute, unsigned mode) { > + SmallVectorImpl<char> &result_path, > + bool makeAbsolute, bool private_file) {Incorrect whitespace change.> SmallString<128> Model; > model.toVector(Model); > // Null terminate. > @@ -380,7 +393,8 @@ > > // Try to open + create the file. > rety_open_create: > - int RandomFD = ::open(RandomPath.c_str(), O_RDWR | O_CREAT | O_EXCL, mode); > + int perms = (private_file ? 0600 : 0666) & ~(current_umask()); > + int RandomFD = ::open(RandomPath.c_str(), O_RDWR | O_CREAT | O_EXCL, perms); > if (RandomFD == -1) { > // If the file existed, try again, otherwise, error. > if (errno == errc::file_exists) > @@ -426,6 +440,28 @@ > return error_code::success(); > } > > + > +error_code unique_file_sized(const Twine &model, size_t size, > + SmallVectorImpl<char> &result_path, > + bool makeAbsolute) { > + int fd; > + if ( error_code ec = unique_file(model, fd, result_path, makeAbsolute, false) ) > + return ec; > + > + if ( ::ftruncate(fd, size) == -1 ) { > + int error = errno; > + result_path.push_back('\0');This is incorrect because result_path now actually includes a \0. I added a function c_str(container) specifically for this case that adds a 0 then removes it.> + ::unlink(result_path.begin()); > + ::close(fd); > + return error_code(error, system_category()); > + } > + > + ::close(fd); > + > + return error_code::success(); > +} > + > + > error_code detail::directory_iterator_construct(detail::DirIterState &it, > StringRef path){ > SmallString<128> path_null(path); > @@ -496,6 +532,97 @@ > return error_code::success(); > } > > +error_code map_file_pages(const Twine &path, off_t file_offset, size_t size, > + bool map_writable, void *&result) { > + SmallString<128> path_storage; > + StringRef name = path.toNullTerminatedStringRef(path_storage); > + int oflags = map_writable ? O_RDWR : O_RDONLY; > + int fd = ::open(name.begin(), oflags); > + if ( fd == -1 ) > + return error_code(errno, system_category()); > + int flags = map_writable ? MAP_SHARED : MAP_PRIVATE; > + int prot = map_writable ? (PROT_READ|PROT_WRITE) : PROT_READ; > +#ifdef MAP_FILE > + flags |= MAP_FILE; > +#endif > + result = ::mmap(0, size, prot, flags, fd, file_offset); > + if (result == MAP_FAILED) { > + ::close(fd); > + return error_code(errno, system_category()); > + } > + > + ::close(fd);Could AutoFD work here?> + return error_code::success(); > +} > + > +error_code unmap_file_pages(void *base, size_t size) { > + if ( ::munmap(base, size) == -1 ) > + return error_code(errno, system_category()); > + > + return error_code::success(); > +} > + > + > +error_code is_writable_file(const Twine &path, bool &result) { > + result = false; > + SmallString<128> path_storage; > + StringRef name = path.toNullTerminatedStringRef(path_storage); > + if ( ::access(name.begin(), W_OK) == -1 ) { > + int error = errno; > + if ( error == EACCES ) > + result = false; > + else > + return error_code(error, system_category()); > + } else { > + result = true; > + } > + return error_code::success(); > +} > + > + > +error_code is_executable_file(const Twine &path, bool &result) { > + result = false; > + SmallString<128> path_storage; > + StringRef name = path.toNullTerminatedStringRef(path_storage); > + if ( ::access(name.begin(), X_OK) == -1 ) { > + int error = errno; > + if ( error == EACCES ) > + result = false; > + else > + return error_code(error, system_category()); > + } else { > + result = true; > + } > + return error_code::success(); > +} > + > + > + > +error_code set_file_executable(const Twine &path) { > + SmallString<128> path_storage; > + StringRef name = path.toNullTerminatedStringRef(path_storage); > + > + struct stat buf; > + if ( stat(name.begin(), &buf) == -1 ) > + return error_code(errno, system_category()); > + > + // Only add 'x' bit where 'r' bit is already set > + mode_t perms = buf.st_mode; > + if ( perms & S_IRUSR ) > + perms |= S_IXUSR; > + if ( perms & S_IRGRP ) > + perms |= S_IXGRP; > + if ( perms & S_IROTH ) > + perms |= S_IXOTH; > + > + if ( ::chmod(name.begin(), perms) == -1) > + return error_code(errno, system_category()); > + > + return error_code::success(); > +} > + > + > + > } // end namespace fs > } // end namespace sys > } // end namespace llvm > Index: lib/Support/FileOutputBuffer.cpp > ==================================================================> --- lib/Support/FileOutputBuffer.cpp (revision 0) > +++ lib/Support/FileOutputBuffer.cpp (revision 0) > @@ -0,0 +1,127 @@ > +//===- FileOutputBuffer.cpp - File Output Buffer ----------------*- C++ -*-===// > +// > +// The LLVM Compiler Infrastructure > +// > +// This file is distributed under the University of Illinois Open Source > +// License. See LICENSE.TXT for details. > +// > +//===----------------------------------------------------------------------===// > +// > +// Utility for creating a in-memory buffer that will be written to a file. > +// > +//===----------------------------------------------------------------------===// > + > +#include "llvm/Support/FileOutputBuffer.h" > +#include "llvm/Support/system_error.h" > +#include "llvm/Support/FileSystem.h" > + > +#include "llvm/Support/raw_ostream.h" > + > +#include "llvm/ADT/SmallVector.h" > +#include "llvm/ADT/OwningPtr.h"These should be sorted alphabetically except for the main header.> + > +namespace llvm { > + > + > +FileOutputBuffer::FileOutputBuffer(uint8_t *start, uint8_t *end, > + StringRef path, StringRef tmpPath)Missing an alignment space.> + : bufferStart(start), bufferEnd(end) { > + finalPath.assign(path); > + tempPath.assign(tmpPath); > +} > + > + > +FileOutputBuffer::~FileOutputBuffer() { > + // If not already commited, delete buffer and remove temp file. > + if ( bufferStart != NULL ) { > + sys::fs::unmap_file_pages((void*)bufferStart, getBufferSize()); > + bool existed; > + sys::fs::remove(Twine(tempPath), existed); > + } > +} > + > + > + > +error_code FileOutputBuffer::create(StringRef filePath, > + size_t size, > + OwningPtr<FileOutputBuffer> &result, > + unsigned flags) {Alignment whitespace and capitalized first character.> + // If file already exists, it must be a regular file (to be mappable). > + Twine filePathTwine(filePath);Twine should not be used like this. It stores references to temporaries. It will work in this case because filePath is not a temporary and you haven't invoked any operators. It's also uneeded. StringRef is convertable to Twine.> + sys::fs::file_status stat; > + bool writable;This is only used in the case below, so it should be folded in there.> + error_code ec = sys::fs::status(filePathTwine, stat);stat is undefined if ec isn't success. ec will be success even in the case of file_not_found.> + switch ( stat.type() ) {No space after ( and before ). Same lots of other places.> + case sys::fs::file_type::file_not_found: > + // If file does not exist, we'll create one. > + break; > + case sys::fs::file_type::regular_file: > + // If file is not currently writable, error out. > + ec = sys::fs::is_writable_file(filePathTwine, writable); > + if ( !ec && !writable ) > + return make_error_code(errc::permission_denied); > + break; > + default: > + if ( ec ) > + return ec; > + else > + return make_error_code(errc::operation_not_permitted); > + } > + > + // Delete target file. > + bool existed; > + if ( error_code ec = sys::fs::remove(filePath, existed) ) > + return ec; > + > + // Create new file of specified size in same directory but with random name. > + SmallString<128> modelStr; > + modelStr.assign(filePath); > + modelStr.append(".tmp%%%%%%%"); > + SmallString<128> tempFilePath; > + if ( error_code ec = sys::fs::unique_file_sized(Twine(modelStr), size, > + tempFilePath, false) ) {This should be: Twine(filePath) + ".tmp%%%%%%%"> + return ec; > + }Unneeded block.> + > + // If requested, make the output file executable. > + if ( flags & F_executable )Tab.> + sys::fs::set_file_executable(Twine(tempFilePath)); > + > + // Memory map new file. > + void *base; > + if ( error_code ec = sys::fs::map_file_pages(Twine(tempFilePath), > + 0, size, true, base) ) { > + return ec; > + } > + > + // Create FileOutputBuffer object to own mapped range. > + uint8_t* start = (uint8_t*)base;* should be right aligned and the case should be a c++ cast.> + result.reset(new FileOutputBuffer(start, start+size, filePath, tempFilePath));Spaces around binary operator +.> + > + return error_code::success(); > +} > + > + > +error_code FileOutputBuffer::commit(int64_t newSmallerSize) { > + // Unmap buffer, letting OS flush dirty pages to file on disk. > + if ( error_code ec = sys::fs::unmap_file_pages((void*)bufferStart,c++ cast.> + getBufferSize()) ) { > + return ec; > + } > + > + // If requested, resize file as part of commit. > + if ( newSmallerSize != -1 ) { > + if ( error_code ec = sys::fs::resize_file(Twine(tempPath), newSmallerSize) ) > + return ec; > + } > + > + // Rename file to final name. > + if ( error_code ec = sys::fs::rename(Twine(tempPath), Twine(finalPath)) ) > + return ec; > + > + return error_code::success(); > +} > + > + > +} // namespace > + > Index: lib/Support/CMakeLists.txt > ==================================================================> --- lib/Support/CMakeLists.txt (revision 157010) > +++ lib/Support/CMakeLists.txt (working copy) > @@ -23,6 +23,7 @@ > Dwarf.cpp > ErrorHandling.cpp > FileUtilities.cpp > + FileOutputBuffer.cpp > FoldingSet.cpp > FormattedStream.cpp > GraphWriter.cpp > Index: lib/Support/Windows/PathV2.inc > ==================================================================> --- lib/Support/Windows/PathV2.inc (revision 157010) > +++ lib/Support/Windows/PathV2.inc (working copy) > @@ -501,7 +501,7 @@ > // it currently comes in as a UNIX mode. > error_code unique_file(const Twine &model, int &result_fd, > SmallVectorImpl<char> &result_path, > - bool makeAbsolute, unsigned mode) { > + bool makeAbsolute, bool private_file) { > // Use result_path as temp storage. > result_path.set_size(0); > StringRef m = model.toStringRef(result_path); > @@ -755,6 +755,43 @@ > return error_code::success(); > } > > +error_code map_file_pages(const Twine &path, off_t file_offset, size_t size, > + bool map_writable, void *&result) { > + assert(0 && "NOT IMPLEMENTED"); > +} > + > +error_code unmap_file_pages(void *base, size_t size) { > + assert(0 && "NOT IMPLEMENTED"); > +} > + > +error_code is_writable_file(const Twine &path, bool &result) { > +#if 0 // verify code below before enabling: > + SmallString<128> path_storage; > + StringRef name = path.toNullTerminatedStringRef(path_storage); > + > + DWORD attr = GetFileAttributes(name.begin()); > + > + if (attr == INVALID_FILE_ATTRIBUTES) > + return windows_error(::GetLastError()); > + > + // Files are writable by default, unless marked READONLY > + result = !(attr & FILE_ATTRIBUTE_READONLY); > + #endifThis looks right, the only thing it's missing is symlink handling.> + return error_code::success(); > +} > + > +error_code is_executable_file(const Twine &path, bool &result) { > + // All files are executable on Windows (ignoring security attributes). > + result = true; > + return error_code::success(); > +} > + > +error_code set_file_executable(const Twine &path) { > + // All files are executable on Windows (ignoring security attributes). > + return error_code::success(); > +} > + > + > } // end namespace fs > } // end namespace sys > } // end namespace llvm >
Michael Spencer
2012-May-18 22:22 UTC
[LLVMdev] [RFC] llvm/include/Support/FileOutputBuffer.h
On Fri, May 18, 2012 at 3:07 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:> >> + error_code ec = sys::fs::status(filePathTwine, stat); > > stat is undefined if ec isn't success. ec will be success even in the case of > file_not_found.Actually I was wrong. The Windows and UNIX implementation disagree on this point. I'm going to change it to match http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3365.html#status error_code semantics.
Nick Kledzik
2012-May-22 21:47 UTC
[LLVMdev] [RFC] llvm/include/Support/FileOutputBuffer.h
On May 18, 2012, at 3:07 PM, Michael Spencer wrote:> On Thu, May 17, 2012 at 3:25 PM, Nick Kledzik <kledzik at apple.com> wrote: >> I now have an implementation of FileOutputBuffer (OutputBuffer was already taken). The patch supports the functionality listed below and I've tested that it works for lld. >> >> To implement the FileOutputBuffer, I needed to add some more functions to llvm/Support/FileSystem.h, including: >> is_writable_file() >> is_executable_file() >> set_file_executable() > > For these parts I would like to follow > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3365.html#TOC > which is what the rest of the fs library is modeled after. So we would > use: > > error_code permissions(const Twine &path, perms p); > > If the name is confusing (it says nothing about modification) I'm fine > with modify_permissions or something similar.Oh my! I thought you were trying to create an abstraction that could be mapped onto unix or windows. That spec is just a posix interface, but with different names. There is even a table that shows the mapping of all the bit values (e.g. group_read == 040 == S_IRGRP. Seems like it would be less complicated to just implement the posix call interface on windows, than to create all these new names that add no value…>> unique_file_sized() > > Instead of adding this I would much rather add resize_file and call it > after unique_file.The problem is that the existing unique_file() function creates the file and returns the file-descriptor. My level does not need that file-descriptor and there is no close() function available in this N3365 spec.> >> map_file_pages() >> unmap_file_pages() > > These are good.But if you are really trying to model N3365, should these go elsewhere?>> + /// Flushes the content of the buffer to its file and deallocates the >> + /// buffer. If commit() is not called before this object's destructor >> + /// is called, the file is deleted in the destructor. The optional parameter >> + /// is used if it turns out you want the file size to be smaller than >> + /// initially requested. >> + virtual error_code commit(int64_t newSmallerSize = -1); > > Why is this virtual? I don't ever see a need to subclass FileOutputBuffer. > >> + /// If this object was previously committed, the destructor just deletes >> + /// this object. If this object was not committed, the destructor >> + /// deallocates the buffer and the target file is never written. >> + virtual ~FileOutputBuffer(); > > Same.I was anticipating there might be alternate implementations - like MemoryBuffer. I'll remove the virtual keyword.>> +error_code unique_file_sized(const Twine &model, size_t size, >> + SmallVectorImpl<char> &result_path, >> + bool makeAbsolute) { >> + int fd; >> + if ( error_code ec = unique_file(model, fd, result_path, makeAbsolute, false) ) >> + return ec; >> + >> + if ( ::ftruncate(fd, size) == -1 ) { >> + int error = errno; >> + result_path.push_back('\0'); > > This is incorrect because result_path now actually includes a \0. I added a > function c_str(container) specifically for this case that adds a 0 then removes > it.c_str() is only available on SmallString<>, not on its parent SmallVectorImpl<char>.>> +error_code map_file_pages(const Twine &path, off_t file_offset, size_t size, >> + bool map_writable, void *&result) { >> + SmallString<128> path_storage; >> + StringRef name = path.toNullTerminatedStringRef(path_storage); >> + int oflags = map_writable ? O_RDWR : O_RDONLY; >> + int fd = ::open(name.begin(), oflags); >> + if ( fd == -1 ) >> + return error_code(errno, system_category()); >> + int flags = map_writable ? MAP_SHARED : MAP_PRIVATE; >> + int prot = map_writable ? (PROT_READ|PROT_WRITE) : PROT_READ; >> +#ifdef MAP_FILE >> + flags |= MAP_FILE; >> +#endif >> + result = ::mmap(0, size, prot, flags, fd, file_offset); >> + if (result == MAP_FAILED) { >> + ::close(fd); >> + return error_code(errno, system_category()); >> + } >> + >> + ::close(fd); > > Could AutoFD work here?Yes!>> +//===- FileOutputBuffer.cpp - File Output Buffer ----------------*- C++ -*-===// >> +// >> +// The LLVM Compiler Infrastructure >> +// >> +// This file is distributed under the University of Illinois Open Source >> +// License. See LICENSE.TXT for details. >> +// >> +//===----------------------------------------------------------------------===// >> +// >> +// Utility for creating a in-memory buffer that will be written to a file. >> +// >> +//===----------------------------------------------------------------------===// >> + >> +#include "llvm/Support/FileOutputBuffer.h" >> +#include "llvm/Support/system_error.h" >> +#include "llvm/Support/FileSystem.h" >> + >> +#include "llvm/Support/raw_ostream.h" >> + >> +#include "llvm/ADT/SmallVector.h" >> +#include "llvm/ADT/OwningPtr.h" > > These should be sorted alphabetically except for the main header.Ok.> > >> + // If file already exists, it must be a regular file (to be mappable). >> + Twine filePathTwine(filePath); > > Twine should not be used like this. It stores references to temporaries. It will > work in this case because filePath is not a temporary and you haven't invoked > any operators. It's also uneeded. StringRef is convertable to Twine. > >> + sys::fs::file_status stat; >> + bool writable; > > This is only used in the case below, so it should be folded in there.Fixed.>> + >> + // Create new file of specified size in same directory but with random name. >> + SmallString<128> modelStr; >> + modelStr.assign(filePath); >> + modelStr.append(".tmp%%%%%%%"); >> + SmallString<128> tempFilePath; >> + if ( error_code ec = sys::fs::unique_file_sized(Twine(modelStr), size, >> + tempFilePath, false) ) { > > This should be: Twine(filePath) + ".tmp%%%%%%%"Yes, that will save some copying from happening.> >> + sys::fs::set_file_executable(Twine(tempFilePath)); >> + >> + // Memory map new file. >> + void *base; >> + if ( error_code ec = sys::fs::map_file_pages(Twine(tempFilePath), >> + 0, size, true, base) ) { >> + return ec; >> + } >> + >> + // Create FileOutputBuffer object to own mapped range. >> + uint8_t* start = (uint8_t*)base; > > * should be right aligned and the case should be a c++ cast. > >> + >> + return error_code::success(); >> +} >> + >> + >> +error_code FileOutputBuffer::commit(int64_t newSmallerSize) { >> + // Unmap buffer, letting OS flush dirty pages to file on disk. >> + if ( error_code ec = sys::fs::unmap_file_pages((void*)bufferStart, > > c++ cast.Fixed.