[lldb-dev] Renaming lldb_private::Error
Lang Hames via lldb-dev
lldb-dev at lists.llvm.org
Mon May 1 17:28:23 PDT 2017
For instances where we know that we just want to log errors and continue
with some sensible default, we could introduce a new utility template along
the lines of:
template <typename T>
T logUnwrap(Expected<T> Result, T DefaultValue = T()) {
if (Result)
return std::move(*Result);
else {
log(Result.takeError());
return DefaultValue;
}
}
Now we can write:
T Result = logUnwrap(foo(...));
and have a strong guarantee that errors will be logged, and that we know
the value of Result if an error occurs.
Cheers,
Lang.
On Mon, May 1, 2017 at 4:52 PM, Zachary Turner <zturner at 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();
> }
> }
>
>
> On Mon, May 1, 2017 at 4:47 PM Jim Ingham <jingham at apple.com> wrote:
>
>>
>> > On May 1, 2017, at 3:29 PM, Zachary Turner <zturner at 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 <jingham at apple.com> wrote:
>> >
>> > > On May 1, 2017, at 3:12 PM, Zachary Turner <zturner at 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 <jingham at apple.com> wrote:
>> > >
>> > > > On May 1, 2017, at 2:51 PM, Zachary Turner <zturner at 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 <jingham at apple.com>
>> wrote:
>> > > >
>> > > > > On May 1, 2017, at 12:54 PM, Zachary Turner <zturner at 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 <jingham at 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 <jingham at apple.com>
>> wrote:
>> > > > > >
>> > > > > >>
>> > > > > >> On May 1, 2017, at 11:48 AM, Zachary Turner <
>> zturner at google.com> wrote:
>> > > > > >>
>> > > > > >>
>> > > > > >>
>> > > > > >> On Mon, May 1, 2017 at 11:28 AM Jim Ingham <jingham at 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
>> > > > >
>> > > >
>> > >
>> >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20170501/773bbf88/attachment-0001.html>
More information about the lldb-dev
mailing list