Does Xcode ship with a build of LLDB that has asserts enabled? Because if not it shouldn't affect anything. If so, can you shed some light on why?
On Mon, May 1, 2017 at 3:08 PM Jim Ingham <jing...@apple.com> wrote: > > > On May 1, 2017, at 2:51 PM, Zachary Turner <ztur...@google.com> wrote: > > > > I think we agree about the SB layer. You can't have mandatory checking > on things that go through the SB API. I think we could code around that > though. > > > > Regarding the other point, I actually think force checked errors *help* > you think about how to back out and leave the debug session alive. > Specifically because they force you think think about it at all. With > unchecked errors, a caller might forget that a function even returns an > error (or Status) at all, and so they may just call a function and proceed > on assuming it worked as expected. With a checked error, this would never > happen because the first time you called that function in a test, > regardless of whether it passed or failed, you would get an assertion > saying you forgot to check the error. Then you can go back and think about > what the most appropriate thing to do is in that situation, and if the > appropriate thing to do is ignore it and continue, then you can do that. > > > > Most of these error conditions are things that rarely happen > (obviously), and it's hard to get test coverage to make sure the debugger > does the right thing when it does happen. Checked errors is at least a way > to help you identify all the places in your code that you may have > overlooked a possible failure condition. And you can always just > explicitly ignore it. > > > > Sure, it is the policy not the tool to enforce it that really matters. > But for instance lldb supports many debug sessions in one process (a mode > it gets used in all the time under Xcode) and no matter how bad things go > in one debug session, none of the other debug sessions care about that. So > unless you know you're about to corrupt memory in some horrible and > unavoidable way, no action in lldb should take down the whole lldb > session. Playing with tools that do just that - and automatically too - > means you've equipped yourself with something you are going to have to be > very careful handling. > > Jim > > > > On Mon, May 1, 2017 at 2:42 PM Jim Ingham <jing...@apple.com> wrote: > > > > > On May 1, 2017, at 12:54 PM, Zachary Turner <ztur...@google.com> > wrote: > > > > > > The rename is just to avoid the spelling collision. Nothing in > particular leads me to believe that unchecked errors are a source of major > bugs. > > > > > > That said, I have some short term plans to begin making use of some > llvm library classes which deal in llvm::Error, and doing this first should > make those changes less confusing. Additionally I'd like to be able to > start writing new LLDB code against llvm::Error where appropriate, so it > would be nice if this collision weren't present. > > > > > > BTW, I'm curious why you think asserting is still bad even in the test > suite when errors don't need to be checked. > > > > I think I was making a more limited statement that you took it to be. > > > > Errors that should be checked locally because you know locally that it > is fatal not to check them should always be checked - testsuite or no. But > a lot of lldb's surface area goes out to the SB API's, and we don't control > the callers of those. All the errors of that sort can't be checked before > they pass the boundary (and are more appropriate as Status's instead.) The > failure to check those errors shouldn't propagate to the SB API's or we are > just making an annoying API set... So test suite asserting for this class > of errors would not be appropriate. > > > > But a larger point about asserting as a result of errors is that it > makes it seem to the lldb developer like once you've raised an assert on > error your job is done. You've stopped the error from propagating, two > points! For the debugger, you should really be thinking "oh, that didn't > go right, how can I back out of that so I can leave the debug session > alive." There's nothing about force checked errors for code you can > reason on locally that enforces one way of resolving errors or the other. > But IME it does favor the "bag out early" model. > > > > Jim > > > > > > > I think of it as something like this: > > > > > > void foo(int X) { > > > return; > > > } > > > > > > And your compiler giving you a warning that you've got an unused > parameter. So to silence it, you write: > > > > > > void foo(int X) { > > > (void)X; > > > } > > > > > > The point here being, it's only the function foo() that knows whether > the parameter is needed. Just like if you write: > > > > > > Error E = foo(); > > > > > > the function foo() cannot possibly know whether the error needs to be > checked, because it depends on the context in which foo() is called. One > caller might care about the error, while the other doesn't. So foo() > should assume that the caller will check the error (otherwise why even > bother returning one if it's just going to be ignored), and the caller can > explicitly opt out of this behavior by writing: > > > consumeError(foo()); > > > > > > which suppresses the assertion. > > > > > > So yes, the error has to be "checked", but you can "check" it by > explicitly ignoring it at a particular callsite. > > > > > > On Mon, May 1, 2017 at 12:38 PM Jim Ingham <jing...@apple.com> wrote: > > > BTW, I'm interested to know if you have some analysis which leads you > to think that propagating unchecked errors actually is a big problem in > lldb, or are you just doing this to remove a spelling collision? I see a > lot of bugs for lldb come by, but I really haven't seen many that this sort > of forced checking would have fixed. > > > > > > Jim > > > > > > > > > > On May 1, 2017, at 12:36 PM, Jim Ingham <jing...@apple.com> wrote: > > > > > > > >> > > > >> On May 1, 2017, at 11:48 AM, Zachary Turner <ztur...@google.com> > wrote: > > > >> > > > >> > > > >> > > > >> On Mon, May 1, 2017 at 11:28 AM Jim Ingham <jing...@apple.com> > wrote: > > > >> I'm mostly but not entirely tongue in cheek wondering why we aren't > calling llvm::Error llvm::LLVMError, as the lldb error class much preceded > it, but that's a minor point. > > > >> FWIW I think the naming chosen by LLVM is correct. It's intended > to be a generic utility class, extensible enough to be used by anything > that links against LLVM. As such, calling it LLVMError kind of gives off > the false impression that it should only be used by errors that originate > from LLVM, when in fact it's much more general purpose. > > > >> > > > >> > > > >> If it is actually causing confusion (I haven't experienced such > yet) I don't mind typing some extra letters. > > > >> I think that's in part because llvm::Error isn't very prevalent > inside of LLDB (yet). > > > >> > > > >> > > > >> As we've discussed several times in the past, we often use errors > for informational purposes (for instance in the ValueObject system) with no > programmatic requirement they be checked. So the llvm::Error class is not > a drop-in replacement for our uses of lldb_private::Error in subset of its > uses. More generally, the environment the debugger lives in is often > pretty dirty, bad connections to devices, uncertain debug information, > arguments with clang about what types mean, weird user input, etc. But the > job of the debugger is to keep going as well/long as it can in the face of > this. For something like a compiler, if some operation goes bad that whole > execution is likely rendered moot, and so bagging out early is the right > thing to do. For lldb, if the debug info for a frame is all horked up, > users can still resort to memory reading and casts, or some other > workaround, provided the debugger stays alive. This makes me a little > leery of adopting an infrastructure whose default action is to abort on > mishandling. > > > >> Just re-iterating from previous discussions, but it only does that > in debug mode. When you have a release build, it will happily continue on > without aborting. The point of all this is that you catch unhandled errors > immediately the first time you run the test suite. > > > > > > > > Yup, we do that for assertions. But asserting isn't appropriate > even in the testsuite for cases where we don't require the errors be > checked. > > > > > > > >> > > > >> Even if you have a bad connection, uncertain debug information, etc > you still have to propagate that up the callstack some number of levels > until someone knows what to do. All this class does is make sure (when in > debug mode) that you're doing that instead of silently ignoring some > condition. > > > >> > > > >> That said, it certainly seems plausible that we could come up with > some kind of abstraction for informational status messages. With that in > mind, I'll change my original renaming proposal from LLDBError to Status. > This way we will have llvm::Error and lldb_private::Status. > > > > > > > > That seems better. > > > > > > > >> > > > >> In the future, perhaps we can discuss with Lang and the larger > community about whether such a class makes in LLVM as well. Maybe there's > a way to get both checked and unchecked errors into LLVM using a single > consistent interface. But at least then the person who generates the error > is responsible for deciding how important it is. > > > >> > > > > > > > > It's not "how important it is" but "does this error need to be dealt > with programmatically proximate to the code that produces it." For > instance, if an error makes it to the SB API level - something that is > quite appropriate for the SBValues for instance, we wouldn't want to use an > llvm::Error. After all forcing everybody to check this at the Python layer > would be really annoying. I guess you could work around this by > hand-checking off any error when you go from lldb_private -> SBError. But > that seems like now you're just pretending to be doing something you > aren't, which I don't think is helpful. > > > > > > > > Probably better as you say to make everything into > lldb_private::Status behaving as it does now, to side-step the name > collision, and then start with all the uses where the error doesn't > propagate very far, and try converting those to use llvm::Error and working > judiciously out from there. 'Course you can't change the SB API names, so > there will always be a little twist there. > > > > > > > > Jim > > > > > > > >> > > > >> > > > >> BTW, I don't think the comment Lang cited had to do with replacing > the errors with some other error backend. It was more intended to handle a > problem that came up with gdb where we tried to multiplex all various > system error numbers into one single error. lldb errors have a flavor > (eErrorTypePosix, eErrorTypeWin32, etc) which allows you to use each native > error number by annotating it with the flavor. > > > >> > > > >> FWIW, using the llvm::Error model, the way this is handled is by > doing something like this: > > > >> > > > >> return make_error<WindowsError>(::GetLastError()); > > > >> > > > >> return make_error<ErrnoError>(errno); > > > >> > > > >> but it is general enough to handle completely different categories > of errors as well, so you can "namespace" out your command interpreter > errors, debug info errors, etc. > > > >> > > > >> return make_error<CommandInterpreterError>("Incorrect command > usage"); > > > >> > > > >> return make_error<DWARFFormatError>("Invalid DIE specification"); > > > >> > > > >> etc > > > > > > >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev