Skip to content

Separate clangimporter errors #6864

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
May 31, 2023
Merged

Separate clangimporter errors #6864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a8c6297
Simplify initializers (NFC)
adrian-prantl May 15, 2023
684b12c
Simplify StoringDiagnosticConsumer::handleDiagnostic() (NFC)
adrian-prantl May 15, 2023
7e362b4
Separate Swift and Clang errors in StoringDiagnosticConsumer
adrian-prantl May 31, 2023
9c48cac
Unify all mechanisms for delaying diagnostics in SwiftASTContext (NFC)
adrian-prantl May 16, 2023
d871215
Replace all mutation of m_fatal_error with calls to RaiseFatalError()…
adrian-prantl May 17, 2023
cddc4e0
Unconditionally initialize StoringDiagnosticConsumer
adrian-prantl May 17, 2023
2a63c52
Report errors in PerformCompileUnitImports to the user in addition to…
adrian-prantl May 18, 2023
71c884b
Remove redundant initialization
adrian-prantl May 25, 2023
98c0f3f
Introduce a "transactional" diagnostic consumer.
adrian-prantl May 19, 2023
c668d58
Surface LLVM verification errors.
adrian-prantl May 25, 2023
a2052d2
Convert GetAllErrors() to return an llvm::Error (NFC)
adrian-prantl May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 45 additions & 40 deletions lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,6 @@ SetupASTContext(SwiftASTContextForExpressions &swift_ast_context,
// TODO: Find a way to get contraint-solver output sent to a stream
// so we can log it.
// swift_ast_context.GetLanguageOptions().DebugConstraintSolver = true;
swift_ast_context.ClearDiagnostics();

// No longer part of debugger support, set it separately.
swift_ast_context.GetLanguageOptions().EnableDollarIdentifiers = true;
Expand Down Expand Up @@ -1217,15 +1216,15 @@ AddArchetypesTypeAliases(std::unique_ptr<SwiftASTManipulator> &code_manipulator,
/// Attempt to parse an expression and import all the Swift modules
/// the expression and its context depend on.
static llvm::Expected<ParsedExpression> ParseAndImport(
SwiftASTContextForExpressions &swift_ast_context, Expression &expr,
SwiftASTContextForExpressions &swift_ast_context,
SwiftASTContext::ScopedDiagnostics &expr_diagnostics, Expression &expr,
SwiftExpressionParser::SILVariableMap &variable_map, unsigned &buffer_id,
DiagnosticManager &diagnostic_manager,
SwiftExpressionParser &swift_expr_parser,
lldb::StackFrameWP &stack_frame_wp, SymbolContext &sc,
ExecutionContextScope &exe_scope,
ExecutionContextScope &exe_scope,
llvm::SmallVectorImpl<SwiftASTManipulator::VariableInfo> &local_variables,
const EvaluateExpressionOptions &options,
bool repl, bool playground) {
const EvaluateExpressionOptions &options, bool repl, bool playground) {
Log *log = GetLog(LLDBLog::Expressions);
LLDB_SCOPED_TIMER();

Expand Down Expand Up @@ -1373,17 +1372,16 @@ static llvm::Expected<ParsedExpression> ParseAndImport(
// inserting them in.
swift_ast_context.AddDebuggerClient(external_lookup);

if (swift_ast_context.HasErrors())
if (expr_diagnostics.HasErrors())
return make_error<SwiftASTContextError>();

// Resolve the file's imports, including the implicit ones returned from
// GetImplicitImports.
swift::performImportResolution(*source_file);

if (swift_ast_context.HasErrors())
if (expr_diagnostics.HasErrors())
return make_error<ModuleImportError>(
swift_ast_context.GetAllErrors().AsCString(
"Explicit module import error"));
llvm::toString(expr_diagnostics.GetAllErrors()));

std::unique_ptr<SwiftASTManipulator> code_manipulator;
if (repl || !playground) {
Expand Down Expand Up @@ -1654,18 +1652,24 @@ RedirectCallFromSinkToTrampolineFunction(llvm::Module &module,
SwiftExpressionParser::ParseResult
SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
uint32_t first_line, uint32_t last_line) {
SwiftExpressionParser::SILVariableMap variable_map;
using ParseResult = SwiftExpressionParser::ParseResult;
Log *log = GetLog(LLDBLog::Expressions);
LLDB_SCOPED_TIMER();

SwiftExpressionParser::SILVariableMap variable_map;
// Get a scoped diagnostics consumer for all diagnostics produced by
// this expression.
auto expr_diagnostics = m_swift_ast_ctx.getScopedDiagnosticConsumer();
m_swift_ast_ctx.GetDiagnosticEngine().resetHadAnyError();

// Helper function to diagnose errors in m_swift_scratch_context.
unsigned buffer_id = UINT32_MAX;
auto DiagnoseSwiftASTContextError = [&]() {
assert(m_swift_ast_ctx.HasErrors() && "error expected");
m_swift_ast_ctx.PrintDiagnostics(diagnostic_manager, buffer_id, first_line,
last_line);
assert((expr_diagnostics->HasErrors() ||
m_swift_ast_ctx.HasClangImporterErrors()) &&
"error expected");
expr_diagnostics->PrintDiagnostics(diagnostic_manager, buffer_id,
first_line, last_line);
};

// In the case of playgrounds, we turn all rewriting functionality off.
Expand All @@ -1677,9 +1681,9 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,

// Parse the expression and import all nececssary swift modules.
auto parsed_expr = ParseAndImport(
m_swift_ast_ctx, m_expr, variable_map, buffer_id, diagnostic_manager,
*this, m_stack_frame_wp, m_sc, *m_exe_scope, m_local_variables, m_options,
repl, playground);
m_swift_ast_ctx, *expr_diagnostics, m_expr, variable_map, buffer_id,
diagnostic_manager, *this, m_stack_frame_wp, m_sc, *m_exe_scope,
m_local_variables, m_options, repl, playground);

if (!parsed_expr) {
bool retry = false;
Expand Down Expand Up @@ -1738,7 +1742,7 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
if (log)
dumpModule("Module after type checking:");

if (m_swift_ast_ctx.HasErrors()) {
if (expr_diagnostics->HasErrors()) {
DiagnoseSwiftASTContextError();
return ParseResult::unrecoverable_error;
}
Expand Down Expand Up @@ -1915,7 +1919,7 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
log->PutCString(s.c_str());
}

if (m_swift_ast_ctx.HasErrors()) {
if (expr_diagnostics->HasErrors()) {
DiagnoseSwiftASTContextError();
return ParseResult::unrecoverable_error;
}
Expand Down Expand Up @@ -1943,7 +1947,7 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
log->PutCString(s.c_str());
}

if (m_swift_ast_ctx.HasErrors()) {
if (expr_diagnostics->HasErrors()) {
DiagnoseSwiftASTContextError();
return ParseResult::unrecoverable_error;
}
Expand All @@ -1969,30 +1973,22 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
m_module.reset(ContextAndModule.second);
}

if (m_swift_ast_ctx.HasErrors()) {
// If IRGen failed without errors, the root cause may be a fatal
// Clang diagnostic.
if (expr_diagnostics->HasErrors() ||
m_swift_ast_ctx.HasClangImporterErrors()) {
diagnostic_manager.PutString(eDiagnosticSeverityRemark,
"couldn't IRGen expression");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these messages meant to be seen by end users? If so, IRGen as a term is pretty specific to clang, swift, and llvm... maybe we can say something a little more user friendly like:

Failed to compile expression: IR generation failure

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a valid concern! Note that this is just one remark in a longer error. For example, the test in lang/swift/clangimporter/clang_errorhandling produces:

error: expression failed to parse:
couldn't IRGen expression
error: /Volumes/Data/swift-5.9/llvm-project/lldb/test/API/lang/swift/clangimporter/clang_errorhandling/bridging-header.h:2:1: error: unknown type name 'i'
i am a syntax error
^

error: /Volumes/Data/swift-5.9/llvm-project/lldb/test/API/lang/swift/clangimporter/clang_errorhandling/bridging-header.h:2:5: error: expected ';' after top level declarator
i am a syntax error
    ^

error: failed to import bridging header '/Volumes/Data/swift-5.9/llvm-project/lldb/test/API/lang/swift/clangimporter/clang_errorhandling/bridging-header.h'

as the full error message.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IRGen remark is really mostly there for LLDB developers to understand at what point the expression failed to help with troubleshooting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this in mind — any suggestions for how to improve the error message?

DiagnoseSwiftASTContextError();
return ParseResult::unrecoverable_error;
}

if (!m_module) {
auto &warnings = m_swift_ast_ctx.GetModuleImportWarnings();
for (StringRef message : warnings) {
// FIXME: Don't store diagnostics as strings.
auto severity = eDiagnosticSeverityWarning;
if (message.consume_front("warning: "))
severity = eDiagnosticSeverityWarning;
if (message.consume_front("error: "))
severity = eDiagnosticSeverityError;
diagnostic_manager.PutString(severity, message);
}
std::string error = "couldn't IRGen expression";
diagnostic_manager.Printf(
eDiagnosticSeverityError, "couldn't IRGen expression. %s",
warnings.empty()
? "Please enable the expression log by running \"log enable lldb "
"expr\", then run the failing expression again, and file a "
"bugreport with the log output."
: "Please check the above error messages for possible root causes.");
eDiagnosticSeverityError,
"couldn't IRGen expression. Please enable the expression log by "
"running \"log enable lldb expr\", then run the failing expression "
"again, and file a bug report with the log output.");
return ParseResult::unrecoverable_error;
}

Expand Down Expand Up @@ -2032,12 +2028,21 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
std::lock_guard<std::recursive_mutex> global_context_locker(
IRExecutionUnit::GetLLVMGlobalContextMutex());

LLVMVerifyModule((LLVMOpaqueModule *)m_module.get(), LLVMReturnStatusAction,
nullptr);
bool has_errors = LLVMVerifyModule((LLVMOpaqueModule *)m_module.get(),
LLVMReturnStatusAction, nullptr);
if (has_errors) {
diagnostic_manager.PutString(eDiagnosticSeverityRemark,
"LLVM verification error");
return ParseResult::unrecoverable_error;
}
}

if (m_swift_ast_ctx.HasErrors())
if (expr_diagnostics->HasErrors()) {
diagnostic_manager.PutString(eDiagnosticSeverityRemark,
"post-IRGen error");
DiagnoseSwiftASTContextError();
return ParseResult::unrecoverable_error;
}

// The Parse succeeded! Now put this module into the context's list
// of loaded modules, and copy the Decls that were globalized as
Expand Down
Loading