Page MenuHomePhabricator

[Target] Generalize some behavior in Thread
ClosedPublic

Authored by xiaobai on May 9 2019, 7:58 PM.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

xiaobai created this revision.May 9 2019, 7:58 PM

If you really are going to support many languages you need to figure out how to tell folks what really happened with more specificity. For instance, you can use C++ to throw anything, so you could throw an ObjC object from a C++ exception throw. So you need to distinguish between the language of the exception thrown (which is captured by the ValueObject you return) and the language runtime throwing the language. So we need a way to query that. Also, there's no formula reason why you couldn't have two languages throwing an exception at the same time (for instance if a C++ exception is unwinding the stack and the destructor of some ObjC object that is getting destroyed throws an NSException, etc... So there needs to be some way to handle that.

This change isn't wrong but it gives a false impression of generality which makes it less well motivated.

If you really are going to support many languages you need to figure out how to tell folks what really happened with more specificity.

I agree.

For instance, you can use C++ to throw anything, so you could throw an ObjC object from a C++ exception throw. So you need to distinguish between the language of the exception thrown (which is captured by the ValueObject you return) and the language runtime throwing the language. So we need a way to query that. Also, there's no formula reason why you couldn't have two languages throwing an exception at the same time (for instance if a C++ exception is unwinding the stack and the destructor of some ObjC object that is getting destroyed throws an NSException, etc... So there needs to be some way to handle that.

This change isn't wrong but it gives a false impression of generality which makes it less well motivated.

Better support for exception handling is definitely something we should work towards. This patch doesn't actually change behavior, but I understand your concern that generalizing will make it look like things are better supported than they actually are. My motivation behind this change is removing language-specific knowledge from Thread, which is a goal that I think is worth pursuing. I could preserve/modify the comments that were there noting that only ObjC works right now.

jingham accepted this revision.May 10 2019, 12:00 PM

With appropriate comments, this seems fine.

This revision is now accepted and ready to land.May 10 2019, 12:00 PM
xiaobai updated this revision to Diff 199048.May 10 2019, 12:14 PM

Add comments to give better context

JDevlieghere accepted this revision.May 13 2019, 2:17 PM

Unfortunately I can't land this patch as-is. With this patch applied, TestObjCExceptions fails. It looks like c++ exceptions are supported at the bare minimum as a part of the objc exception handling logic.

It was failing 2 subtests: test_objc_exceptions_at_throw and test_cxx_exceptions_at_abort. The first test was failing because of a minor bug that I will include a fix for in this patch. The second test is failing because of an assertion that the current exception we get should not be valid, but now it is valid. That is because we are able to get the exception object from the C++ runtime now. However, the assertion that the exception backtrace isn't valid still holds (since that isn't implemented for C++). I'm going to update this patch to reflect this new information in the near future and then request another review from y'all.

xiaobai updated this revision to Diff 199364.May 13 2019, 9:00 PM
  • Fix minor bug in ItaniumABILanguageRuntime
  • Modify test to accomodate new behavior
JDevlieghere accepted this revision.May 14 2019, 3:51 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2019, 6:46 PM
Herald added a subscriber: teemperor. · View Herald Transcript