From: Raphael Isemann Date: Wed, 9 Oct 2019 08:30:06 +0000 (+0000) Subject: [lldb] Don't crash when the ASTImporter produces diagnostics but instead log them. X-Git-Tag: llvmorg-11-init~7123 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=4e969da33e113dffc1dfcf5c5e3ef97e3b594161;p=platform%2Fupstream%2Fllvm.git [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). llvm-svn: 374145 --- diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index defcefd..db3f139 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -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 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; + 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); - } + 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); } } }