> On May 1, 2017, at 4:52 PM, Zachary Turner <ztur...@google.com> wrote:
>
> Yea, grouping the error and the result together is one of the most compelling
> features of it. It's called Expected<T>, so where we would currently write
> something like:
>
> int getNumberOfSymbols(Error &Err) {}
>
> or
>
> Error getNumberOfSymbols(int &Count) {}
>
> You would now write:
>
> Expected<int> getNumberOfSymbols() {
> if (foo) return 1;
> else return make_error<DWARFError>("No symbols!");
> }
>
> and on the caller side you write:
>
> Error processAllSymbols() {
> if (auto Syms = getNumberOfSymbols()) {
> outs() << "There are " << *Syms << " symbols!";
> } else {
> return Syms.takeError();
> // alternatively, you could write:
> // consumeError(Syms.takeError());
> // return Error::success();
> }
> }
>
Interesting.
This pattern doesn't quite work for fetching symbols - maybe that really is
more suitable as a Status than an Error. After all, number of symbols == 0 is
not necessarily an error, there just might not have been any symbols (e.g. a
fully stripped main); and I'm going to work on whatever symbols I get back,
since there's nothing I can do about the ones that didn't make it. I just want
to propagate the error so the user knows that there was a problem.
Jim
>
> On Mon, May 1, 2017 at 4:47 PM Jim Ingham <jing...@apple.com> wrote:
>
> > On May 1, 2017, at 3:29 PM, Zachary Turner <ztur...@google.com> wrote:
> >
> > I'm confused. By having the library assert, you are informed of places
> > where you didn't do a good job of backing from errors, thereby allowing you
> > to do a *better* job.
> >
> > You said this earlier:
> >
> > > 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!
> >
> > But when you're using llvm::Error, no developer is actively thinking about
> > asserts. Nobody is thinking "well the library is going to assert, so my
> > job is done here " because that doesn't make any sense. !!!!It's going to
> > assert even if the operation was successful!!!!
> >
> > Your job can't possibly be done because if you don't check the error, you
> > will assert 100% of the time you execute that codepath. You might as well
> > have just written int x = *nullptr; Surely nobody could agree that their
> > job is done after writing int x = *nullptr; in their code.
> >
> > If you write this:
> >
> > Error foo(int &x) {
> > x = 42;
> > return Error::success();
> > }
> >
> > void bar() {
> > int x;
> > foo(x);
> > cout << x;
> > }
> >
> > Then this code is going to assert. It doesn't matter that no error
> > actually occurred. That is why I'm saying it is strictly a win, no matter
> > what, in all situations. If you forget to check an error code, you
> > *necessarily* aren't doing the best possible job backing out of the code in
> > case an error does occur. Now you will find it and be able to fix it.
>
> Yeah, Lang was just explaining this. I think I was over-reacting to the
> asserts part because llvm's aggressive use of early failure was a real
> problem for lldb. So my hackles go up when something like it comes up again.
>
> In practical terms, lldb quite often uses another measure than the error to
> decide how it's going to proceed. I ask for some symbols, and I get some,
> but at the same time, one of 10 object files had some bad DWARF so an error
> was produced. I'll pass that error along for informational purposes, but I
> don't really care, I'm still going to set breakpoints on all the symbols I
> found. Lang said it is possible to gang something like the "number of
> symbols" and the error, so that checking the number of symbols automatically
> ticks the error box as well. If eventually ever comes we'll have to deal with
> this sort of complication.
>
> As for Error -> Status to avoid confusion, that seems fine, though if you are
> going to do it, I agree with Pavel it would be gross to have "Status error;"
> all over the place.
>
> Jim
>
>
> >
> > On Mon, May 1, 2017 at 3:19 PM Jim Ingham <jing...@apple.com> wrote:
> >
> > > On May 1, 2017, at 3:12 PM, Zachary Turner <ztur...@google.com> wrote:
> > >
> > > 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?
> >
> > Not sure that's entirely relevant. The point is not to make failure points
> > assert then turn them off in production because they shouldn't assert. The
> > point is to make sure that instead of asserting you always do the best job
> > you can backing out from any error.
> >
> > Jim
> >
> >
> > >
> > > 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
