> On Sep 19, 2016, at 2:24 PM, Zachary Turner <ztur...@google.com> wrote:
> On Mon, Sep 19, 2016 at 2:02 PM Greg Clayton <gclay...@apple.com> wrote:
> > On Sep 19, 2016, at 1:18 PM, Zachary Turner via lldb-dev 
> > <lldb-dev@lists.llvm.org> wrote:
> >
> > Following up with Kate's post from a few weeks ago, I think the dust has 
> > settled on the code reformat and it went over pretty smoothly for the most 
> > part.  So I thought it might be worth throwing out some ideas for where we 
> > go from here.  I have a large list of ideas (more ideas than time, sadly) 
> > that I've been collecting over the past few weeks, so I figured I would 
> > throw them out in the open for discussion.
> >
> > I’ve grouped the areas for improvement into 3 high level categories.
> >
> >       • De-inventing the wheel - We should use more code from LLVM, and 
> > delete code in LLDB where LLVM provides a solution.  In cases where there 
> > is an LLVM thing that is *similar* to what we need, we should extend the 
> > LLVM thing to support what we need, and then use it.  Following are some 
> > areas I've identified.  This list is by no means complete.  For each one, 
> > I've given a personal assessment of how likely it is to cause some 
> > (temporary) hiccups, how much it would help us in the long run, and how 
> > difficult it would be to do.  Without further ado:
> >               • Use llvm::Regex instead of lldb::Regex
> >                       • llvm::Regex doesn’t support enhanced mode.  Could 
> > we add support for this to llvm::Regex?
> >                       • Risk: 6
> >                       • Impact: 3
> >                       • Difficulty / Effort: 3  (5 if we have to add 
> > enhanced mode support)
> As long as it supports all of the features, then this is fine. Otherwise we 
> will need to modify the regular expressions to work with the LLVM version or 
> back port any advances regex features we need into the LLVM regex parser.
> >               • Use llvm streams instead of lldb::StreamString
> >                       • Supports output re-targeting (stderr, stdout, 
> > std::string, etc), printf style formatting, and type-safe streaming 
> > operators.
> >                       • Interoperates nicely with many existing llvm 
> > utility classes
> >                       • Risk: 4
> >                       • Impact: 5
> >                       • Difficulty / Effort: 7
> I have worked extensively with both C++ streams and with printf. I don't find 
> the type safe streaming operators to be readable and a great way to place 
> things into streams. Part of my objection to this will be quelled if the LLVM 
> streams are not stateful like C++ streams are (add an extra "std::hex" into 
> the stream and suddenly other things down stream start coming out in hex). As 
> long as printf functionality is in the LLVM streams I am ok with it.
> I agree with you on the streams for the most part.  llvm streams do not use 
> stateful manipulators, although they do have stateless manipulators.  For 
> example, you can write:
> stream << llvm::format_hex(n);
> and that would print n as hex, but it wouldn't affect the printing of 
> subsequent items.  You also still get printf style formatting by using the 
> llvm::format stateless manipulator.  Like this:
> stream << llvm::format("%0.4f", myfloat)
> >               • Use llvm::Error instead of lldb::Error
> >                       • llvm::Error is an error class that *requires* you 
> > to check whether it succeeded or it will assert.  In a way, it's similar to 
> > a C++ exception, except that it doesn't come with the performance hit 
> > associated with exceptions.  It's extensible, and can be easily extended to 
> > support the various ways LLDB needs to construct errors and error messages.
> >                       • Would need to first rename lldb::Error to LLDBError 
> > so that te conversion from LLDBError to llvm::Error could be done 
> > incrementally.
> >                       • Risk: 7
> >                       • Impact: 7
> >                       • Difficulty / Effort: 8
> We must be very careful here. Nothing can crash LLDB and adding something 
> that will be known to crash in some cases can only be changed if there is a 
> test that can guarantee that it won't crash. assert() calls in a shared 
> library like LLDB are not OK and shouldn't fire. Even if they do, when 
> asserts are off, then it shouldn't crash LLDB. We have made efforts to only 
> use asserts in LLDB for things that really can't happen, but for things like 
> constructing a StringRef with NULL is one example of why I don't want to use 
> certain classes in LLVM. If we can remove the crash threat, then lets use 
> them. But many people firmly believe that asserts belong in llvm and clang 
> code and I don't agree. Also many asserts are in header files so even if you 
> build llvm and clang with asserts off, but then build our project that uses 
> LLVM with asserts on, you get LLVM and clang asserts when you don't want them.
> We can probably add something to llvm::Error to make it not assert but to do 
> something else instead.  Something that would be up to the person raising the 
> error, so we could say that all lldb errors wouldn't cause a problem.
> As an aside, have you guys ever talked about the idea of using out of process 
> isolation so it doesn't bring down all of xcode when it crashes?  Obviously 
> it's a ton of work, but it would solve these kidns of problems.

