llvm dev - May 2012 - [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.
>>>>

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.
>
> -Nick
As 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);
> + #endif
This 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
>

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.

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.