[lldb-dev] debugserver and llvm

[Zachary Turner via lldb-dev]

I think the easiest way to do this is to copy the current implementation to
StdStringExtractor and then have debugserver use that.  That only requires
1-2 lines of code change in debugserver, and no code change in LLDB.  That
way existing code all picks up the new llvm-based implementation, and I can
work on it iteratively so I don't have to go full StringRef right from the
start (which turns into a massive changelist and makes bisecting a
regression next to impossible).

On Mon, Aug 29, 2016 at 11:15 AM Zachary Turner <zturner at google.com> wrote:

> Making a StringRefExtractor, switching everything over to that, and then
> moving StringExtractor to debugserver once everything else is using
> StringRefExtractor seems like a reasonable approach
> On Mon, Aug 29, 2016 at 11:12 AM Greg Clayton <gclayton at apple.com> wrote:
>
>>
>> > 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.
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160829/ecd9f8f3/attachment-0001.html>