> 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