[Lldb-commits] [lldb] r374145 - [lldb] Don't crash when the ASTImporter produces diagnostics but instead log them.
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Wed Oct 9 01:30:06 PDT 2019
Author: teemperor
Date: Wed Oct 9 01:30:06 2019
New Revision: 374145
URL: http://llvm.org/viewvc/llvm-project?rev=374145&view=rev
Log:
[lldb] Don't crash when the ASTImporter produces diagnostics but instead log them.
When playing with the C++ module prototype I noticed I can get LLDB to crash
by making a result type that depends on __make_integer_seq (a BuiltinTemplate)
which is not supported by the ASTImporter yet. This causes the ASTImporter to emit
a diagnostic when copying the type to the ScratchASTContext. As deporting the result
type is done after we are done parsing and the Clang's diagnostic engine asserts that
it can only be used during parsing, it crashes LLDB while trying to render the diagnostic
in the HandleDiagnostic method of ClangDiagnosticManagerAdapter.
This patch just moves the HandleDiagnostic call to Clang behind our check that we still
have a DiagnosticManager (which we remove after parsing) which prevents the assert
from firing. We also shouldn't ignore such diagnostics, so I added a log statement for
them.
There doesn't seem to way to test this as these diagnostic only happen when we copy
a node that's not supported by the ASTImporter which should never happen once
we can copy everything with the ASTImporter, so every test case we add here will
eventually become invalid.
(Note that most of this diff is just whitespace changes as we now use an early exit
instead of a huge 'if' block).
Modified:
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp?rev=374145&r1=374144&r2=374145&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (original)
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp Wed Oct 9 01:30:06 2019
@@ -162,51 +162,66 @@ public:
void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) override {
+ if (!m_manager) {
+ // We have no DiagnosticManager before/after parsing but we still could
+ // receive diagnostics (e.g., by the ASTImporter failing to copy decls
+ // when we move the expression result ot the ScratchASTContext). Let's at
+ // least log these diagnostics until we find a way to properly render
+ // them and display them to the user.
+ Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+ if (log) {
+ llvm::SmallVector<char, 32> diag_str;
+ Info.FormatDiagnostic(diag_str);
+ diag_str.push_back('\0');
+ const char *plain_diag = diag_str.data();
+ LLDB_LOG(log, "Received diagnostic outside parsing: {0}", plain_diag);
+ }
+ return;
+ }
+
// Render diagnostic message to m_output.
m_output.clear();
m_passthrough->HandleDiagnostic(DiagLevel, Info);
m_os->flush();
- if (m_manager) {
- lldb_private::DiagnosticSeverity severity;
- bool make_new_diagnostic = true;
-
- switch (DiagLevel) {
- case DiagnosticsEngine::Level::Fatal:
- case DiagnosticsEngine::Level::Error:
- severity = eDiagnosticSeverityError;
- break;
- case DiagnosticsEngine::Level::Warning:
- severity = eDiagnosticSeverityWarning;
- break;
- case DiagnosticsEngine::Level::Remark:
- case DiagnosticsEngine::Level::Ignored:
- severity = eDiagnosticSeverityRemark;
- break;
- case DiagnosticsEngine::Level::Note:
- m_manager->AppendMessageToDiagnostic(m_output);
- make_new_diagnostic = false;
- }
- if (make_new_diagnostic) {
- // ClangDiagnostic messages are expected to have no whitespace/newlines
- // around them.
- std::string stripped_output = llvm::StringRef(m_output).trim();
-
- ClangDiagnostic *new_diagnostic =
- new ClangDiagnostic(stripped_output, severity, Info.getID());
- m_manager->AddDiagnostic(new_diagnostic);
-
- // Don't store away warning fixits, since the compiler doesn't have
- // enough context in an expression for the warning to be useful.
- // FIXME: Should we try to filter out FixIts that apply to our generated
- // code, and not the user's expression?
- if (severity == eDiagnosticSeverityError) {
- size_t num_fixit_hints = Info.getNumFixItHints();
- for (size_t i = 0; i < num_fixit_hints; i++) {
- const clang::FixItHint &fixit = Info.getFixItHint(i);
- if (!fixit.isNull())
- new_diagnostic->AddFixitHint(fixit);
- }
+ lldb_private::DiagnosticSeverity severity;
+ bool make_new_diagnostic = true;
+
+ switch (DiagLevel) {
+ case DiagnosticsEngine::Level::Fatal:
+ case DiagnosticsEngine::Level::Error:
+ severity = eDiagnosticSeverityError;
+ break;
+ case DiagnosticsEngine::Level::Warning:
+ severity = eDiagnosticSeverityWarning;
+ break;
+ case DiagnosticsEngine::Level::Remark:
+ case DiagnosticsEngine::Level::Ignored:
+ severity = eDiagnosticSeverityRemark;
+ break;
+ case DiagnosticsEngine::Level::Note:
+ m_manager->AppendMessageToDiagnostic(m_output);
+ make_new_diagnostic = false;
+ }
+ if (make_new_diagnostic) {
+ // ClangDiagnostic messages are expected to have no whitespace/newlines
+ // around them.
+ std::string stripped_output = llvm::StringRef(m_output).trim();
+
+ ClangDiagnostic *new_diagnostic =
+ new ClangDiagnostic(stripped_output, severity, Info.getID());
+ m_manager->AddDiagnostic(new_diagnostic);
+
+ // Don't store away warning fixits, since the compiler doesn't have
+ // enough context in an expression for the warning to be useful.
+ // FIXME: Should we try to filter out FixIts that apply to our generated
+ // code, and not the user's expression?
+ if (severity == eDiagnosticSeverityError) {
+ size_t num_fixit_hints = Info.getNumFixItHints();
+ for (size_t i = 0; i < num_fixit_hints; i++) {
+ const clang::FixItHint &fixit = Info.getFixItHint(i);
+ if (!fixit.isNull())
+ new_diagnostic->AddFixitHint(fixit);
}
}
}
More information about the lldb-commits
mailing list