This is an archive of the discontinued LLVM Phabricator instance.

[Symbol] Use llvm::Expected when getting TypeSystems
ClosedPublic

Authored by xiaobai on Jul 22 2019, 3:40 PM.

Details

Summary

This commit achieves the following:

  • Functions used to return a TypeSystem * return an llvm::Expected<TypeSystem *> now. This means that the result of a call is always checked, forcing clients to move more carefully.
  • TypeSystemMap::GetTypeSystemForLanguage will either return an Error or a non-null pointer to a TypeSystem.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.Jul 22 2019, 3:40 PM
JDevlieghere added inline comments.Jul 22 2019, 4:22 PM
source/Breakpoint/Watchpoint.cpp
45 ↗(On Diff #211218)
LLDB_LOG(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_WATCHPOINTS), "Watchpoint::Watchpoint(): Failed to set type.\nReason: {}", llvm::toString(type_system_or_err.takeError())
source/Core/ValueObjectRegister.cpp
261 ↗(On Diff #211218)

As a general note: I think it's good practice to use llvm::consumeError when discarding the error to make it explicit that it's not handled. This also makes it easier down the line to handle the error, e.g. by logging it.

source/Expression/Materializer.cpp
875 ↗(On Diff #211218)

Another possible alternative would be to join the errors (llvm::joinErrors) and use the resulting error to initialize the status. Not sure if that makes things better here though.

source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp
105 ↗(On Diff #211218)

Seems like this could be a lot less complex with an early return:

if (!m_cu) {
    return llvm::make_error<llvm::StringError>(
        "Unable to get TypeSystem, no compilation unit available",
        llvm::inconvertibleErrorCode());
}
return m_cu->GetTypeSystem();
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
744 ↗(On Diff #211218)

Remove the braces for consistency with the ifs below.

labath added a subscriber: labath.Jul 23 2019, 12:18 AM

The intention is for the pointer in Expected<TypeSystem *> to be non-null in the success case, right? If that's true, then I'd suggest to make that explicit by returning a reference (Expected<TypeSystem&>) instead.

source/Core/ValueObjectRegister.cpp
261 ↗(On Diff #211218)

This is not just good practice, it is actually required. An simply checking the bool-ness of the Expected object will not set the "checked" flag of the error object, and it will assert when it is destroyed (if asserts are enabled).

263 ↗(On Diff #211218)

If you returned a reference then you also wouldn't need this extra variable to avoid weird double-deref.

jdoerfert resigned from this revision.Jul 23 2019, 6:34 AM
xiaobai marked 4 inline comments as done.Jul 23 2019, 3:56 PM
xiaobai added inline comments.
source/Core/ValueObjectRegister.cpp
261 ↗(On Diff #211218)

So what you're saying is do not define it in an if condition, but rather get the Expected and if it isn't valid then we consume the error, else use the type system. Is that right?

source/Expression/Materializer.cpp
875 ↗(On Diff #211218)

I'm not sure either, I'll try it and see what it looks like.

source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp
105 ↗(On Diff #211218)

Hmm, yeah that looks better.

labath added inline comments.Jul 24 2019, 12:22 AM
source/Core/ValueObjectRegister.cpp
261 ↗(On Diff #211218)

Yes, although technically, the value can still be defined in the if-condition, as its scope extends into the else clause. So the following code is perfectly valid (if slightly surprising):

if (auto expected_foo = maybe_get_foo())
  do_stuff(*expected_foo);
else
  log(expected_foo.takeError());
source/Expression/Materializer.cpp
875 ↗(On Diff #211218)

I'm not a fan of joined errors, and this does not sound like the right place to start using them, as these aren't two separate errors that we've encountered, but rather one error that ends up causing another error to happen. So the best way to model this IMO would be to use nested errors the same way that Java has nested exceptions (but I don't think it's worth doing that here).

After going through this and modifying this patch, I can't help but wonder if llvm::Optional<TypeSystem &> would be more appropriate. There are plenty of instances where it's not a hard error if you can't get a TypeSystem and the appropriate action is probably just to log and move on. I am conflicted because I like how Expected forces you to be more rigorous with error handling but I can't help but feel it is the wrong abstraction. Thoughts?

After going through this and modifying this patch, I can't help but wonder if llvm::Optional<TypeSystem &> would be more appropriate. There are plenty of instances where it's not a hard error if you can't get a TypeSystem and the appropriate action is probably just to log and move on. I am conflicted because I like how Expected forces you to be more rigorous with error handling but I can't help but feel it is the wrong abstraction. Thoughts?

I think an Optional would be fine. We can always create an Error when necessary, with the trade-off of being a little less precision about the root cause, which honestly doesn't seem that informative anyway.

After going through this and modifying this patch, I can't help but wonder if llvm::Optional<TypeSystem &> would be more appropriate. There are plenty of instances where it's not a hard error if you can't get a TypeSystem and the appropriate action is probably just to log and move on. I am conflicted because I like how Expected forces you to be more rigorous with error handling but I can't help but feel it is the wrong abstraction. Thoughts?

I think an Optional would be fine. We can always create an Error when necessary, with the trade-off of being a little less precision about the root cause, which honestly doesn't seem that informative anyway.

I'm not that familiar with type systems, so I don't know if an explicit error value is useful. Adopting Error/Expected initially has a slightly higher barrier because the surrounding code does not know about Errors, and so you have to do something explicit and verbose to the error, and then create a default value of the return type anyway. But, as the surrounding code adopts Errors, the verboseness should disappear. This, of course, assumes that we want to adopt Errors here and in the surrounding code.

What I am familiar with though is the Optional template, I can note that there is no such thing as an Optional<T&> (the Optional template does not play well with references). You could make that a thing, but an Optional<T&> is essentially just a T *, which is what we have now :D. And I wouldn't want Optional<T*> as that just creates more ambiguity.

Thinking ahead I am wondering if we could just arrange things such that the TypeSystem creation always succeeds. One day, I am hoping to arrange is so that a Module will always have an associated ObjectFile and a SymbolFile (by avoiding creating a Module if the ObjectFile construction fails, and creating an "empty" SymbolFile if needed). At that point being able to always create a type system would not seem unreasonable. I don't think any of this is directly relevant here and now, but it may speak against introducing Expected here (if you agree with my vision, that is).

It seems to me part of the goal of Alex's efforts is to make it possible for a given lldb to have or not have support for any particular type system. In some future day they might even be live pluggable, so that which ones get loaded would be a user choice. In that future, it is never an error to not have a given type system. And even if lldb has builtin code to support a given type system, I may not want to pay the cost to construct it. If I'm debugging ObjC code in a program that also supports swift, I might want to tell lldb not to bother with swift types.

If that seems reasonable, then TypeSystems are really optional, and you should always be prepared for one not to exist. IIUC that's better expressed by Optional than Expected.

I'm going to update this diff with what I changed to give y'all a better idea of what has been changed. I shoulda done that to begin with... :P

After going through this and modifying this patch, I can't help but wonder if llvm::Optional<TypeSystem &> would be more appropriate. There are plenty of instances where it's not a hard error if you can't get a TypeSystem and the appropriate action is probably just to log and move on. I am conflicted because I like how Expected forces you to be more rigorous with error handling but I can't help but feel it is the wrong abstraction. Thoughts?

I think an Optional would be fine. We can always create an Error when necessary, with the trade-off of being a little less precision about the root cause, which honestly doesn't seem that informative anyway.

I'm not that familiar with type systems, so I don't know if an explicit error value is useful. Adopting Error/Expected initially has a slightly higher barrier because the surrounding code does not know about Errors, and so you have to do something explicit and verbose to the error, and then create a default value of the return type anyway. But, as the surrounding code adopts Errors, the verboseness should disappear. This, of course, assumes that we want to adopt Errors here and in the surrounding code.

What I am familiar with though is the Optional template, I can note that there is no such thing as an Optional<T&> (the Optional template does not play well with references). You could make that a thing, but an Optional<T&> is essentially just a T *, which is what we have now :D. And I wouldn't want Optional<T*> as that just creates more ambiguity.

Thinking ahead I am wondering if we could just arrange things such that the TypeSystem creation always succeeds. One day, I am hoping to arrange is so that a Module will always have an associated ObjectFile and a SymbolFile (by avoiding creating a Module if the ObjectFile construction fails, and creating an "empty" SymbolFile if needed). At that point being able to always create a type system would not seem unreasonable. I don't think any of this is directly relevant here and now, but it may speak against introducing Expected here (if you agree with my vision, that is).

Yeah, switching to an Optional<T&> or Optional<T*> doesn't seem like an actual tangible improvement over what we have now... And an Expected<T&> seems to at least promote better error handling. The initial cost is indeed high though. :P
Eventually having a system such that makes TypeSystem creation always succeed would be much nicer. Though I agree, I think that using an Expected here is better than returning a pointer since TypeSystem creation can fail right now. I don't think it should be too difficult to change back if and when we get to that point.

It seems to me part of the goal of Alex's efforts is to make it possible for a given lldb to have or not have support for any particular type system. In some future day they might even be live pluggable, so that which ones get loaded would be a user choice. In that future, it is never an error to not have a given type system. And even if lldb has builtin code to support a given type system, I may not want to pay the cost to construct it. If I'm debugging ObjC code in a program that also supports swift, I might want to tell lldb not to bother with swift types.

If that seems reasonable, then TypeSystems are really optional, and you should always be prepared for one not to exist. IIUC that's better expressed by Optional than Expected.

Yes, that is definitely a part of my goal. That's why I felt Optional better suited this than Expected... but I also feel that Expected forces people to be more careful.

xiaobai updated this revision to Diff 212031.Jul 26 2019, 5:51 PM

Address comments:

  • Use Expected<TypeSystem&>
  • Did some formatting
  • Made error checking more explicit

I have one remark about the consumeError+LLDB_LOG pattern. As for whether this is better than status quo or not, I still don't have an opinion on that. :)

source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
80–84 ↗(On Diff #212031)

I don't believe this will work. Once you consume the error, it is left in an indeterminate state (which happens to be the "success" state, but it's best not to rely on it).
If you do want to log the error (which I do recommend), then you can use the LLDB_LOG_ERROR macro. This one will clear the error for you, and it will do so even if logging is disabled. I.e., if you log the error, there's no need for an additional consumeError call.

xiaobai updated this revision to Diff 212210.Jul 29 2019, 12:05 PM

Fix incorrect error logging pattern

I have one remark about the consumeError+LLDB_LOG pattern. As for whether this is better than status quo or not, I still don't have an opinion on that. :)

Thanks for pointing out LLDB_LOG_ERROR. I updated this change so people could have a chance to see what the change would look like so they have something concrete to look at instead of just speculating what it could look like.

JDevlieghere added inline comments.Jul 29 2019, 12:58 PM
source/API/SBModule.cpp
493 ↗(On Diff #212210)

No else before return?

525 ↗(On Diff #212031)

Any reason you do this instead of

if (!type_system_or_err) {
  llvm::consumeError(type_system_or_err.takeError());
...
xiaobai updated this revision to Diff 212232.Jul 29 2019, 2:44 PM

Small logic change in SBModule

xiaobai added inline comments.Jul 29 2019, 2:44 PM
source/API/SBModule.cpp
525 ↗(On Diff #212031)

No reason in particular. Is there a benefit in doing so or something I'm misunderstanding?

JDevlieghere added inline comments.Jul 29 2019, 3:21 PM
source/API/SBModule.cpp
525 ↗(On Diff #212031)

I like it because it's shorter and a bit easier to read. Probably it doesn't matter. If this works I don't want to hold up your change because of it.

JDevlieghere accepted this revision.Jul 29 2019, 3:26 PM
This revision is now accepted and ready to land.Jul 29 2019, 3:26 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 3:11 PM