Separate clangimporter errors #6864

adrian-prantl · 2023-05-16T01:18:04Z

SwiftASTContext resets the state of the DiagnosticConsumer after each
import to make failed module imports non-fatal, however, the internal
state of the DiagnosticEngine in Swift's embedded Clang compiler does
not reset when encountering a fatal error. By storing Clang
diagnostics separately, these errors can be surfaces when an
expression fails to IRGen (which is usually when these fatal Clang
diagnostics turn into an unrecoverable problem).

rdar://108557254

adrian-prantl · 2023-05-16T21:04:07Z

@swift-ci test

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

lldb/source/Plugins/ExpressionParser/Swift/SwiftUserExpression.cpp

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.h

lldb/source/Target/Target.cpp

adrian-prantl · 2023-05-17T20:20:55Z

@swift-ci test

adrian-prantl · 2023-05-17T22:39:50Z

@swift-ci test

adrian-prantl · 2023-05-19T22:16:25Z

@swift-ci test

adrian-prantl · 2023-05-19T22:17:06Z

Introduce a "transactional" diagnostic consumer.

Swift compiler and ClangImporter and LLDB operations can produce
diagnostics, which are stored in StoringDiagnosticConsumer. Operations
such as expression evaluation of module imports may produce multiple
non-fatal error diagnostics. Prior to this patch, the expression
parser would clear the diagnostic consumer before running. This had
the unwanted side-effect of sometimes clearing out unreported earlier
diagnostics.

This patch introduces SwiftASTContext::forkDiagnosticConsumer() which
returns an RAII object that behaves like a diagnostic consumer, can
tell whether diagnostics where produced during its lifetime, and
resets the StoringDiagnosticConsumer to its previous state upon
destruction. This way

unreported diagnostics don't get cleared by the expression parser
the expression parser has precise access to only the diagnostics it produced
while still being able to report persistent errors
(such as clang import failures) that happened before the expression
parser started.

I'm not happy by the amount of code needed to make this work, or the
specific implementation, but at least the interface provided to the
outside is nice and clean.

adrian-prantl · 2023-05-19T23:29:43Z

@swift-ci test

adrian-prantl · 2023-05-19T23:30:02Z

@swift-ci test windows

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp

adrian-prantl · 2023-05-24T21:08:28Z

@swift-ci test

bulbazord

Haven't looked at all of SwiftASTContext yet but the other stuff looks ok I think.

bulbazord · 2023-05-25T00:15:19Z

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