Yep. Xcode 8 does this just for this reason. More details on that later.

> >               • StringRef instead of const char *, len everywhere
> >                       • Can do most common string operations in a way that 
> > is guaranteed to be safe.
> >                       • Reduces string manipulation algorithm complexity by 
> > an order of magnitude.
> >                       • Can potentially eliminate tens of thousands of 
> > string copies across the codebase.
> >                       • Simplifies code.
> >                       • Risk: 3
> >                       • Impact: 8
> >                       • Difficulty / Effort: 7
> I don't think StringRef needs to be used everywhere, but it many places it 
> does make sense. For example our public API should not contain any LLVM 
> classes in the API. "const char *" is a fine parameter for public functions. 
> I really hate the fact that constructing llvm::StringRef with NULL causes an 
> assertion. Many people don't know that and don't take that into account when 
> making their changes. I would love to see the assertion taken out to tell you 
> the truth. That would make me feel better about using StringRef in many more 
> places within LLDB as we shouldn't ever crash due to this. I would expect 
> most internal APIs are safe to move to llvm::StringRef, but we will need to 
> make sure none of these require a null terminated string which would cause us 
> to have to call "str.str().c_str()". So internally, yes we can cut over to 
> more use of StringRef. But the public API can't be changed.
> Definitely no changing the public API, I agree with you there.  A lot of 
> times where we currently "need" a null terminated string, that's only because 
> the null terminated string is being passed to a C api which has a StringRef 
> counterpart.  Not always, but often.  I think when you have StringRefs all 
> the way down, the problem of constructing it with a null pointer largely goes 
> away because you don't have that many pointers left to begin with.
> >               • ArrayRef instead of const void *, len everywhere
> >                       • Same analysis as StringRef
> Internally yes, external APIs no.
> >               • MutableArrayRef instead of void *, len everywhere
> >                       • Same analysis as StringRef
> Internally yes, external APIs no.
> >               • Delete ConstString, use a modified StringPool that is 
> > thread-safe.
> >                       • StringPool is a non thread-safe version of 
> > ConstString.
> >                       • Strings are internally refcounted so they can be 
> > cleaned up when they are no longer used.  ConstStrings are a large source 
> > of memory in LLDB, so ref-counting and removing stale strings has the 
> > potential to be a huge savings.
> >                       • Risk: 2
> >                       • Impact: 9
> >                       • Difficulty / Effort: 6
> We can't currently rely on ref counting. We currently hand out "const char *" 
> as return values from many public API calls and these rely on the strings 
> living forever. We many many existing clients and we can't change the public 
> API. So I vote to avoid this. We are already using LLVM string pools under 
> the covers and we also associate the mangled/demangled names in the map as is 
> saves us a ton of cycles when parsing stuff as we never demangle (very 
> expensive) the same name twice. So I don't see the need to change this as it 
> is already backed by LLVM technology under the covers.

> Just thinking out loud here, but one possibility is to have multiple pools.  
> One which is ref counted and one which isn't.  If you need something to live 
> forever, vend it from the non-ref-counted pool.  Otherwise vend it from the 
> ref counted pool.

Again, since we already are using LLVM under the hood here, I would hope to 
avoid changes if possible.

lldb-dev mailing list

Reply via email to