[lldb-dev] debugserver and llvm

[Greg Clayton via lldb-dev]

> On Aug 29, 2016, at 10:58 AM, Zachary Turner <zturner at google.com> wrote:
> 
> I don't plan to change debugserver's link requirements.  What I'm saying is that debugserver is including StringExtractor.h cross-project from LLDB, and so even something as simple as including an LLVM header from StringExtractor.h will (if I understand correctly) break debugserver.  If I'm correct, then I don't think this is an acceptable limitation for an LLDB project header.

Feel free to fix it and fixup the debugserver use. The intention was to be able to use StringExtractor in both LLDB and debugserver and to not require LLVM headers for this file only. I was trying to make sure we don't have two copies of a very similar class and have to fix bugs in two places, but I see your point about the file being in LLDB.

> 
> I have a patch locally which changes GetNameColonValue() to return two StringRefs instead of two std::strings, eliminating hundreds of string copies and is perfectly safe. So I don't see this as a particularly controversial change to get into LLDB, but it will require debugserver to keep its own local copy of StringExtractor.h, similar to how it already does with JSON.h and JSON.cpp.  I hoping to get this change in today since it is a strict improvement over the current StringExtractor.

Feel free to do it all. I don't like having two copies of code and the original change where I started to use that was to avoid this, but I guess it will be back.
> 
> 
> There are still some additional improvements I would like to make independently of this initial change, and they do culminate in changing StringExtractor to store an llvm::StringRef.  It turns out that while yes, it uses an std::string, I cannot find one single user (and I have looked at all of them) that depends on this. In every single case, it can use a StringRef with no ownership issues, and the number of string copies is further reduced.  So I don't see a convincing argument to keep this as storing a std::string.  But maybe there is something I'm not aware of?

Just every packet handler for the GDB remote stuff. StringExtractorGDBRemote inherits from StringExtractor and uses the std::string to store the packet. Not to say that this can't be fixed with software.

I still vote to just make a StringRefExtractor.h/.cpp that completely cuts over to using llvm::StringRef only. We can cut over to using it everywhere. If nothing is left inside of LLDB, we can move StringExtractor.cpp/.h over into the debugserver code and have it gone from LLDB.

> 
> On Mon, Aug 29, 2016 at 10:51 AM Greg Clayton <gclayton at apple.com> wrote:
> 
> > On Aug 27, 2016, at 3:14 PM, Zachary Turner via lldb-dev <lldb-dev at lists.llvm.org> wrote:
> >
> > What is the status of using LLVM from debugserver?  AFAICT, it doesn't use llvm, but it DOES use some lldb private libraries, in which case it is already implicitly linking against LLVM anyway.
> >
> > So why can't LLVM headers be included and used from debugserver?  Just now I was changing the signature of an LLDB function to include an llvm header, and I got everything working and ready to go but searched through code that doesn't compile on Windows, and I noticed that debugserver includes some headers from lldb, and since those lldb headers are including llvm headers, I'm assuming this is not going to work.  But I think I'm missing something, because AFAICT this will already cause a link dependency from debugserver to llvm, and has been for some time.  So I'm not entirely sure what the status is of this.
> >
> > In any case, I know the easy way out is to just say don't include an llvm header from that lldb header, but I don't think that's a great idea as I think the patch I'm working on is a big win for performance, code readability, and simplicity.
> >
> > It looks like there is already precedent for debugserver to copy some code from lldb and keep a local copy in debugserver, for example JSON.h and JSON.cpp seem to be copies of the code from lldb/Utility.  Can this be done for StringExtractor.h as well so that I can reference llvm from within StringExtractor.h in lldb?
> 
> debugserver currently doesn't link against any of the llvm .a files so I don't think it can be using any llvm stuff unless it uses header file only inlined functions. We aren't going to spend any time on this and we have builds of debugserver that link against even less (debugserver-mini) that the normal debugserver. So I advise you don't change debugserver's link requirements as we build it in many different places and we don't always build llvm when we build, so debugserver can't have requirements to link against llvm or clang .a files.
> 
> For one I would prefer to leave StringExtractor alone. It uses the std::string to store the packet content and we want that the remain as is. I am fine with coming up with a replacement StringRefExtractor.h/.cpp that uses an llvm::StringRef that we cut over to using internally in LLDB, but again, for anyone needing the data to stay stored somewhere, they should use StringExtractor.cpp.