[LLVMdev] [RFC] llvm/include/Support/FileOutputBuffer.h

[Nick Kledzik]

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.