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 <ztur...@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 <gclay...@apple.com> wrote: > >> >> > On Aug 29, 2016, at 10:58 AM, Zachary Turner <ztur...@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 <gclay...@apple.com> >> wrote: >> > >> > > On Aug 27, 2016, at 3:14 PM, Zachary Turner via lldb-dev < >> lldb-dev@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. >> >>
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
