[LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer

[avogelsgesang]

Summary

The pretty printer for std::coroutine_handle was running into

Assertion failed: (target_ctx != source_ctx && "Can't import into itself")

from ClangASTImporter.h, line 270.

This commit fixes the issue by removing the CopyType call from the
pretty printer. While this call was necessary in the past, it seems to
be no longer required, at least all test cases are still passing. Maybe
something changed in the meantime around the handling of TypesystemClang
instances. I don't quite understand why CopyType was necessary earlier.

I am not sure how to add a regression test for this, though. It seems
the issue is already triggered by the exising TestCoroutineHandle.py,
but API tests seem to ignore all violations of lldbassert and still
report the test as "passed", even if assertions were triggered

[aprantl]

This change makes me a little nervous, but mostly because I don't quite understand the design here.
So ASTImporter does not have a check whether src context and dst context are identical?

[avogelsgesang]

This change makes me a little nervous, but mostly because I don't quite understand the design here.
So ASTImporter does not have a check whether src context and dst context are identical?

I am completely with you. I also don't understand the design of ASTImporter.
I had to learn the hard way that there are multiple ASTs around at the same time (one per object file + one for the "debugging session" in which all temporary types etc. are created).
But I don't quite understand their lifetimes, and which types of cross-AST linking are supported and in which cases types need to be copied over.

In that sense, I see this commit mostly as a basis for discussions, but I am not completely confident that this is the right solution. I would hope to find someone more experienced with lldb who can point me in the right direction :)

[labath]

Well.. I don't believe we have any ASTImporter calls in any of the other pretty printers, so I think this change is good. And if it causes some other issues, then maybe we need to look at solving them in some other way...

[avogelsgesang]

Agree, other pretty-printers also don't use ASTImporter. I think the call was still needed originally in https://github.com/llvm/llvm-project/commit/01f4c305fae9ff2f165ce0f635a90f8e2292308c, because the promise_type was combined with newly created AST types which were created in a different AST-instance (see GetCoroutineFrameType in this old version of the code). My interpretation now is: Building struct types which refer to types from other AST instances is not supported. Returning synthetic children with types from different ASTs is supported, thoguh.

I will wait until Wednesday morning before merging this fix, to give other potentially interested reviewers some time. After merging this to main, I will open a backport request for LLDB-16.

I am still a bit concerned that the lldbassert was not caught by the existing tests. It seems the tests are ignoring lldbassert given that those asserts only print some information to stderr but don't actually terminate lldb? I assume this is something which should be fixed globally for all API test? Do you have an idea how I could fix this?

[aprantl]

Well.. I don't believe we have any ASTImporter calls in any of the other pretty printers, so I think this change is good. And if it causes some other issues, then maybe we need to look at solving them in some other way...

Okay I guess I can live with that reasoning ...

[avogelsgesang]

@labath @aprantl Thanks for your reviews! I would like to backport this fix together with the fix from https://reviews.llvm.org/D132815 to the 16.0 release branch now. For that I would need your approval on https://github.com/llvm/llvm-project-release-prs/pull/254

lldb/source/Plugins/Language/CPlusPlus/Coroutines.h

 
 #include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/TypeSummary.h"
 #include "lldb/DataFormatters/TypeSynthetic.h"
 #include "lldb/Utility/Stream.h"
 
 namespace lldb_private {
 
-class ClangASTImporter;
-
 namespace formatters {
 
 /// Summary provider for `std::coroutine_handle<T>` from  libc++, libstdc++ and
 /// MSVC STL.
 bool StdlibCoroutineHandleSummaryProvider(ValueObject &valobj, Stream &stream,
                                           const TypeSummaryOptions &options);
 
 /// Synthetic children frontend for `std::coroutine_handle<promise_type>` from
public:
   bool MightHaveChildren() override;
 
   size_t GetIndexOfChildWithName(ConstString name) override;
 
 private:
   lldb::ValueObjectSP m_resume_ptr_sp;
   lldb::ValueObjectSP m_destroy_ptr_sp;
   lldb::ValueObjectSP m_promise_ptr_sp;
-  std::unique_ptr<lldb_private::ClangASTImporter> m_ast_importer;
 };
 
 SyntheticChildrenFrontEnd *
 StdlibCoroutineHandleSyntheticFrontEndCreator(CXXSyntheticChildren *,
                                               lldb::ValueObjectSP);
 
 } // namespace formatters
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_LANGUAGE_CPLUSPLUS_COROUTINES_H

lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp

 //===-- Coroutines.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Coroutines.h"
 
-#include "Plugins/ExpressionParser/Clang/ClangASTImporter.h"
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/VariableList.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
if (frame_ptr_addr == 0) {
     stream.Printf("coro frame = 0x%" PRIx64, frame_ptr_addr);
   }
 
   return true;
 }
 
 lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
     StdlibCoroutineHandleSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp),
-      m_ast_importer(std::make_unique<ClangASTImporter>()) {
+    : SyntheticChildrenFrontEnd(*valobj_sp) {
   if (valobj_sp)
     Update();
 }
 
 lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
     ~StdlibCoroutineHandleSyntheticFrontEnd() = default;
 
 size_t lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
bool lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::
   if (!promise_type)
     return false;
 
   // Try to infer the promise_type if it was type-erased
   if (promise_type.IsVoidType()) {
     if (Function *destroy_func =
             ExtractDestroyFunction(target_sp, frame_ptr_addr)) {
       if (CompilerType inferred_type = InferPromiseType(*destroy_func)) {
-        // Copy the type over to the correct `TypeSystemClang` instance
-        promise_type = m_ast_importer->CopyType(*ast_ctx, inferred_type);
+        promise_type = inferred_type;
       }
     }
   }
 
   // If we don't know the promise type, we don't display the `promise` member.
   // `CreateValueObjectFromAddress` below would fail for `void` types.
   if (promise_type.IsVoidType()) {
     return false;