[Support] Add MemoryBuffer::getFileSlice()

[kledzik]

Summary

mach-o supports "fat" files which are a header/table-of-contents followed by a concatenation of mach-o files of different architectures. Currently, MemoryBuffer has no easy way to map a subrange (slice) of a file which lld will need to select a mach-o slice of a fat file. The new function provides an easy way to map a slice of a file into a MemoryBuffer. Test case included.

[silvas]

What are the downsides to mapping the whole file and then taking a StringRef to the sub-file?

[kledzik]

There are two issues:

1. Apple has hundreds of dylibs and they are usually fat. A 32-bit process could run out of address space if it had to map the entirety of every fat file.

2. The lld the structure is that the InputGraphNode maps the file, then some Reader claims it and parses it. The parsing may or may not have pointers into the original buffer. If it does, the created lld::File object takes ownership of the buffer as part of parsing. If not, the ownership stays with the InputGraphNode which quickly frees it. In that model, you cannot pass down a StringRef because that cannot transfer ownership of the original MemoryBuffer. If MemoryBuffer had getFileSlice(), then the darwin InputGraphNode can check if the file is fat, and if so, only map the relevant slice. That would solve the address space issue and fit into the lld MemoryBuffer ownership model.

[• rafael]

I think this is a good idea. It is perfectly reasonable for a tool to know that it needs only a part of a file and let the OS unmap the rest.

I noticed some horrible inconsistencies in the pre existing code and fixed them. I should hopefully make this change a bit simpler.

include/llvm/Support/MemoryBuffer.h

135

For consistency with getOpenFileSlice, please make this

getFileSlice(const Twine &Filename, uint64_t MapSize,  int64_t Offset);

or update getOpenFileSlice.

lib/Support/MemoryBuffer.cpp

174

Can't you just add an MapSize argument to getFileAux?

[kledzik]

Rework to use getFileAux()

[• rafael]

include/llvm/Support/MemoryBuffer.h

131

s/Length/MapSize/

Pass MapSize before Offset.

Alternatively, update getOpenFileSlice to have this order/name. My only preference is that they match.

lib/Support/MemoryBuffer.cpp

335

Why was it necessary to move this?

Currently if the user passes the MapSize and we don't require a null terminator, we never call stat in here, since we don't need the file size.

I now notice that we call stat in mapped_file_region, which is a regression from when we transitioned to mapped_file_region :-(

In any case , if we can avoid an extra stat we should.

[kledzik]

include/llvm/Support/MemoryBuffer.h

131

Fixed to match getOpenFileSlice

lib/Support/MemoryBuffer.cpp

335

My thought was that since getFileSlice() provides a MapSize but not FileSize, we need to determine the file size.

But it seems we don't actually need the FileSize when mapping. The stat() looks like it is there to set the FileSize, but it is really there to get the MapSize.

[kledzik]

fixes from latest Rafael comments.

[• rafael]

LGTM

[kledzik]

committed in r219260

include/llvm/Support/MemoryBuffer.h

   /// Read all of stdin into a file buffer, and return it.
   static ErrorOr<std::unique_ptr<MemoryBuffer>> getSTDIN();
 
   /// Open the specified file as a MemoryBuffer, or open stdin if the Filename
   /// is "-".
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFileOrSTDIN(const Twine &Filename, int64_t FileSize = -1);
 
+  /// Map a subrange of the the specified file as a MemoryBuffer.
+  static ErrorOr<std::unique_ptr<MemoryBuffer>>
+  getFileSlice(const Twine &Filename, uint64_t MapSize, uint64_t Offset);
+
   //===--------------------------------------------------------------------===//
   // Provided for performance analysis.
   //===--------------------------------------------------------------------===//
 
   /// The kind of memory backing used to support the MemoryBuffer.
   enum BufferKind {
     MemoryBuffer_Malloc,
     MemoryBuffer_MMap
   };
 
   /// Return information on the memory mechanism used to support the

lib/Support/MemoryBuffer.cpp

   }
 
   BufferKind getBufferKind() const override {
     return MemoryBuffer_Malloc;
   }
 };
 }
 
+static ErrorOr<std::unique_ptr<MemoryBuffer>>
+getFileAux(const Twine &Filename, int64_t FileSize, uint64_t MapSize, 
+           uint64_t Offset, bool RequiresNullTerminator, bool IsVolatileSize);
+
 std::unique_ptr<MemoryBuffer>
 MemoryBuffer::getMemBuffer(StringRef InputData, StringRef BufferName,
                            bool RequiresNullTerminator) {
   auto *Ret = new (NamedBufferAlloc(BufferName))
       MemoryBufferMem(InputData, RequiresNullTerminator);
   return std::unique_ptr<MemoryBuffer>(Ret);
 }
 

   SmallString<256> NameBuf;
   StringRef NameRef = Filename.toStringRef(NameBuf);
 
   if (NameRef == "-")
     return getSTDIN();
   return getFile(Filename, FileSize);
 }
 
