MemoryBlock: Do not automatically extend a given size to a multiple of page size.

[ruiu]

Summary

Previously, MemoryBlock automatically extends a requested buffer size to a
multiple of page size because (I believe) doing it was thought to be harmless
and with that you could get more memory (on average 2KiB on 4KiB-page systems)
"for free".

That programming interface turned out to be error-prone. If you request N
bytes, you usually expect that a resulting object returns N for size().
That's not the case for MemoryBlock.

Looks like there is only one place where we take the advantage of
allocating more memory than the requested size. So, with this patch, I
simply removed the automatic size expansion feature from MemoryBlock
and do it on the caller side when needed. MemoryBlock now always
returns a buffer whose size is equal to the requested size.

[ruiu]

• rebase

[thakis]

It's a bit weird that allocateMappedMemory() returns a buffer whose size is larger than what's requested, but the method is explicitly documented as such: http://llvm-cs.pcc.me.uk/include/llvm/Support/Memory.h#64

I looked around a bit through callers and several others seem to get this wrong too, at least from a quick look (e.g. http://llvm-cs.pcc.me.uk/include/llvm/Support/BinaryByteStream.h#205 , used by MSFBuilder::commit() (http://llvm-cs.pcc.me.uk/lib/DebugInfo/MSF/MSFBuilder.cpp#347) -- unless that's supposed to write the extra data.

Did you look through callers and determined that the api as-is is the best compromise, or should we change the Memory::allocateMappedMemory() to set the requested size instead of the actual one?

(Having said that, I don't know this code well and I'm probably not the best person to review it.)

[ruiu]

Good point. I found that that's very confusing too. Maybe size() should return the requested size instead of the actual allocated size, and if there's a need to get an allocated size, we can provide it as a different member function such as allocatedSize() or something. Let me take a look.

[ruiu]

• Change allocateMemoryBlock instead of FileOutputBuffer

[thakis]

This looks fine to me, but as I said I'm not an expert. On the other hand, if someone depended on the old tricky semantics and didn't write a test, it's on them if they get broken :-)

[lhames]

Bah -- I missed this at the time. This should be reverted. We can rename methods to clarify, but MemoryBlock should hold the allocated size, not the requested size.

[lhames]

See discussion on https://reviews.llvm.org/D61599.

[ruiu]

https://reviews.llvm.org/D61599 doesn't seem to revert this, but instead adding a method to obtain an allocated size as well as a requested size (and rename size() to allocatedSize()). That's fine by me.

[mvhooren]

In D56941#1494415, @ruiu wrote:

https://reviews.llvm.org/D61599 doesn't seem to revert this, but instead adding a method to obtain an allocated size as well as a requested size (and rename size() to allocatedSize()). That's fine by me.

D61599 has now been updated as was discussed there. It now does revert this patch. size() was renamed to allocatedSize() and the InMemoryBuffer in FileOutputBuffer.cpp now tracks the requested size separately.

lld/test/ELF/stdout.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
-# RUN: ld.lld %t.o -o - > %t
-# RUN: llvm-objdump -d %t | FileCheck %s
+# RUN: ld.lld %t.o -o - > %t1
+# RUN: llvm-objdump -d %t1 | FileCheck %s
 
 # CHECK: 0000000000201000 _start:
 # CHECK: 201000: 90 nop
 
+# RUN: ld.lld %t.o -o %t2
+# RUN: diff %t1 %t2
+
 .globl _start
 _start:
   nop

llvm/lib/ExecutionEngine/SectionMemoryManager.cpp

 // Trivial implementation of SectionMemoryManager::MemoryMapper that just calls
 // into sys::Memory.
 class DefaultMMapper final : public SectionMemoryManager::MemoryMapper {
 public:
   sys::MemoryBlock
   allocateMappedMemory(SectionMemoryManager::AllocationPurpose Purpose,
                        size_t NumBytes, const sys::MemoryBlock *const NearBlock,
                        unsigned Flags, std::error_code &EC) override {
-    return sys::Memory::allocateMappedMemory(NumBytes, NearBlock, Flags, EC);
+    // allocateMappedMemory calls mmap(2). We round up a request size
+    // to page size to get extra space for free.
+    static const size_t PageSize = sys::Process::getPageSize();
+    size_t ReqBytes = (NumBytes + PageSize - 1) & ~(PageSize - 1);
+    return sys::Memory::allocateMappedMemory(ReqBytes, NearBlock, Flags, EC);
   }
 
   std::error_code protectMappedMemory(const sys::MemoryBlock &Block,
                                       unsigned Flags) override {
     return sys::Memory::protectMappedMemory(Block, Flags);
   }
 
   std::error_code releaseMappedMemory(sys::MemoryBlock &M) override {

llvm/lib/Support/Unix/Memory.inc

 Memory::allocateMappedMemory(size_t NumBytes,
                              const MemoryBlock *const NearBlock,
                              unsigned PFlags,
                              std::error_code &EC) {
   EC = std::error_code();
   if (NumBytes == 0)
     return MemoryBlock();
 
-  static const size_t PageSize = Process::getPageSize();
-  const size_t NumPages = (NumBytes+PageSize-1)/PageSize;
-
   int fd = -1;
 
   int MMFlags = MAP_PRIVATE |
 #ifdef MAP_ANONYMOUS
   MAP_ANONYMOUS
 #else
   MAP_ANON
 #endif
   ; // Ends statement above
 
   int Protect = getPosixProtectionFlags(PFlags);
 
 #if defined(__NetBSD__) && defined(PROT_MPROTECT)
   Protect |= PROT_MPROTECT(PROT_READ | PROT_WRITE | PROT_EXEC);
 #endif
 
   // Use any near hint and the page size to set a page-aligned starting address
   uintptr_t Start = NearBlock ? reinterpret_cast<uintptr_t>(NearBlock->base()) +
                                       NearBlock->size() : 0;
+  static const size_t PageSize = Process::getPageSize();
   if (Start && Start % PageSize)
     Start += PageSize - Start % PageSize;
 
-  void *Addr = ::mmap(reinterpret_cast<void*>(Start), PageSize*NumPages,
-                      Protect, MMFlags, fd, 0);
+  void *Addr = ::mmap(reinterpret_cast<void *>(Start), NumBytes, Protect,
+                      MMFlags, fd, 0);
   if (Addr == MAP_FAILED) {
     if (NearBlock) //Try again without a near hint
       return allocateMappedMemory(NumBytes, nullptr, PFlags, EC);
 
     EC = std::error_code(errno, std::generic_category());
     return MemoryBlock();
   }
 
   MemoryBlock Result;
   Result.Address = Addr;
-  Result.Size = NumPages*PageSize;
+  Result.Size = NumBytes;
 
   // Rely on protectMappedMemory to invalidate instruction cache.
   if (PFlags & MF_EXEC) {
     EC = Memory::protectMappedMemory (Result, PFlags);
     if (EC != std::error_code())
       return MemoryBlock();
   }