[lldb] Add asserts that prevent construction of cycles in the decl origin tracking

[teemperor]

Summary

LLDB tracks where any imported clang::Decl originally came from via a simple map
from 'imported decl' to 'original decl'. That information is used to later complete parts
of the Decl when more information is requested about a certain Decl (e.g., via the
ExternalASTSource interface from Clang).

When finding the 'original decl' for a given decl, the ASTImporterDelegate essentially
just recursively follows the previously mentioned map from 'imported' to 'original decl'
until it can find any further 'original decl'. The final found decl is then the one that
will be imported. The recursion is necessary as in LLDB we don't just import decls from
one ASTContext to another, but also from one ASTContext to another via a
(potentially temporary) ASTContext. For example, the expression parser creates a temporary
ASTContext for parsing the current expression.

The problem with the recursion is however that if we somehow get a cycle into our mapping,
then the ASTImporterDelegate will just infinite recurse. As the infinite recursion usually
happens after the cycle was already created in a code path such as completing a type, the
crash backtraces we get for these bugs are not very useful. However having the backtrace
where the faulty map entry is created usually makes the code trivial to fix (as there should
be some rogue CopyType call or something similar nearby. See for example D96366).

This patch tries to make these issues easier to track down by putting a bunch of sanity asserts
in the code that fills out the map. All the asserts are just checking that there is no direct cycle
(ASTContext maps to itself) when updating the origin tracking map.

The assert in the ASTImportDelegate constructor is
an lldbassert (which also is getting checked in release builds with disabled asserts) as the code
path there is pretty cold and we can reliably detect a rogue CopyType call from there.

I also had to update some code in ClangASTImporter::ASTImporterDelegate::Imported. This code
already had a safety check for creating a cycle in the origin tracking map, but it still
constructed an ASTImporter while checking for the cycle (by requesting a delegate via GetDelegate
and passing two identical ASTContexts which looks like a rogue CopyType call to the checks).

[shafik]

LGTM

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h

 #include "clang/AST/CharUnits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/CompilerDeclContext.h"
