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

[Raphael Isemann via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2105912ee0b8: [lldb] Add asserts that prevent construction of cycles in the decl origin… (authored by teemperor).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97300/new/

https://reviews.llvm.org/D97300

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


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
+++ 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 @@
     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 @@
         : 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);
     }
 
@@ -272,6 +280,13 @@
     /// 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;
     }
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -830,6 +830,10 @@
 
   // 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 @@
     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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D97300.326047.patch
Type: text/x-patch
Size: 4558 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20210224/736ceacc/attachment.bin>