Make sure DataBufferLLVM contents are writable

[labath]

Summary

We sometimes need to write to the object file we've mapped into memory,
generally to apply relocations to debug info sections. We've had that
ability before, but with the introduction of DataBufferLLVM, we have
lost it, as the underlying llvm class (MemoryBuffer) only supports
read-only mappings. The way DataBufferLLVM implemented writable buffers
was through the Private=true flag, which causes the file contents to be
read into a malloc()ed buffer.

This wasn't a problem then, since we weren't aware of any cases where we
need to relocate the debug info, and we just always set Private=false
for the cases where we mmapped the full object file. This has changed in
D38142, where needed to relocate the FreeBSD kernel, and we've flipped
the private flag to true, without even realising the full impact of
that. (The impact is significant slowdown in debug info loading and a
huge number of dirty pages).

This (not particularly elegant) commit tries to restore the performance
and bring down memory usage by bringing back the private copy-on-write
file mappings. It does this by falling back to the lower-level llvm
primitive (mapped_file_region), which supports this type of mappings.
Unfortunately, this primitive does not support non-mmapped regions, so
for volatile regions I have to still use the MemoryBuffer class.

I have also removed the "Private" argument to DataBufferLLVM factory
functions -- what it really meant was "read-only", but we generally
assume all our databuffers are writable. If we ever want to have
read-only buffers, we can bring it back with an appropriate name.

[clayborg]

I had suggested in D38142 that we have ObjectFileELF check the file type of the file and only map with write abilities if the ELF file is an object file since that is the only time we need relocations. If we can pass a flag through to indicate how to map, would that provide any additional performance improvements?

[labath]

In D40079#926328, @clayborg wrote:

I had suggested in D38142 that we have ObjectFileELF check the file type of the file and only map with write abilities if the ELF file is an object file since that is the only time we need relocations. If we can pass a flag through to indicate how to map, would that provide any additional performance improvements?

Sorry, I hadn't seen that comment.