+ErrorOr<std::unique_ptr<MemoryBuffer>>
+MemoryBuffer::getFileSlice(const Twine &FilePath, uint64_t MapSize, 
+                           uint64_t Offset) {
+  return getFileAux(FilePath, -1, MapSize, Offset, false, false);
+}
+
 
 //===----------------------------------------------------------------------===//
 // MemoryBuffer::getFile implementation.
 //===----------------------------------------------------------------------===//
 
 namespace {
 /// \brief Memory maps a file descriptor using sys::fs::mapped_file_region.
 ///

       return std::error_code(errno, std::generic_category());
     }
     Buffer.set_size(Buffer.size() + ReadBytes);
   } while (ReadBytes != 0);
 
   return MemoryBuffer::getMemBufferCopy(Buffer, BufferName);
 }
 
-static ErrorOr<std::unique_ptr<MemoryBuffer>>
-getFileAux(const Twine &Filename, int64_t FileSize, bool RequiresNullTerminator,
-           bool IsVolatileSize);
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getFile(const Twine &Filename, int64_t FileSize,
                       bool RequiresNullTerminator, bool IsVolatileSize) {
-  return getFileAux(Filename, FileSize, RequiresNullTerminator,
-                    IsVolatileSize);
+  return getFileAux(Filename, FileSize, FileSize, 0,
+                    RequiresNullTerminator, IsVolatileSize);
 }
 
 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getOpenFileImpl(int FD, const Twine &Filename, uint64_t FileSize,
                 uint64_t MapSize, int64_t Offset, bool RequiresNullTerminator,
                 bool IsVolatileSize);
 
 static ErrorOr<std::unique_ptr<MemoryBuffer>>
-getFileAux(const Twine &Filename, int64_t FileSize, bool RequiresNullTerminator,
-           bool IsVolatileSize) {
+getFileAux(const Twine &Filename, int64_t FileSize, uint64_t MapSize,
+           uint64_t Offset, bool RequiresNullTerminator, bool IsVolatileSize) {
   int FD;
   std::error_code EC = sys::fs::openFileForRead(Filename, FD);
   if (EC)
     return EC;
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> Ret =
-      getOpenFileImpl(FD, Filename, FileSize, FileSize, 0,
+      getOpenFileImpl(FD, Filename, FileSize, MapSize, Offset,
                       RequiresNullTerminator, IsVolatileSize);
   close(FD);
   return Ret;
 }
 
 static bool shouldUseMmap(int FD,
                           size_t FileSize,
                           size_t MapSize,

     return false;
 
   if (!RequiresNullTerminator)
     return true;
 
 
   // If we don't know the file size, use fstat to find out.  fstat on an open
   // file descriptor is cheaper than stat on a random path.
   // FIXME: this chunk of code is duplicated, but it avoids a fstat when

unittests/Support/MemoryBufferTest.cpp

 TEST_F(MemoryBufferTest, getOpenFileNoReopen) {
   testGetOpenFileSlice(false);
 }
 
 TEST_F(MemoryBufferTest, getOpenFileReopened) {
   testGetOpenFileSlice(true);
 }
 
+
+TEST_F(MemoryBufferTest, slice) {
+  // Create a file that is six pages long with different data on each page.
+  int FD;
+  SmallString<64> TestPath;
+  sys::fs::createTemporaryFile("MemoryBufferTest_Slice", "temp", FD, TestPath);
+  raw_fd_ostream OF(FD, true, /*unbuffered=*/true);
+  for (unsigned i = 0; i < 0x2000 / 8; ++i) {
+    OF << "12345678";
+  }
+  for (unsigned i = 0; i < 0x2000 / 8; ++i) {
+    OF << "abcdefgh";
+  }
+  for (unsigned i = 0; i < 0x2000 / 8; ++i) {
+    OF << "ABCDEFGH";
+  }
+  OF.close();
+
+  // Try offset of one page.
+  ErrorOr<OwningBuffer> MB = MemoryBuffer::getFileSlice(TestPath.str(),
+                                                        0x4000, 0x1000);
+  std::error_code EC = MB.getError();
+  ASSERT_FALSE(EC);
+  EXPECT_EQ(0x4000UL, MB.get()->getBufferSize());
+ 
+  StringRef BufData = MB.get()->getBuffer();
+  EXPECT_TRUE(BufData.substr(0x0000,8).equals("12345678"));
+  EXPECT_TRUE(BufData.substr(0x0FF8,8).equals("12345678"));
+  EXPECT_TRUE(BufData.substr(0x1000,8).equals("abcdefgh"));
+  EXPECT_TRUE(BufData.substr(0x2FF8,8).equals("abcdefgh"));
+  EXPECT_TRUE(BufData.substr(0x3000,8).equals("ABCDEFGH"));
+  EXPECT_TRUE(BufData.substr(0x3FF8,8).equals("ABCDEFGH"));
+   
+  // Try non-page aligned.
+  ErrorOr<OwningBuffer> MB2 = MemoryBuffer::getFileSlice(TestPath.str(),
+                                                         0x3000, 0x0800);
+  EC = MB2.getError();
+  ASSERT_FALSE(EC);
+  EXPECT_EQ(0x3000UL, MB2.get()->getBufferSize());
+  
+  StringRef BufData2 = MB2.get()->getBuffer();
+  EXPECT_TRUE(BufData2.substr(0x0000,8).equals("12345678"));
+  EXPECT_TRUE(BufData2.substr(0x17F8,8).equals("12345678"));
+  EXPECT_TRUE(BufData2.substr(0x1800,8).equals("abcdefgh"));
+  EXPECT_TRUE(BufData2.substr(0x2FF8,8).equals("abcdefgh"));
+ 
+}
+
+
+
 }