[Target] Generalize some behavior in Thread

[xiaobai]

Summary

I don't think there's a good reason for this behavior to be considered
ObjC-specific. We can generalize this.

[jingham]

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.

[xiaobai]

In D61776#1498225, @jingham wrote:

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]

With appropriate comments, this seems fine.

[xiaobai]

Add comments to give better context

[xiaobai]

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]

• Fix minor bug in ItaniumABILanguageRuntime
• Modify test to accomodate new behavior

packages/Python/lldbsuite/test/lang/objc/exceptions/TestObjCExceptions.py

def test_cxx_exceptions_at_abort(self):
         self.assertTrue(target, VALID_TARGET)
 
         self.runCmd("run 2")
 
         # We should be stopped at pthread_kill because of an unhandled exception
         self.expect("thread list",
             substrs=['stopped', 'stop reason = signal SIGABRT'])
 
-        self.expect('thread exception', substrs=[])
+        self.expect('thread exception', substrs=['exception ='])
 
         process = self.dbg.GetSelectedTarget().process
         thread = process.GetSelectedThread()
 
-        # C++ exceptions are not exposed in the API (yet).
-        self.assertFalse(thread.GetCurrentException().IsValid())
+        self.assertTrue(thread.GetCurrentException().IsValid())
+
+        # C++ exception backtraces are not exposed in the API (yet).
         self.assertFalse(thread.GetCurrentExceptionBacktrace().IsValid())

source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp

if (func_call_ret != eExpressionCompleted || !error.Success()) {
     return ValueObjectSP();
   }
 
   size_t ptr_size = m_process->GetAddressByteSize();
   addr_t result_ptr = results.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
   addr_t exception_addr =
       m_process->ReadPointerFromMemory(result_ptr - ptr_size, error);
 
+  if (!error.Success()) {
+    return ValueObjectSP();
+  }
+
   lldb_private::formatters::InferiorSizedWord exception_isw(exception_addr,
                                                             *m_process);
   ValueObjectSP exception = ValueObject::CreateValueObjectFromData(
       "exception", exception_isw.GetAsData(m_process->GetByteOrder()), exe_ctx,
       voidstar);
   exception = exception->GetDynamicValue(eDynamicDontRunTarget);
 
   return exception;

source/Target/Thread.cpp

 }
 
 ValueObjectSP Thread::GetCurrentException() {
   if (auto frame_sp = GetStackFrameAtIndex(0))
     if (auto recognized_frame = frame_sp->GetRecognizedFrame())
       if (auto e = recognized_frame->GetExceptionObject())
         return e;
 
-  // FIXME: For now, only ObjC exceptions are supported. This should really
-  // iterate over all language runtimes and ask them all to give us the current
-  // exception.
-  if (auto runtime = GetProcess()->GetObjCLanguageRuntime())
+  // NOTE: Even though this behavior is generalized, only ObjC is actually
+  // supported at the moment.
+  for (unsigned lang = eLanguageTypeUnknown; lang < eNumLanguageTypes; lang++) {
+    if (auto runtime = GetProcess()->GetLanguageRuntime(
+            static_cast<lldb::LanguageType>(lang)))
       if (auto e = runtime->GetExceptionObjectForThread(shared_from_this()))
         return e;
+  }
 
   return ValueObjectSP();
 }
 
 ThreadSP Thread::GetCurrentExceptionBacktrace() {
   ValueObjectSP exception = GetCurrentException();
-  if (!exception) return ThreadSP();
+  if (!exception)
+    return ThreadSP();
 
-  // FIXME: For now, only ObjC exceptions are supported. This should really
-  // iterate over all language runtimes and ask them all to give us the current
-  // exception.
-  auto runtime = GetProcess()->GetObjCLanguageRuntime();
-  if (!runtime) return ThreadSP();
+  // NOTE: Even though this behavior is generalized, only ObjC is actually
+  // supported at the moment.
+  for (unsigned lang = eLanguageTypeUnknown; lang < eNumLanguageTypes; lang++) {
+    if (auto runtime = GetProcess()->GetLanguageRuntime(
+            static_cast<lldb::LanguageType>(lang)))
+      if (auto bt = runtime->GetBacktraceThreadFromException(exception))
+        return bt;
+  }
 
-  return runtime->GetBacktraceThreadFromException(exception);
+  return ThreadSP();
 }