> 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
lldb-dev@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
