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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
