> On Feb 15, 2017, at 10:30 AM, Zachary Turner <ztur...@google.com> wrote: > > Yes. In some cases I want to try deleting LLDB's implementation and seeing > if we can fall back on LLVM. For example, MemoryMapFileContents comes to > mind. LLVM has some code to memory map files as well. I've spent 0 time > looking into how widely FileSpec::MemoryMapFileContents is used, so it's > possible that after 1 minute I realize it's not possible / too much work. > But I'd like to at least do due diligence and see if there's any parts of the > FileSpec interface that can be removed in favor of LLVM classes first.
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. Greg > > On Wed, Feb 15, 2017 at 10:20 AM Greg Clayton <gclay...@apple.com > <mailto:gclay...@apple.com>> wrote: >> On Feb 15, 2017, at 10:14 AM, Zachary Turner <ztur...@google.com >> <mailto:ztur...@google.com>> wrote: >> >> I think the main improvement that it provides is that you need *something* >> which represents only a path. Because we use FileSpecs to refer to paths on >> remote machines, and then what does it mean to call Exists or Resolve? So, >> we could have something like PathSpec and then have FileSpec contain a >> PathSpec, and PathSpec's only purpose would be to know about the grammar of >> a path. > > Agreed. Are you ok with starting with moving as much stuff from FileSpec over > into File to start with? > >> >> I agree though, in the spirit of doing things incrementally, purely moving >> things around and not making major substantive changes is a good starting >> point and reduces the scope of the problem. > > Agreed. > >> >> On Wed, Feb 15, 2017 at 10:07 AM Greg Clayton <gclay...@apple.com >> <mailto:gclay...@apple.com>> wrote: >> I would prefer things stay object oriented when possible. I don't think it >> adds anything by making everything a static function. It is ok where it >> makes sense, but I would vote to add static functions in the FileSpec class >> over making purely stand alone functions as they indicate that it should be >> used on a FileSpec object. It is also ok to have both methods and static >> functions where the methods (like AppendPathComponent() can call through to >> the static versions if needed. >> >> We also have the "File.h" class that defines lldb_private::File. This is the >> object that wraps the access to the data in the file (the file descriptor >> integers and the "FILE *" abstraction. It would be interesting to move any >> functions in lldb_private::FileSpec, like the mmap functions, over into >> lldb_private::File where needed. And then see if there is even a need for >> the FileSystem class at all. If there is we can divvy it up as needed. >> >> I would say lets start with purely moving things around and get the file >> locations all set and the correct abstractions where FileSpec is for >> representing a file specification efficiently and all operations on that >> affect file spec mutations, moving anything that is accessing file contents >> over into lldb_private::File, and then seeing if we have any functions left >> over and if we even need FileSystem. >> >> That being said I generally find it useful to do something like: >> >> FileSpec f("/tmp/a.txt"); >> if (f.Exists()) >> ... >> >> >> It seems like the suggestions would be to move to something like: >> >> FileSpec f("/tmp/a.txt"); >> if (File::Exists(f)) >> ... >> >> Both are fine, but the latter is much less discoverable. Think about when >> you use IDEs with auto completion. So while it might be cleaner from a code >> linking standpoint, I don't think it actually improves the code for users. I >> much prefer the object oriented methodology and I would prefer code linking >> considerations don't override the clean API we currently have. >> >> > On Feb 15, 2017, at 8:05 AM, Pavel Labath via lldb-dev >> > <lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>> wrote: >> > >> > I prefer free functions as well, but I think switching FileSpec to >> > that would be a non-trivial task. If you want to try it out I would >> > encourage it, but I don't think it's a requirement. >> > >> > On 15 February 2017 at 15:41, Zachary Turner <ztur...@google.com >> > <mailto:ztur...@google.com>> wrote: >> >> Having FileSpec in Utility and FileSystem in Host would work for now. >> >> >> >> Any opinions on the FileSpec interface? Llvm's path and file system >> >> libraries use free functions for everything. in a perfect world I feel >> >> that's the best design, and with lldb we could even do slightly better and >> >> make all functions pure (returning copies instead of mutating) since >> >> everything is ConstString. >> >> >> >> If we were starting over from scratch this would definitely be the way to >> >> go >> >> imo, but I'm not sure if it's worth the effort now. What do you think? >> >> >> >> On Wed, Feb 15, 2017 at 7:34 AM Pavel Labath <lab...@google.com >> >> <mailto:lab...@google.com>> wrote: >> >>> >> >>> Right, I see. In that case, I guess my response would be "let's wait >> >>> and see how things look like after FileSpec is moved". >> >>> >> >>> I kinda like how we have the all the host code in a separate module (I >> >>> expect we will have a lot more of it than llvm), but I am not against >> >>> it if it turns out there is no other way to organize dependencies. I >> >>> just don't think we've reached that point yet -- right now I don't see >> >>> why we couldn't have FileSpec in Utility and FileSystem in Host. >> >>> >> >>> Or have you thought ahead and found a problem already? >> >>> >> >>> >> >>> On 15 February 2017 at 15:17, Zachary Turner <ztur...@google.com >> >>> <mailto:ztur...@google.com>> wrote: >> >>>> Yes, in fact that mirrors how i had planned to tackle this. >> >>>> >> >>>> The question is, can we put in Utility code that is separated by >> >>>> platform, >> >>>> which typically has been for Host? This mirrors what llvm does >> >>>> On Wed, Feb 15, 2017 at 6:29 AM Pavel Labath <lab...@google.com >> >>>> <mailto:lab...@google.com>> wrote: >> >>>>> >> >>>>> I agree that the next module that needs figuring on is the host one. >> >>>>> However I am not sure if the decision on what to put in what module >> >>>>> should be motivated by the FileSpec class, as I think it's current >> >>>>> interface has a some issues, and our choice on how to resolve them can >> >>>>> greatly affect what things it depends on. >> >>>>> >> >>>>> The main thing that bugs me with FileSpec is that it is used for a two >> >>>>> distinct purposes: >> >>>>> - manipulations on abstract path names (e.g. PrependPathComponent) >> >>>>> - manipulations of actual files present on the host (e.g. >> >>>>> MemoryMapFileContents) >> >>>>> For the first one you don't need the files to exist on the host (in >> >>>>> fact they may not even use the same path syntax as the host). For the >> >>>>> other one, they *must* exist and *must* use the same path syntax. This >> >>>>> is currently not very well separated and enforced, and I think it >> >>>>> should be. I believe this is the reason the FileSystem pseudo-class >> >>>>> was created. >> >>>>> >> >>>>> So, my counter-proposal would be to finish moving the host-specific >> >>>>> things out of the FileSpec class (basically just replace >> >>>>> file_spec.Foo(...) with FileSystem::Foo(file_spec, ...). At that point >> >>>>> FileSpec should only depend on string manipulation functions, and we >> >>>>> should be able to move it without much hassle. After that, we can take >> >>>>> another look and decide what to do next. >> >>>>> >> >>>>> The thing I really like about this idea is that we will end up with >> >>>>> two classes that very closely mirror llvm functionality (FileSpec is a >> >>>>> version of Support/Path.h that does not assume host path syntax, >> >>>>> FileSystem is similar to Support/FileSystem.h, but it supports some >> >>>>> more fancy operations like mmap()). Then we could proceed to merge >> >>>>> this functionality with llvm pretty much independently of any other >> >>>>> refactoring we will be doing. >> >>>>> >> >>>>> What do you think? >> >>>>> >> >>>>> >> >>>>> >> >>>>> On 15 February 2017 at 01:48, Zachary Turner via lldb-dev >> >>>>> <lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org>> wrote: >> >>>>>> After https://reviews.llvm.org/D29964 >> >>>>>> <https://reviews.llvm.org/D29964>, we finally have a starting >> >>>>>> point >> >>>>>> at >> >>>>>> which we can begin unravelling the cross-project cyclic dependencies >> >>>>>> in >> >>>>>> LLDB. lldb/Utility now is very similar in spirit to llvm/Support. >> >>>>>> >> >>>>>> But llvmSupport goes one step further and includes what lldb would >> >>>>>> normally >> >>>>>> put under Host. I think this makes some sense. Practically all >> >>>>>> parts >> >>>>>> of a >> >>>>>> codebase have need of a single OS abstraction layer. So, I think >> >>>>>> that a >> >>>>>> lot >> >>>>>> of the functionality currently in lldbHost is in fact needed by the >> >>>>>> rest >> >>>>>> of >> >>>>>> LLDB. >> >>>>>> >> >>>>>> So, I wonder if it makes sense to follow the path that LLVM has >> >>>>>> taken, >> >>>>>> and >> >>>>>> start moving some of this code from Host down to Utility. Doing so >> >>>>>> would >> >>>>>> allow us to break one of the biggest links in the dependency cycle in >> >>>>>> the >> >>>>>> entire codebase, which is that Host depends on everything, and >> >>>>>> everything >> >>>>>> depends on Host. >> >>>>>> >> >>>>>> Of course, it can't just be a straight move. Some things in Host >> >>>>>> interact >> >>>>>> with Target, with CommandInterpreter, and with many other things. >> >>>>>> And >> >>>>>> stuff >> >>>>>> going into Utility can't take a dependency. >> >>>>>> >> >>>>>> So there will be some splitting, some moving, some refactoring, etc. >> >>>>>> But to >> >>>>>> me tackling Host seems like the logical next step, in large part >> >>>>>> because >> >>>>>> Host is where FileSpec is, and it's going to be hard to break any >> >>>>>> dependencies without first addressing FileSpec. >> >>>>>> >> >>>>>> The way LLVM handles cross-platform differences in Support is that >> >>>>>> you >> >>>>>> include a single header and it does conditional includes into a >> >>>>>> platform >> >>>>>> specific subdirectory for the parts that differ. >> >>>>>> >> >>>>>> I'm thinking to follow the same pattern here in lldb/Utility, and >> >>>>>> begin >> >>>>>> looking for ways to get pieces of Host down into Utility this way, >> >>>>>> until >> >>>>>> ultimately I can get FileSpec down there. >> >>>>>> >> >>>>>> Thoughts? >> >>>>>> >> >>>>>> _______________________________________________ >> >>>>>> lldb-dev mailing list >> >>>>>> lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org> >> >>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev >> >>>>>> <http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev> >> >>>>>> >> > _______________________________________________ >> > lldb-dev mailing list >> > lldb-dev@lists.llvm.org <mailto:lldb-dev@lists.llvm.org> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev >> > <http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev> >> >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
