[Lldb-commits] [PATCH] D74660: WIP: [lldb/FileSystem] Add & use CreateReadonlyDataBuffer where possible

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: labath.
labath added a comment.

Before you get too carried away with this, I'd like us to clarify something. Your comment seems to imply that `FileSystem::CreateDataBuffer` does not use mmap. That doesn't sound right, and it's not what I see happening on linux now (and I don't see why macos would be different):

  $ strace -e trace=file,desc bin/lldb bin/lldb -o "image dump sections" -b
  ...
  openat(AT_FDCWD, "bin/lldb", O_RDONLY|O_CLOEXEC) = 4
  fstat(4, {st_mode=S_IFREG|0770, st_size=414096, ...}) = 0
  mmap(NULL, 414096, PROT_READ|PROT_WRITE, MAP_PRIVATE, 4, 0) = 0x7f94160d7000

What this does is create a mapping, where unmodified pages are backed by the file on disk, and memory is allocated for any modified pages in the usual copy-on-write fashion. So, if you don't modify any pages -- you get no allocations.

It is true that `FileSystem::CreateDataBuffer` can sometimes use a heap allocation (malloc) to fulfill the request, but this has nothing to do with the writability of that buffer. What happens is that if we detect that the file we are about to read comes from a "remote" system (NFS, etc.), then we will fall back to malloc+read, because mmaps of volatile files are somewhat precarious.

For this reason, I am somewhat doubtful that this patch will solve the memory problems with memory usage, except maybe by playing some accounting tricks, where a read-only mapping would get reported in a different bucket than a read-write COW one. So, I gotta ask: How sure are you that this patch will solve the problems reported by your users? Is it possible that these two users have some kinds of remote mounts or something similar that could be throwing us off track?

Regardless of the answers to the questions above, I think that this patch, in principle, is a good idea, but I think we should encode the mutability of the DataBuffer in the type system. If we can ensure that `CreateReadonlyDataBuffer` returns a `const DataBuffer`, then we can have the compiler check for us that noone writes to these buffers, instead of us trying to guess.

As for ObjectFileELF, relocating the data inside `RelocateDebugSections` would basically reimplement what the COW mmap gives us for free. So, I think a simpler solution is to just make sure ObjectFileELF always creates a read-write mapping for the object file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74660/new/

https://reviews.llvm.org/D74660