+  if (expr_diagnostics->HasErrors() ||
+      m_swift_ast_ctx.HasClangImporterErrors()) {
+    diagnostic_manager.PutString(eDiagnosticSeverityRemark,
+                                 "couldn't IRGen expression");


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

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.

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

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

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.h

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp

adrian-prantl · 2023-05-25T18:19:20Z

@swift-ci test

adrian-prantl · 2023-05-25T18:35:31Z

@swift-ci test

adrian-prantl · 2023-05-25T18:37:25Z

@swift-ci test windows

JDevlieghere

I didn't re-review everything but I see my concerns were addressed to LGTM.

SwiftASTContext resets the state of the DiagnosticConsumer after each import to make failed module imports non-fatal, however, the internal state of the DiagnosticEngine in Swift's embedded Clang compiler does not reset when encountering a fatal error. By storing Clang diagnostics separately, these errors can be surfaces when an expression fails to IRGen (which is usually when these fatal Clang diagnostics turn into an unrecoverable problem). rdar://108557254

… (NFC)

… logging them.

Swift compiler and ClangImporter and LLDB operations can produce diagnostics, which are stored in StoringDiagnosticConsumer. Operations such as expression evaluation of module imports may produce multiple non-fatal error diagnostics. Prior to this patch, the expression parser would clear the diagnostic consumer before running. This had the unwanted side-effect of sometimes clearing out unreported earlier diagnostics. This patch introduces SwiftASTContext::forkDiagnosticConsumer() which returns an RAII object that behaves like a diagnostic consumer, can tell whether diagnostics where produced during its lifetime, and resets the StoringDiagnosticConsumer to its previous state upon destruction. This way - unreported diagnostics don't get cleared by the expression parser - the expression parser has precise access to only the diagnostics it produced - while still being able to report persistent errors (such as clang import failures) that happened before the expression parser started. I'm not happy by the amount of code needed to make this work, or the specific implementation, but at least the interface provided to the outside is nice and clean.

adrian-prantl · 2023-05-31T21:55:13Z

@swift-ci test

adrian-prantl · 2023-05-31T21:56:37Z

@swift-ci test windows

adrian-prantl force-pushed the separate-clangimporter-errors branch from 6a192b7 to f5dfc31 Compare May 16, 2023 21:03

adrian-prantl requested review from kastiglione, JDevlieghere and augusto2112 May 16, 2023 21:03

JDevlieghere reviewed May 17, 2023

View reviewed changes

adrian-prantl force-pushed the separate-clangimporter-errors branch from f5dfc31 to f619b5f Compare May 17, 2023 20:17

adrian-prantl requested a review from bulbazord May 19, 2023 22:17

kastiglione reviewed May 22, 2023

View reviewed changes

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp Outdated Show resolved Hide resolved

adrian-prantl force-pushed the separate-clangimporter-errors branch from d11204f to a300454 Compare May 24, 2023 21:08

bulbazord reviewed May 25, 2023

View reviewed changes

JDevlieghere reviewed May 25, 2023

View reviewed changes

adrian-prantl force-pushed the separate-clangimporter-errors branch 2 times, most recently from 3c23fdd to f79bcc7 Compare May 25, 2023 17:59

JDevlieghere approved these changes May 30, 2023

View reviewed changes

adrian-prantl added 5 commits May 31, 2023 14:53

Simplify initializers (NFC)

a8c6297

Simplify StoringDiagnosticConsumer::handleDiagnostic() (NFC)

684b12c

Unify all mechanisms for delaying diagnostics in SwiftASTContext (NFC)

9c48cac

Replace all mutation of m_fatal_error with calls to RaiseFatalError()…

d871215

… (NFC)

adrian-prantl added 6 commits May 31, 2023 14:54

Unconditionally initialize StoringDiagnosticConsumer

cddc4e0

Report errors in PerformCompileUnitImports to the user in addition to…

2a63c52

… logging them.

Remove redundant initialization

71c884b

Surface LLVM verification errors.

c668d58

Convert GetAllErrors() to return an llvm::Error (NFC)

a2052d2

adrian-prantl force-pushed the separate-clangimporter-errors branch from c7a7056 to a2052d2 Compare May 31, 2023 21:55

adrian-prantl merged commit ce97507 into swiftlang:swift/release/5.9 May 31, 2023

Separate clangimporter errors #6864

Separate clangimporter errors #6864

Uh oh!

Conversation

adrian-prantl commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrian-prantl commented May 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrian-prantl commented May 17, 2023

Uh oh!

adrian-prantl commented May 17, 2023

Uh oh!

adrian-prantl commented May 19, 2023

Uh oh!

adrian-prantl commented May 19, 2023

Uh oh!

adrian-prantl commented May 19, 2023

Uh oh!

adrian-prantl commented May 19, 2023

Uh oh!

Uh oh!

adrian-prantl commented May 24, 2023

Uh oh!

bulbazord left a comment

Choose a reason for hiding this comment

Uh oh!

bulbazord May 25, 2023

Choose a reason for hiding this comment

Uh oh!

adrian-prantl May 25, 2023

Choose a reason for hiding this comment

Uh oh!

adrian-prantl May 25, 2023

Choose a reason for hiding this comment

Uh oh!

adrian-prantl May 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adrian-prantl commented May 25, 2023

Uh oh!

adrian-prantl commented May 25, 2023

Uh oh!

adrian-prantl commented May 25, 2023

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented May 31, 2023

Uh oh!

adrian-prantl commented May 31, 2023

Uh oh!

Uh oh!

adrian-prantl commented May 16, 2023 •

edited

Loading