> On Sep 20, 2016, at 10:47 AM, Mehdi Amini <mehdi.am...@apple.com> wrote:
>> On Sep 20, 2016, at 10:33 AM, Greg Clayton <gclay...@apple.com> wrote:
>>> On Sep 19, 2016, at 2:46 PM, Mehdi Amini <mehdi.am...@apple.com> wrote:
>>>> On Sep 19, 2016, at 2:34 PM, Greg Clayton <gclay...@apple.com> wrote:
>>>>> On Sep 19, 2016, at 2:20 PM, Mehdi Amini <mehdi.am...@apple.com> wrote:
>>>>>> On Sep 19, 2016, at 2:02 PM, Greg Clayton via lldb-dev 
>>>>>> <lldb-dev@lists.llvm.org> 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.
>>>>>>>                 • 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. 

lldb-dev mailing list

Reply via email to