[lldb-dev] About lldbHost

Greg Clayton via lldb-dev lldb-dev at lists.llvm.org
Wed Feb 15 10:35:03 PST 2017 

> On Feb 15, 2017, at 10:30 AM, Zachary Turner <zturner at 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 <gclayton at apple.com <mailto:gclayton at apple.com>> wrote:
>> On Feb 15, 2017, at 10:14 AM, Zachary Turner <zturner at google.com <mailto:zturner at 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 <gclayton at apple.com <mailto:gclayton at 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 at lists.llvm.org <mailto:lldb-dev at 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 <zturner at google.com <mailto:zturner at 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 <labath at google.com <mailto:labath at 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 <zturner at google.com <mailto:zturner at 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 <labath at google.com <mailto:labath at 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 at lists.llvm.org <mailto:lldb-dev at 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 at lists.llvm.org <mailto:lldb-dev at 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 at lists.llvm.org <mailto:lldb-dev at lists.llvm.org>
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev>
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20170215/e74b1ef3/attachment-0001.html>

More information about the lldb-dev mailing list