[lldb][ClangASTImporter][NFC] Emit a log message when we break MapImported invariant #112748
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch emits a warning into the expression log when we call The underlying issue is discussed in more detail in: #112566. In a non-asserts build, the last few expression log entries would look as follows: Note how we start "completing" rdar://135551810 Full diff: https://github.com/llvm/llvm-project/pull/112748.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 630ad7e20ab7e0..2fbd8e6ca688fc 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 "
|
labath
approved these changes
Oct 18, 2024
| llvm::raw_string_ostream name_stream(name_string); | ||
| from_named_decl->printName(name_stream); | ||
| } | ||
| std::string name_string = getDeclName(from); |
There was a problem hiding this comment.
You might as well put this call directly into the log message (line 1184)
| return name_string; | ||
| }; | ||
|
|
||
| if (auto *D = GetAlreadyImportedOrNull(from); D && D != to) |
There was a problem hiding this comment.
If this is expensive (I honestly don't know), you may want to put this in a if(log) block.
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Oct 21, 2024
…orted invariant (llvm#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: llvm#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 (cherry picked from commit aa32060)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch emits a warning into the expression log when we call
MapImportedon a decl which has already been imported, but with a newtodestination decl. In asserts builds this would lead to triggering this ASTImporter::MapImported assertion. 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: #112566.
In a non-asserts build, the last few expression log entries would look as follows:
Note how we start "completing"
Foo. Then emit our newWARNING. Shortly after, we crash, and the log abruptly ends.rdar://135551810