[Lldb-commits] [lldb] aa32060 - [lldb][ClangASTImporter][NFC] Emit a log message when we break MapImported invariant (#112748)

[via lldb-commits]

Author: Michael Buch
Date: 2024-10-19T10:08:29+01:00
New Revision: aa320600e2b7136f5156dd0c31f98ec0f8d5bce1

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

LOG: [lldb][ClangASTImporter][NFC] Emit a log message when we break MapImported invariant (#112748)

This patch emits a warning into the expression log when we call
`MapImported` on a decl which has already been imported, but with a new
`to` destination decl. In asserts builds this would lead to triggering
this [ASTImporter::MapImported
assertion](https://github.com/llvm/llvm-project/blob/6d7712a70c163d2ae9e1dc928db31fcb45d9e404/clang/lib/AST/ASTImporter.cpp#L10493-L10494).
In no-asserts builds we will likely crash, in potentially non-obvious
ways. The hope is that the log message will help in diagnosing this type
of issue in the field.

The underlying issue is discussed in more detail in:
https://github.com/llvm/llvm-project/pull/112566.

In a non-asserts build, the last few expression log entries would look
as follows:
```
     CompleteTagDecl on (ASTContext*)scratch ASTContext Completing (TagDecl*)0x00000001132d31d0 named Foo
       CTD Before:
CXXRecordDecl 0x1132d31d0 <<invalid sloc>> <invalid sloc> <undeserialized declarations> struct Foo

 [ClangASTImporter] WARNING: overwriting an already imported decl '0x000000014378fd80' ('Foo') from '0x0000000143790c00' with 0x00000001132d31d0. Likely due to a name conflict when importing 'Foo'.
     [ClangASTImporter] Imported (FieldDecl*)0x0000000143790220, named service (from (Decl*)0x0000000143791270), metadata 271
     [ClangASTImporter] Decl has no origin information in (ASTContext*)0x00000001132c8c00
 FindExternalLexicalDecls on (ASTContext*)0x0000000143c1f600 'scratch ASTContext' in 'Foo' (CXXRecordDecl*)0x000000014378FD80
   FELD Original decl (ASTContext*)0x00000001132c8c00 (Decl*)0x0000000143790c00:
CXXRecordDecl 0x143790c00 <<invalid sloc>> <invalid sloc> struct Foo definition
|-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
| |-DefaultConstructor exists trivial needs_implicit
| |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| |-MoveConstructor exists simple trivial needs_implicit
| |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
| |-MoveAssignment exists simple trivial needs_implicit
| `-Destructor simple irrelevant trivial needs_implicit
|-FieldDecl 0x143791270 <<invalid sloc>> <invalid sloc> service 'Service *'
`-FieldDecl 0x1437912c8 <<invalid sloc>> <invalid sloc> mach_endpoint 'int'

   FELD Adding [to CXXRecordDecl Foo] lexical FieldDecl FieldDecl 0x143791270 <<invalid sloc>> <invalid sloc> service 'Service *'

   FELD Adding [to CXXRecordDecl Foo] lexical FieldDecl FieldDecl 0x1437912c8 <<invalid sloc>> <invalid sloc> mach_endpoint 'int'

     [ClangASTImporter] Imported (FieldDecl*)0x0000000143790278, named mach_endpoint (from (Decl*)0x00000001437912c8), metadata 280
     [ClangASTImporter] Decl has no origin information in (ASTContext*)0x00000001132c8c00
```
Note how we start "completing" `Foo`. Then emit our new `WARNING`.
Shortly after, we crash, and the log abruptly ends.

rdar://135551810

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 630ad7e20ab7e0..db9a6dd197b3a6 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -1136,6 +1136,29 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
 
 void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
     clang::Decl *to, clang::Decl *from) {
+  Log *log = GetLog(LLDBLog::Expressions);
+
+  auto getDeclName = [](Decl const *decl) {
+    std::string name_string;
+    if (auto const *from_named_decl = dyn_cast<clang::NamedDecl>(decl)) {
+      llvm::raw_string_ostream name_stream(name_string);
+      from_named_decl->printName(name_stream);
+    }
+
+    return name_string;
+  };
+
+  if (log) {
+    if (auto *D = GetAlreadyImportedOrNull(from); D && D != to) {
+      LLDB_LOG(
+          log,
+          "[ClangASTImporter] ERROR: overwriting an already imported decl "
+          "'{0:x}' ('{1}') from '{2:x}' with '{3:x}'. Likely due to a name "
+          "conflict when importing '{1}'.",
+          D, getDeclName(from), from, to);
+    }
+  }
+
   // We might have a forward declaration from a shared library that we
   // gave external lexical storage so that Clang asks us about the full
   // definition when it needs it. In this case the ASTImporter isn't aware
@@ -1145,8 +1168,6 @@ void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
   // tell the ASTImporter that 'to' was imported from 'from'.
   MapImported(from, to);
 
-  Log *log = GetLog(LLDBLog::Expressions);
-
   if (llvm::Error err = ImportDefinition(from)) {
     LLDB_LOG_ERROR(log, std::move(err),
                    "[ClangASTImporter] Error during importing definition: {0}");
@@ -1158,18 +1179,13 @@ void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(
       to_tag->setCompleteDefinition(from_tag->isCompleteDefinition());
 
       if (Log *log_ast = GetLog(LLDBLog::AST)) {
-        std::string name_string;
-        if (NamedDecl *from_named_decl = dyn_cast<clang::NamedDecl>(from)) {
-          llvm::raw_string_ostream name_stream(name_string);
-          from_named_decl->printName(name_stream);
-        }
         LLDB_LOG(log_ast,
                  "==== [ClangASTImporter][TUDecl: {0:x}] Imported "
                  "({1}Decl*){2:x}, named {3} (from "
                  "(Decl*){4:x})",
                  static_cast<void *>(to->getTranslationUnitDecl()),
-                 from->getDeclKindName(), static_cast<void *>(to), name_string,
-                 static_cast<void *>(from));
+                 from->getDeclKindName(), static_cast<void *>(to),
+                 getDeclName(from), static_cast<void *>(from));
 
         // Log the AST of the TU.
         std::string ast_string;