But to answer to question, it would not provide *additional* performance improvements -- the memory will still be shared until you actually try to write to it (with the read-only shared mapping you'll get a SEGV, with a private read-write mapping, the page will be COWed).

However, I suppose something like that could be done *instead* of this patch. That way we would be using mmap for most object files, and only resorting to malloc+read for those that need relocating. It's not an ideal solution, but then again, neither is this.

[zturner]

On second thought I actually think the strategy used here is fine. But we have llvm::WritableBinaryStream which could server as a single base interface for a mapped_file_region based implementation as well as a malloc-based implementation.

For the malloc based implementation, we already have WritableBinaryByteStream, so you don't even need to implement that.

The nice thing about this is that it could later be improved to use the stream reader / stream writer idiom, which gets rid of a ton of manual pointer arithmetic and management.

[labath]

It took a while, but we've finally landed an llvm::WritableMemoryBuffer class.
This patch now becomes simple, as all I need to do is use that instead of the
MemoryBuffer class.

Well.. almost. The new class does not have the null-termination capability, so I
also update our buffer uses to not require null termination.

[clayborg]

See inlined comment. Quick fix and this will be good to go.

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

407–409

We should put the DataBufferLLVM::CreateSliceFromPath() call into a new static function in ObjectFile:

static DataBufferSP ObjectFile::MapFileData(FileSpec file, uint64_t Size, uint64_t Offset);

Then when we need to make fixes, we change just one place instead of all of them. We should add this new function and switch all ObjectFile subclasses over to using it.

[labath]

Add the suggested MapFileData function

[clayborg]

Looks good, thanks for the making the changes. This will make future tweaks to reading of file data in object files just a single change in ObjectFile.cpp.

include/lldb/Utility/DataBufferLLVM.h

 //===--- DataBufferLLVM.h ---------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLDB_CORE_DATABUFFERLLVM_H
-#define LLDB_CORE_DATABUFFERLLVM_H
+#ifndef LLDB_UTILITY_DATABUFFERLLVM_H
+#define LLDB_UTILITY_DATABUFFERLLVM_H
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/lldb-types.h" // for offset_t
 
 #include <memory>
 #include <stdint.h> // for uint8_t, uint64_t
 
 namespace llvm {
-class MemoryBuffer;
 class Twine;
 }
 
 namespace lldb_private {
 
 class DataBufferLLVM : public DataBuffer {
 public:
   ~DataBufferLLVM();
 
   static std::shared_ptr<DataBufferLLVM>
-  CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size, uint64_t Offset, bool Private = false);
+  CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size, uint64_t Offset);
 
   static std::shared_ptr<DataBufferLLVM>
-  CreateFromPath(const llvm::Twine &Path, bool NullTerminate = false, bool Private = false);
-
-  uint8_t *GetBytes() override;
-  const uint8_t *GetBytes() const override;
-  lldb::offset_t GetByteSize() const override;
+  CreateFromPath(const llvm::Twine &Path, bool NullTerminate = false);
 
   char *GetChars() { return reinterpret_cast<char *>(GetBytes()); }
-
-private:
-  /// \brief Construct a DataBufferLLVM from \p Buffer.  \p Buffer must be a
-  /// valid pointer.
-  explicit DataBufferLLVM(std::unique_ptr<llvm::MemoryBuffer> Buffer);
-  const uint8_t *GetBuffer() const;
-
-  std::unique_ptr<llvm::MemoryBuffer> Buffer;
 };
 }
 
 #endif

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

 
 ObjectFile *ObjectFileELF::CreateInstance(const lldb::ModuleSP &module_sp,
                                           DataBufferSP &data_sp,
                                           lldb::offset_t data_offset,
                                           const lldb_private::FileSpec *file,
                                           lldb::offset_t file_offset,
                                           lldb::offset_t length) {
   if (!data_sp) {
-    data_sp =
-        DataBufferLLVM::CreateSliceFromPath(file->GetPath(), length, file_offset, true);
+    data_sp = DataBufferLLVM::CreateSliceFromPath(file->GetPath(), length,
+                                                  file_offset);
     if (!data_sp)
       return nullptr;
     data_offset = 0;
   }
 
   assert(data_sp);
 
   if (data_sp->GetByteSize() <= (llvm::ELF::EI_NIDENT + data_offset))
     return nullptr;
 
   const uint8_t *magic = data_sp->GetBytes() + data_offset;
   if (!ELFHeader::MagicBytesMatch(magic))
     return nullptr;
 
   // Update the data to contain the entire file if it doesn't already
   if (data_sp->GetByteSize() < length) {
-    data_sp =
-        DataBufferLLVM::CreateSliceFromPath(file->GetPath(), length, file_offset, true);
+    data_sp = DataBufferLLVM::CreateSliceFromPath(file->GetPath(), length,
+                                                  file_offset);
     if (!data_sp)
       return nullptr;
     data_offset = 0;
     magic = data_sp->GetBytes();
   }
 
   unsigned address_size = ELFHeader::AddressSizeInBytes(magic);
   if (address_size == 4 || address_size == 8) {

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

bool ObjectFilePECOFF::ParseCOFFOptionalHeader(lldb::offset_t *offset_ptr) {
   return success;
 }
 
 DataExtractor ObjectFilePECOFF::ReadImageData(uint32_t offset, size_t size) {
   if (m_file) {
     // A bit of a hack, but we intend to write to this buffer, so we can't 
     // mmap it.
     auto buffer_sp =
-        DataBufferLLVM::CreateSliceFromPath(m_file.GetPath(), size, offset, true);
+        DataBufferLLVM::CreateSliceFromPath(m_file.GetPath(), size, offset);
     return DataExtractor(buffer_sp, GetByteOrder(), GetAddressByteSize());
   }
   ProcessSP process_sp(m_process_wp.lock());
   DataExtractor data;
   if (process_sp) {
     auto data_ap = llvm::make_unique<DataBufferHeap>(size, 0);
     Status readmem_error;
     size_t bytes_read =

source/Utility/DataBufferLLVM.cpp

 //===--- DataBufferLLVM.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/DataBufferLLVM.h"
+#include "lldb/Utility/CleanUp.h"
+#include "lldb/Utility/Log.h"
 
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 #include <assert.h>    // for assert
 #include <type_traits> // for move
 
+#if !defined(_MSC_VER) && !defined(__MINGW32__)
+#include <unistd.h>
+#else
+#include <io.h>
+#endif
+
 using namespace lldb_private;
 
-DataBufferLLVM::DataBufferLLVM(std::unique_ptr<llvm::MemoryBuffer> MemBuffer)
-    : Buffer(std::move(MemBuffer)) {
-  assert(Buffer != nullptr &&
-         "Cannot construct a DataBufferLLVM with a null buffer");
+namespace {
+class MMapBuffer : public DataBufferLLVM {
+public:
+  static std::shared_ptr<DataBufferLLVM>
+  CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size, uint64_t Offset);
+
+  uint8_t *GetBytes() override { return Start; }
+  const uint8_t *GetBytes() const override { return Start; }
+  lldb::offset_t GetByteSize() const override { return Len; }
+
+private:
+  llvm::sys::fs::mapped_file_region MFR;
+  uint8_t *Start;
+  lldb::offset_t Len;
+
+  static uint64_t getLegalMapOffset(uint64_t Offset) {
+    return Offset & ~(llvm::sys::fs::mapped_file_region::alignment() - 1);
   }
 
-DataBufferLLVM::~DataBufferLLVM() {}
+  static uint64_t getLegalMapSize(uint64_t Len, uint64_t Offset) {
+    return Len + (Offset - getLegalMapOffset(Offset));
+  }
 
-std::shared_ptr<DataBufferLLVM>
-DataBufferLLVM::CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size,
-                               uint64_t Offset, bool Private) {
-  // If the file resides non-locally, pass the volatile flag so that we don't
-  // mmap it.
-  if (!Private)
-    Private = !llvm::sys::fs::is_local(Path);
+  MMapBuffer(int FD, uint64_t Len, uint64_t Offset, std::error_code &EC)
+      : MFR(FD, llvm::sys::fs::mapped_file_region::priv,
+            getLegalMapSize(Len, Offset), getLegalMapOffset(Offset), EC),
+        Start(nullptr), Len(Len) {
+    if (!EC)
+      Start = reinterpret_cast<uint8_t *>(getStart(Len, Offset));
+  }
 
-  auto Buffer = llvm::MemoryBuffer::getFileSlice(Path, Size, Offset, Private);
-  if (!Buffer)
-    return nullptr;
-  return std::shared_ptr<DataBufferLLVM>(
-      new DataBufferLLVM(std::move(*Buffer)));
+  char *getStart(uint64_t Len, uint64_t Offset) {
+    return MFR.data() + (Offset - getLegalMapOffset(Offset));
+  }
+};
+
+class MemoryBuffer : public DataBufferLLVM {
+public:
+  /// \brief Construct a MemoryBuffer from \p Buffer.  \p Buffer must be a
+  /// valid pointer.
+  explicit MemoryBuffer(std::unique_ptr<llvm::MemoryBuffer> Buffer);
+
+  uint8_t *GetBytes() override { return const_cast<uint8_t *>(GetBuffer()); }
+  const uint8_t *GetBytes() const override { return GetBuffer(); }
+
+  lldb::offset_t GetByteSize() const override {
+    return Buffer->getBufferSize();
+  }
+
+private:
+  std::unique_ptr<llvm::MemoryBuffer> Buffer;
+
+  const uint8_t *GetBuffer() const {
+    return reinterpret_cast<const uint8_t *>(Buffer->getBufferStart());
+  }
+};
 }
 
 std::shared_ptr<DataBufferLLVM>
-DataBufferLLVM::CreateFromPath(const llvm::Twine &Path, bool NullTerminate, bool Private) {
-  // If the file resides non-locally, pass the volatile flag so that we don't
-  // mmap it.
-  if (!Private)
-    Private = !llvm::sys::fs::is_local(Path);
+MMapBuffer::CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size,
+                                uint64_t Offset) {
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
+  int FD;
+  if (std::error_code EC = llvm::sys::fs::openFileForRead(Path, FD)) {
+    LLDB_LOG(log, "Failed to open file `{0}`: {1}", Path, EC.message());
+    return nullptr;
+  }
+  lldb_utility::CleanUp<int, int> CleanUpFD(FD, -1, close);
+
+  if (Size == uint64_t(-1)) {
+    llvm::sys::fs::file_status Status;
+    if (std::error_code EC = llvm::sys::fs::status(FD, Status)) {
+      LLDB_LOG(log, "Failed to stat file `{0}`: {1}", Path, EC.message());
+      return nullptr;
+    }
+    Size = Status.getSize();
+  }
 
-  auto Buffer = llvm::MemoryBuffer::getFile(Path, -1, NullTerminate, Private);
-  if (!Buffer)
+  std::error_code EC;
+  std::shared_ptr<DataBufferLLVM> SP(new MMapBuffer(FD, Size, Offset, EC));
+  if (EC) {
+    LLDB_LOG(log, "Failed to mmap file `{0}`: {1}", Path, EC.message());
     return nullptr;
-  return std::shared_ptr<DataBufferLLVM>(
-      new DataBufferLLVM(std::move(*Buffer)));
+  }
+  return SP;
 }
 
-uint8_t *DataBufferLLVM::GetBytes() {
-  return const_cast<uint8_t *>(GetBuffer());
+MemoryBuffer::MemoryBuffer(std::unique_ptr<llvm::MemoryBuffer> MemBuffer)
+    : Buffer(std::move(MemBuffer)) {
+  assert(Buffer && "Cannot construct a MemoryBuffer with a null buffer");
 }
 
-const uint8_t *DataBufferLLVM::GetBytes() const { return GetBuffer(); }
+DataBufferLLVM::~DataBufferLLVM() = default;
 
-lldb::offset_t DataBufferLLVM::GetByteSize() const {
-  return Buffer->getBufferSize();
+std::shared_ptr<DataBufferLLVM>
+DataBufferLLVM::CreateSliceFromPath(const llvm::Twine &Path, uint64_t Size,
+                                    uint64_t Offset) {
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
+  const bool IsVolatile = !llvm::sys::fs::is_local(Path);
+  if (IsVolatile) {
+    auto Buffer =
+        llvm::MemoryBuffer::getFileSlice(Path, Size, Offset, IsVolatile);
+    if (!Buffer) {
+      LLDB_LOG(log, "Failed to read file `{0}`: {1}", Path,
+               Buffer.getError().message());
+      return nullptr;
+    }
+    return std::make_shared<MemoryBuffer>(std::move(*Buffer));
   }
 
-const uint8_t *DataBufferLLVM::GetBuffer() const {
-  return reinterpret_cast<const uint8_t *>(Buffer->getBufferStart());
+  return MMapBuffer::CreateSliceFromPath(Path, Size, Offset);
+}
+
+std::shared_ptr<DataBufferLLVM>
+DataBufferLLVM::CreateFromPath(const llvm::Twine &Path, bool NullTerminate) {
+  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
+  const bool IsVolatile = !llvm::sys::fs::is_local(Path);
+  if (IsVolatile || NullTerminate) {
+    auto Buffer =
+        llvm::MemoryBuffer::getFile(Path, -1, NullTerminate, IsVolatile);
+    if (!Buffer) {
+      LLDB_LOG(log, "Failed to read file `{0}`: {1}", Path,
+               Buffer.getError().message());
+      return nullptr;
+    }
+    return std::make_shared<MemoryBuffer>(std::move(*Buffer));
+  }
+
+  return MMapBuffer::CreateSliceFromPath(Path, -1, 0);
 }