https://github.com/Michael137 created https://github.com/llvm/llvm-project/pull/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 >From 247f2638be2decf0f806050e90c25c4b664b2f97 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Thu, 17 Oct 2024 17:40:35 +0100 Subject: [PATCH 1/2] [lldb][ClangASTImporter][NFC] Emit a log message when we break MapImported invariant --- .../Clang/ClangASTImporter.cpp | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp index 630ad7e20ab7e0..9a0e354daff712 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp @@ -1136,6 +1136,25 @@ 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 (auto *D = GetAlreadyImportedOrNull(from); D && D != to) + LLDB_LOG(log, + "[ClangASTImporter] WARNING: 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 +1164,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,11 +1175,7 @@ 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); - } + std::string name_string = getDeclName(from); LLDB_LOG(log_ast, "==== [ClangASTImporter][TUDecl: {0:x}] Imported " "({1}Decl*){2:x}, named {3} (from " >From c1e78158074549d83448733a6560fa53fa843041 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Thu, 17 Oct 2024 17:52:07 +0100 Subject: [PATCH 2/2] fixup! add quotes --- .../Plugins/ExpressionParser/Clang/ClangASTImporter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp index 9a0e354daff712..2fbd8e6ca688fc 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp @@ -1151,8 +1151,8 @@ void ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo( if (auto *D = GetAlreadyImportedOrNull(from); D && D != to) LLDB_LOG(log, "[ClangASTImporter] WARNING: overwriting an already imported decl " - "'{0:x}' ('{1}') from '{2:x}' with {3:x}. Likely due to a name " - "conflict when importing {1}.", + "'{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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
