[Lldb-commits] [lldb] 2105912 - [lldb] Add asserts that prevent construction of cycles in the decl origin tracking

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2021-02-24T13:25:49+01:00
New Revision: 2105912ee0b831d5141146b7700c1934c4185bd6

URL: https://github.com/llvm/llvm-project/commit/2105912ee0b831d5141146b7700c1934c4185bd6
DIFF: https://github.com/llvm/llvm-project/commit/2105912ee0b831d5141146b7700c1934c4185bd6.diff

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

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).

Reviewed By: shafik

Differential Revision: https://reviews.llvm.org/D97300

Added: 
    

Modified: 
    lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
    lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index e2601a059bb7..c1c115c1fe74 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -830,6 +830,10 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
 
   // 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
@@ -1088,22 +1092,23 @@ void ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl *from,
     DeclOrigin origin = from_context_md->getOrigin(from);
 
     if (origin.Valid()) {
-      if (!to_context_md->hasOrigin(to) || user_id != LLDB_INVALID_UID)
-        if (origin.ctx != &to->getASTContext())
+      if (origin.ctx != &to->getASTContext()) {
+        if (!to_context_md->hasOrigin(to) || user_id != LLDB_INVALID_UID)
           to_context_md->setOrigin(to, origin);
 
-      ImporterDelegateSP direct_completer =
-          m_master.GetDelegate(&to->getASTContext(), origin.ctx);
+        ImporterDelegateSP direct_completer =
+            m_master.GetDelegate(&to->getASTContext(), origin.ctx);
 
-      if (direct_completer.get() != this)
-        direct_completer->ASTImporter::Imported(origin.decl, to);
+        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());
+        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);

diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
index bf4ad174cf9c..f4ea3af8d9b9 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
@@ -23,6 +23,7 @@
 
 #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"
@@ -148,7 +149,10 @@ class ClangASTImporter {
     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;
@@ -190,6 +194,10 @@ class ClangASTImporter {
         : 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 
diff erent AST.
+      lldbassert(target_ctx != source_ctx && "Can't import into itself");
       setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
     }
 
@@ -272,6 +280,13 @@ class ClangASTImporter {
     /// 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 
diff erent 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;
     }