[lldb-dev] LLDB Evolution - Final Form

[Greg Clayton via lldb-dev]

> On Sep 20, 2016, at 10:47 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
>> 
>> On Sep 20, 2016, at 10:33 AM, Greg Clayton <gclayton at apple.com> wrote:
>> 
>>> 
>>> On Sep 19, 2016, at 2:46 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>> 
>>>> 
>>>> On Sep 19, 2016, at 2:34 PM, Greg Clayton <gclayton at apple.com> wrote:
>>>> 
>>>>> 
>>>>> On Sep 19, 2016, at 2:20 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On Sep 19, 2016, at 2:02 PM, Greg Clayton via lldb-dev <lldb-dev at lists.llvm.org> wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Sep 19, 2016, at 1:18 PM, Zachary Turner via lldb-dev <lldb-dev at 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.
>>>>>> 
>>>>>>> 		• 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.
>>>>> 
>>>>> I’m surprise by your aversion to assertions, what is your suggested alternative? Are you expecting to check and handle every possible invariants everywhere and recover (or signal an error) properly? That does not seem practical, and it seems to me that it'd lead to just involving UB (or breaking design invariant) without actually noticing it.
>>>> 
>>>> We have to as a debugger. We get input from a variety of different compilers and we parse and use things there were produced by our toolchains and others. Crashing Xcode because someone accidentally created a llvm::StringRef(NULL) should not happen. It is not OK for library code to crash. This is quite OK for compilers, linkers and other tools, but it isn't for any other code. 
>>> 
>>> Well, except that no, it is not OK for the compiler either, we want to use it as a library (JIT, etc.).
>>> But it comes back to my point: you are against asserting for enforcing “external input” where there should be sanitization done with proper error handling, which does not say anything about the majority of assertions which are not dealing with user input.
>> 
>> I am all for asserting in debug builds. Just not in release builds. 
> 
> Right, we’re on the same page, and that’s what we do in LLVM.
> 
> 
>> And just because you assert, this doesn't mean it is ok to not handle what is being asserted. There is code in clang like:
>> 
>> void do_something(foo *p)
>> {
>>    assert(p);
>>    p->crash();
>> }
>> 
>> This should be:
>> 
>> void do_something(foo *p)
>> {
>>    assert(p);
>>    if (p)
>> 	p->crash();
>> }
> 
> Well to be honest I wouldn’t use a pointer in the first place but a reference in such case.
> Otherwise, I’d be fine with such code inside a private API (inside a LLVM library for example), where you sanitize the input at the API boundary and assert on invariant inside the library.
> 
>>> For instance, it is still not clear to me what’s wrong with the asserting Error class mentioned before.
>> 
>> If someone adds code that succeeds almost all of the time, including in the test suite, and then we release LLDB and someone uses bad debug info and the error condition suddenly fires and they didn't check the error, it is not acceptable to crash.
> 
> This is not what it is about, the assertion fires when the error is *unchecked*, independently of success or failure, for example:
> 
> ErrorOr<Foo> getFoo();
> Foo bar() {
>   auto F = getFoo();
>   return F.getValue();
> }
> 
> Here the developer does not check the returned error from the call to getFoo(). This is a case where bad-things would happen when there is an actual error, but it wouldn’t be caught during testing until the error actually happens.
> 
> The LLVM Error class will *always* assert, even when getFoo() succeeds and there is no actual error.

Then I am fine with it. If it is always enforced, then there is no problem.