-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[lldb][ClangExpressionParser] Reset DiagnosticManager before we create persistent variables #160074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e persistent variables Here's an example crash that we've seen sporadically over the years: ``` 0 libsystem_kernel.dylib 0x19d392388 __pthread_kill + 8 1 libsystem_pthread.dylib 0x19d3cb88c pthread_kill + 296 2 libsystem_c.dylib 0x19d29cd04 raise + 32 3 LLDB 0x112cbbc94 SignalHandler(int, __siginfo*, void*) + 324 4 libsystem_platform.dylib 0x19d4056a4 _sigtramp + 56 5 LLDB 0x110dcbd38 clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 1216 6 LLDB 0x110dcbd38 clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 1216 7 LLDB 0x10de12128 ClangDiagnosticManagerAdapter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 332 8 LLDB 0x1121eb3dc clang::DiagnosticIDs::ProcessDiag(clang::DiagnosticsEngine&) const + 200 9 LLDB 0x1121e67a0 clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) + 100 10 LLDB 0x111d766cc IsStructurallyEquivalent(clang::StructuralEquivalenceContext&, clang::FieldDecl*, clang::FieldDecl*, clang::QualType) + 1568 11 LLDB 0x111d71b54 IsStructurallyEquivalent(clang::StructuralEquivalenceContext&, clang::RecordDecl*, clang::RecordDecl*) + 2076 12 LLDB 0x111d6e448 clang::StructuralEquivalenceContext::Finish() + 204 13 LLDB 0x111d6e1e0 clang::StructuralEquivalenceContext::IsEquivalent(clang::Decl*, clang::Decl*) + 32 14 LLDB 0x111d3b788 clang::ASTNodeImporter::IsStructuralMatch(clang::Decl*, clang::Decl*, bool, bool) + 168 15 LLDB 0x111d404e0 clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) + 1300 16 LLDB 0x111d5cae0 clang::ASTImporter::ImportImpl(clang::Decl*) + 24 17 LLDB 0x10ddf30bc lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(clang::Decl*) + 308 18 LLDB 0x111d48140 clang::ASTImporter::Import(clang::Decl*) + 984 19 LLDB 0x10ddee9dc lldb_private::ClangASTImporter::CopyDecl(clang::ASTContext*, clang::Decl*) + 100 20 LLDB 0x10ddfab40 lldb_private::ClangASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) + 1692 21 LLDB 0x111e1cb84 clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const + 180 22 LLDB 0x111e1df50 clang::DeclContext::buildLookup() + 204 23 LLDB 0x111e1dcf4 clang::DeclContext::makeDeclVisibleInContextWithFlags(clang::NamedDecl*, bool, bool) + 504 24 LLDB 0x111d3b01c clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) + 724 25 LLDB 0x111d62d10 clang::ASTImporter::ImportDefinition(clang::Decl*) + 428 26 LLDB 0x10ddf1cb0 lldb_private::ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(clang::Decl*, clang::Decl*) + 524 27 LLDB 0x10ddef3c8 (anonymous namespace)::CompleteTagDeclsScope::~CompleteTagDeclsScope() + 616 28 LLDB 0x10ddef6dc lldb_private::ClangASTImporter::DeportDecl(clang::ASTContext*, clang::Decl*) + 436 29 LLDB 0x10ddec3dc lldb_private::ASTResultSynthesizer::CommitPersistentDecls() + 236 30 LLDB 0x10de1091c lldb_private::ClangExpressionParser::ParseInternal(lldb_private::DiagnosticManager&, clang::CodeCompleteConsumer*, unsigned int, unsigned int) + 2292 31 LLDB 0x10de21238 lldb_private::ClangUserExpression::TryParse(lldb_private::DiagnosticManager&, lldb_private::ExecutionContext&, lldb_private::ExecutionPolicy, bool, bool) + 328 ... ``` Here `ASTImporter::IsStructurallyEquivalent` is trying to emit a diagnostic using `ClangExpressionParser`'s `ClangDiagnosticManagerAdapter`. But `TextDiagnosticPrinter::TextDiag` seems to be `nullptr`. This can only happen when we call `HandleDiagnostic` on `ClangDiagnosticManagerAdapter` after we called `EndSourceFile`. Specifically, it looks like when moving a type from `Expression` AST to `Scratch` AST (in `CommitPersistentDecls`), something went wrong, so the ASTImporter tried to emit a diagnostic, but we already called `EndSourceFile` at that point. This patch moves the call to `ResetManager` to before `CommitPersistentDecls`, so if we called `HandleDiagnostic`, we would correctly short-circuit out of it. This seems to have been intended anyway based on this comment: https://github.com/llvm/llvm-project/blob/cecdff92838f3c049548e3445a15d8c9c7a49205/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp#L200-L204 But something must have broken that during a refactor. I'm not 100% sure how best to test this because we need a scenario where moving a type into the scratch AST fails, but the expression itself succeeded.
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesHere's an example crash that we've seen sporadically over the years:
Here This patch moves the call to llvm-project/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp Lines 200 to 204 in cecdff9
But something must have broken that during a refactor. I'm not 100% sure how best to test this because we need a scenario where moving a type into the scratch AST fails, but the expression itself succeeded. rdar://159647906 Full diff: https://github.com/llvm/llvm-project/pull/160074.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 6885977baa24e..924953cc43fa2 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1307,6 +1307,10 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
m_compiler->setSema(nullptr);
adapter->EndSourceFile();
+ // Creating persistent variables can trigger diagnostic emission.
+ // Make sure we reset the manager so we don't get asked to handle
+ // diagnostics after we finished parsing.
+ adapter->ResetManager();
unsigned num_errors = adapter->getNumErrors();
@@ -1322,8 +1326,6 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
type_system_helper->CommitPersistentDecls();
}
- adapter->ResetManager();
-
return num_errors;
}
|
The I assume something got refactored since then that broke it again. Or maybe it was fixing a different codepath leading to this crash |
Same reason we didn't add a test in 4e969da still pretty much applies. Any test that hits this codepath would be quite fragile to future ASTImporter improvements |
…e persistent variables (llvm#160074) Here's an example crash that we've seen sporadically over the years: ``` 0 libsystem_kernel.dylib 0x19d392388 __pthread_kill + 8 1 libsystem_pthread.dylib 0x19d3cb88c pthread_kill + 296 2 libsystem_c.dylib 0x19d29cd04 raise + 32 3 LLDB 0x112cbbc94 SignalHandler(int, __siginfo*, void*) + 324 4 libsystem_platform.dylib 0x19d4056a4 _sigtramp + 56 5 LLDB 0x110dcbd38 clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 1216 6 LLDB 0x110dcbd38 clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 1216 7 LLDB 0x10de12128 ClangDiagnosticManagerAdapter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 332 8 LLDB 0x1121eb3dc clang::DiagnosticIDs::ProcessDiag(clang::DiagnosticsEngine&) const + 200 9 LLDB 0x1121e67a0 clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) + 100 10 LLDB 0x111d766cc IsStructurallyEquivalent(clang::StructuralEquivalenceContext&, clang::FieldDecl*, clang::FieldDecl*, clang::QualType) + 1568 11 LLDB 0x111d71b54 IsStructurallyEquivalent(clang::StructuralEquivalenceContext&, clang::RecordDecl*, clang::RecordDecl*) + 2076 12 LLDB 0x111d6e448 clang::StructuralEquivalenceContext::Finish() + 204 13 LLDB 0x111d6e1e0 clang::StructuralEquivalenceContext::IsEquivalent(clang::Decl*, clang::Decl*) + 32 14 LLDB 0x111d3b788 clang::ASTNodeImporter::IsStructuralMatch(clang::Decl*, clang::Decl*, bool, bool) + 168 15 LLDB 0x111d404e0 clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) + 1300 16 LLDB 0x111d5cae0 clang::ASTImporter::ImportImpl(clang::Decl*) + 24 17 LLDB 0x10ddf30bc lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(clang::Decl*) + 308 18 LLDB 0x111d48140 clang::ASTImporter::Import(clang::Decl*) + 984 19 LLDB 0x10ddee9dc lldb_private::ClangASTImporter::CopyDecl(clang::ASTContext*, clang::Decl*) + 100 20 LLDB 0x10ddfab40 lldb_private::ClangASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) + 1692 21 LLDB 0x111e1cb84 clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const + 180 22 LLDB 0x111e1df50 clang::DeclContext::buildLookup() + 204 23 LLDB 0x111e1dcf4 clang::DeclContext::makeDeclVisibleInContextWithFlags(clang::NamedDecl*, bool, bool) + 504 24 LLDB 0x111d3b01c clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) + 724 25 LLDB 0x111d62d10 clang::ASTImporter::ImportDefinition(clang::Decl*) + 428 26 LLDB 0x10ddf1cb0 lldb_private::ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(clang::Decl*, clang::Decl*) + 524 27 LLDB 0x10ddef3c8 (anonymous namespace)::CompleteTagDeclsScope::~CompleteTagDeclsScope() + 616 28 LLDB 0x10ddef6dc lldb_private::ClangASTImporter::DeportDecl(clang::ASTContext*, clang::Decl*) + 436 29 LLDB 0x10ddec3dc lldb_private::ASTResultSynthesizer::CommitPersistentDecls() + 236 30 LLDB 0x10de1091c lldb_private::ClangExpressionParser::ParseInternal(lldb_private::DiagnosticManager&, clang::CodeCompleteConsumer*, unsigned int, unsigned int) + 2292 31 LLDB 0x10de21238 lldb_private::ClangUserExpression::TryParse(lldb_private::DiagnosticManager&, lldb_private::ExecutionContext&, lldb_private::ExecutionPolicy, bool, bool) + 328 ... ``` Here `ASTImporter::IsStructurallyEquivalent` is trying to emit a diagnostic using `ClangExpressionParser`'s `ClangDiagnosticManagerAdapter`. But `TextDiagnosticPrinter::TextDiag` seems to be `nullptr`. This can only happen when we call `HandleDiagnostic` on `ClangDiagnosticManagerAdapter` after we called `EndSourceFile`. Specifically, it looks like when moving a type from `Expression` AST to `Scratch` AST (in `CommitPersistentDecls`), something went wrong, so the ASTImporter tried to emit a diagnostic, but we already called `EndSourceFile` at that point. This patch moves the call to `ResetManager` to before `CommitPersistentDecls`, so if we called `HandleDiagnostic`, we would correctly short-circuit out of it. This seems to have been intended anyway based on this comment: https://github.com/llvm/llvm-project/blob/cecdff92838f3c049548e3445a15d8c9c7a49205/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp#L200-L204 But something must have broken that during a refactor. I'm not 100% sure how best to test this because we need a scenario where moving a type into the scratch AST fails, but the expression itself succeeded. rdar://159647906 (cherry picked from commit cbfa5c8)
…e persistent variables (llvm#160074) Here's an example crash that we've seen sporadically over the years: ``` 0 libsystem_kernel.dylib 0x19d392388 __pthread_kill + 8 1 libsystem_pthread.dylib 0x19d3cb88c pthread_kill + 296 2 libsystem_c.dylib 0x19d29cd04 raise + 32 3 LLDB 0x112cbbc94 SignalHandler(int, __siginfo*, void*) + 324 4 libsystem_platform.dylib 0x19d4056a4 _sigtramp + 56 5 LLDB 0x110dcbd38 clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 1216 6 LLDB 0x110dcbd38 clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 1216 7 LLDB 0x10de12128 ClangDiagnosticManagerAdapter::HandleDiagnostic(clang::DiagnosticsEngine::Level, clang::Diagnostic const&) + 332 8 LLDB 0x1121eb3dc clang::DiagnosticIDs::ProcessDiag(clang::DiagnosticsEngine&) const + 200 9 LLDB 0x1121e67a0 clang::DiagnosticsEngine::EmitCurrentDiagnostic(bool) + 100 10 LLDB 0x111d766cc IsStructurallyEquivalent(clang::StructuralEquivalenceContext&, clang::FieldDecl*, clang::FieldDecl*, clang::QualType) + 1568 11 LLDB 0x111d71b54 IsStructurallyEquivalent(clang::StructuralEquivalenceContext&, clang::RecordDecl*, clang::RecordDecl*) + 2076 12 LLDB 0x111d6e448 clang::StructuralEquivalenceContext::Finish() + 204 13 LLDB 0x111d6e1e0 clang::StructuralEquivalenceContext::IsEquivalent(clang::Decl*, clang::Decl*) + 32 14 LLDB 0x111d3b788 clang::ASTNodeImporter::IsStructuralMatch(clang::Decl*, clang::Decl*, bool, bool) + 168 15 LLDB 0x111d404e0 clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) + 1300 16 LLDB 0x111d5cae0 clang::ASTImporter::ImportImpl(clang::Decl*) + 24 17 LLDB 0x10ddf30bc lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(clang::Decl*) + 308 18 LLDB 0x111d48140 clang::ASTImporter::Import(clang::Decl*) + 984 19 LLDB 0x10ddee9dc lldb_private::ClangASTImporter::CopyDecl(clang::ASTContext*, clang::Decl*) + 100 20 LLDB 0x10ddfab40 lldb_private::ClangASTSource::FindExternalLexicalDecls(clang::DeclContext const*, llvm::function_ref<bool (clang::Decl::Kind)>, llvm::SmallVectorImpl<clang::Decl*>&) + 1692 21 LLDB 0x111e1cb84 clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const + 180 22 LLDB 0x111e1df50 clang::DeclContext::buildLookup() + 204 23 LLDB 0x111e1dcf4 clang::DeclContext::makeDeclVisibleInContextWithFlags(clang::NamedDecl*, bool, bool) + 504 24 LLDB 0x111d3b01c clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) + 724 25 LLDB 0x111d62d10 clang::ASTImporter::ImportDefinition(clang::Decl*) + 428 26 LLDB 0x10ddf1cb0 lldb_private::ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(clang::Decl*, clang::Decl*) + 524 27 LLDB 0x10ddef3c8 (anonymous namespace)::CompleteTagDeclsScope::~CompleteTagDeclsScope() + 616 28 LLDB 0x10ddef6dc lldb_private::ClangASTImporter::DeportDecl(clang::ASTContext*, clang::Decl*) + 436 29 LLDB 0x10ddec3dc lldb_private::ASTResultSynthesizer::CommitPersistentDecls() + 236 30 LLDB 0x10de1091c lldb_private::ClangExpressionParser::ParseInternal(lldb_private::DiagnosticManager&, clang::CodeCompleteConsumer*, unsigned int, unsigned int) + 2292 31 LLDB 0x10de21238 lldb_private::ClangUserExpression::TryParse(lldb_private::DiagnosticManager&, lldb_private::ExecutionContext&, lldb_private::ExecutionPolicy, bool, bool) + 328 ... ``` Here `ASTImporter::IsStructurallyEquivalent` is trying to emit a diagnostic using `ClangExpressionParser`'s `ClangDiagnosticManagerAdapter`. But `TextDiagnosticPrinter::TextDiag` seems to be `nullptr`. This can only happen when we call `HandleDiagnostic` on `ClangDiagnosticManagerAdapter` after we called `EndSourceFile`. Specifically, it looks like when moving a type from `Expression` AST to `Scratch` AST (in `CommitPersistentDecls`), something went wrong, so the ASTImporter tried to emit a diagnostic, but we already called `EndSourceFile` at that point. This patch moves the call to `ResetManager` to before `CommitPersistentDecls`, so if we called `HandleDiagnostic`, we would correctly short-circuit out of it. This seems to have been intended anyway based on this comment: https://github.com/llvm/llvm-project/blob/cecdff92838f3c049548e3445a15d8c9c7a49205/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp#L200-L204 But something must have broken that during a refactor. I'm not 100% sure how best to test this because we need a scenario where moving a type into the scratch AST fails, but the expression itself succeeded. rdar://159647906 (cherry picked from commit cbfa5c8)
Here's an example crash that we've seen sporadically over the years:
Here
ASTImporter::IsStructurallyEquivalent
is trying to emit a diagnostic usingClangExpressionParser
'sClangDiagnosticManagerAdapter
. ButTextDiagnosticPrinter::TextDiag
seems to benullptr
. This can only happen when we callHandleDiagnostic
onClangDiagnosticManagerAdapter
after we calledEndSourceFile
. Specifically, it looks like when moving a type fromExpression
AST toScratch
AST (inCommitPersistentDecls
), something went wrong, so the ASTImporter tried to emit a diagnostic, but we already calledEndSourceFile
at that point.This patch moves the call to
ResetManager
to beforeCommitPersistentDecls
, so if we calledHandleDiagnostic
, we would correctly short-circuit out of it. This seems to have been intended anyway based on this comment:llvm-project/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
Lines 200 to 204 in cecdff9
But something must have broken that during a refactor.
I'm not 100% sure how best to test this because we need a scenario where moving a type into the scratch AST fails, but the expression itself succeeded.
rdar://159647906