> On Feb 15, 2017, at 11:22 AM, Zachary Turner <ztur...@google.com> wrote: > > Yea, the flag seems like one you would want to use almost always, so I'm not > opposed to having it. I'll see about making the changes in LLVM, even if we > end up not using it in LLDB, they seem useful in LLVM independently. > > BTW, one difference in LLVM's mmap code is that in LLDB we always use > MAP_PRIVATE, but in LLVM if you are opening for readwrite it uses MAP_SHARED. > Are you against using MAP_SHARED when mmaping with readwrite privileges?
You will need to do some testing. If you do MAP_SHARED and the file goes away, you might crash as it won't keep the file around as long as it needs it. I could be wrong on this. But I do remember explicitly picking MAP_PRIVATE for some reason in the past... > On Wed, Feb 15, 2017 at 11:19 AM Greg Clayton <gclay...@apple.com > <mailto:gclay...@apple.com>> wrote: >> On Feb 15, 2017, at 11:07 AM, Zachary Turner <ztur...@google.com >> <mailto:ztur...@google.com>> wrote: >> >> If you only ever call MemoryMapContentsIfLocal, then is that first flag even >> doing anything? And if you are passing that flag, then can you just mmap it >> always even if non-local? > > If that is the only call people are using, then we don't really need the > flag. Not sure how else as local file could go away such that the backing > store wouldn't be available. mmap() on unix will keep the file around as long > as its needed AFAIK, so even if someone deletes it, it would be kept around. > So if those are our only clients we should be ok. The code signing bit would > need to be added for Mac though. > >> I searched the code and only in the windows minidump plugin are we >> unconditionally mmaping, and that could be changed to a conditional-on-local >> just like everywhere else. If we do that, then nobody is ever mmaping any >> non-local file AFAICT > > That currently is true, but it would be shame to lose the ability to be > resilient in cases where you do want to mmap. If a file is too large, we > really should probably have an upper end cutoff on file size that would mmap > even if remote. If we have a 4 GB file that we want to access, probably not a > great thing to just pop into a heap based memory buffer... > > Greg > >> >> On Wed, Feb 15, 2017 at 11:02 AM Greg Clayton <gclay...@apple.com >> <mailto:gclay...@apple.com>> wrote: >> >>> On Feb 15, 2017, at 10:58 AM, Zachary Turner <ztur...@google.com >>> <mailto:ztur...@google.com>> wrote: >>> >>> >>> On Wed, Feb 15, 2017 at 10:47 AM Reid Kleckner <r...@google.com >>> <mailto:r...@google.com>> wrote: >>> On Wed, Feb 15, 2017 at 10:35 AM, Greg Clayton via lldb-dev >>> <lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>> wrote: >>> I am fine with switching mmap over to llvm if it works. One important thing >>> to LLDB is we have a "mmap if not on remote file system" that must continue >>> to work. If you mmap something from a network drive and then it gets >>> disconnected, you can crash LLDB. So we have a function we used that >>> implements mmap if local, read all contents if remote, that must work to >>> avoid crashes. >>> >>> LLVM's MemoryBuffer API already serves too many different use cases. >>> Initially it was designed to be a utility for efficiently reading source >>> file inputs. It has a bunch of functionality around ensuring that the >>> buffer is null terminated, and a boolean to avoid using mmap when the user >>> might modify the file concurrently. It's too complicated. I wouldn't >>> recommend using it without a good reason beyond just reusing a platform >>> abstraction. mmap and MapViewOfFile are not that complicated. LLDB is >>> probably better off doing its own thing unless it needs to pass mapped file >>> contents to other parts of LLVM, like maybe clang's VFS. >>> >>> I just took a look and it seems like almost a drop in replacement. Only >>> thing that would need changing is updating shouldUseMmap() to return false >>> if a file is on a network share. But this seems like a good change >>> independently of lldb. >>> >>> After that all lldb has to do is say MemoryBuffer::getOpenFile() and >>> everything works. >>> >>> Is there a good reason *not* to use it? Evenif internally the >>> implementation is complex, the external interface is not >> >> The other thing is on Mac we add new flags to mmap that allow us not to >> crash if the backing store (network mount) goes away. There is also a flag >> that says "if code signature is borked, please still allow me to read the >> file without crashing. That functionality will need to be ported into the >> LLVM code somehow so we don't start crashing again. Previously we would >> crash if someone would do: >> >> (lldb) file /tmp/invalid_codesig_app >> >> And also if the network mounted share would go away on something that was >> mmap'ed and someone would touch any byte within it. Our version of mmap >> works around this and we need that to be in any version we adopt. >> >> Greg >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