+#include "lldb/Utility/LLDBAssert.h"
 #include "lldb/lldb-types.h"
 
 #include "Plugins/ExpressionParser/Clang/CxxModuleHandler.h"
 
 #include "llvm/ADT/DenseMap.h"
 
 namespace lldb_private {
 
public:
 
   void ForgetDestination(clang::ASTContext *dst_ctx);
   void ForgetSource(clang::ASTContext *dst_ctx, clang::ASTContext *src_ctx);
 
   struct DeclOrigin {
     DeclOrigin() : ctx(nullptr), decl(nullptr) {}
 
     DeclOrigin(clang::ASTContext *_ctx, clang::Decl *_decl)
-        : ctx(_ctx), decl(_decl) {}
+        : ctx(_ctx), decl(_decl) {
+      // The decl has to be in its associated ASTContext.
+      assert(_decl == nullptr || &_decl->getASTContext() == _ctx);
+    }
 
     DeclOrigin(const DeclOrigin &rhs) {
       ctx = rhs.ctx;
       decl = rhs.decl;
     }
 
     void operator=(const DeclOrigin &rhs) {
       ctx = rhs.ctx;
public:
   /// CxxModuleHandler to replace any missing or malformed declarations with
   /// their counterpart from a C++ module.
   struct ASTImporterDelegate : public clang::ASTImporter {
     ASTImporterDelegate(ClangASTImporter &master, clang::ASTContext *target_ctx,
                         clang::ASTContext *source_ctx)
         : clang::ASTImporter(*target_ctx, master.m_file_manager, *source_ctx,
                              master.m_file_manager, true /*minimal*/),
           m_master(master), m_source_ctx(source_ctx) {
+      // Target and source ASTContext shouldn't be identical. Importing AST
+      // nodes within the same AST doesn't make any sense as the whole idea
+      // is to import them to a different AST.
+      lldbassert(target_ctx != source_ctx && "Can't import into itself");
       setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
     }
 
     /// Scope guard that attaches a CxxModuleHandler to an ASTImporterDelegate
     /// and deattaches it at the end of the scope. Supports being used multiple
     /// times on the same ASTImporterDelegate instance in nested scopes.
     class CxxModuleScope {
       /// The handler we attach to the ASTImporterDelegate.
public:
     DelegateMap m_delegates;
 
     NamespaceMetaMap m_namespace_maps;
     MapCompleter *m_map_completer = nullptr;
 
     /// Sets the DeclOrigin for the given Decl and overwrites any existing
     /// DeclOrigin.
     void setOrigin(const clang::Decl *decl, DeclOrigin origin) {
+      // Setting the origin of any decl to itself (or to a different decl
+      // in the same ASTContext) doesn't make any sense. It will also cause
+      // ASTImporterDelegate::ImportImpl to infinite recurse when trying to find
+      // the 'original' Decl when importing code.
+      assert(&decl->getASTContext() != origin.ctx &&
+             "Trying to set decl origin to its own ASTContext?");
+      assert(decl != origin.decl && "Trying to set decl origin to itself?");
       m_origins[decl] = origin;
     }
 
     /// Removes any tracked DeclOrigin for the given decl.
     void removeOrigin(const clang::Decl *decl) { m_origins.erase(decl); }
 
     /// Remove all DeclOrigin entries that point to the given ASTContext.
     /// Useful when an ASTContext is about to be deleted and all the dangling

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

if (D) {
       // the debug info.
       m_decls_to_ignore.insert(*D);
       return *D;
     }
   }
 
   // Check which ASTContext this declaration originally came from.
   DeclOrigin origin = m_master.GetDeclOrigin(From);
+
+  // Prevent infinite recursion when the origin tracking contains a cycle.
+  assert(origin.decl != From && "Origin points to itself?");
+
   // If it originally came from the target ASTContext then we can just
   // pretend that the original is the one we imported. This can happen for
   // example when inspecting a persistent declaration from the scratch
   // ASTContext (which will provide the declaration when parsing the
   // expression and then we later try to copy the declaration back to the
   // scratch ASTContext to store the result).
   // Without this check we would ask the ASTImporter to import a declaration
   // into the same ASTContext where it came from (which doesn't make a lot of
ASTContextMetadataSP to_context_md =
       m_master.GetContextMetadata(&to->getASTContext());
   ASTContextMetadataSP from_context_md =
       m_master.MaybeGetContextMetadata(m_source_ctx);
 
   if (from_context_md) {
     DeclOrigin origin = from_context_md->getOrigin(from);
 
     if (origin.Valid()) {
+      if (origin.ctx != &to->getASTContext()) {
         if (!to_context_md->hasOrigin(to) || user_id != LLDB_INVALID_UID)
-        if (origin.ctx != &to->getASTContext())
           to_context_md->setOrigin(to, origin);
 
         ImporterDelegateSP direct_completer =
             m_master.GetDelegate(&to->getASTContext(), origin.ctx);
 
         if (direct_completer.get() != this)
           direct_completer->ASTImporter::Imported(origin.decl, to);
 
         LLDB_LOG(log,
                  "    [ClangASTImporter] Propagated origin "
                  "(Decl*){0}/(ASTContext*){1} from (ASTContext*){2} to "
                  "(ASTContext*){3}",
                  origin.decl, origin.ctx, &from->getASTContext(),
                  &to->getASTContext());
+      }
     } else {
       if (m_new_decl_listener)
         m_new_decl_listener->NewDeclImported(from, to);
 
       if (!to_context_md->hasOrigin(to) || user_id != LLDB_INVALID_UID)
         to_context_md->setOrigin(to, DeclOrigin(m_source_ctx, from));
 
       LLDB_LOG(log,