Hi @Adrian_Prantl, @dcci and @Slava_Pestov. I took a look at this bug, and found the culprit.
So the reason LLDB is crashing is because of a null pointer. At SwiftUserExpression::ScanContext
, we have a dyn_cast
of null:
if (auto *ref_type = llvm::dyn_cast<swift::ReferenceStorageType>(
GetSwiftType(self_type).getPointer())) { // GetSwiftType(self_type).getPointer() returns null
So by simply changing it to dyn_cast_or_null
we stop crashing, but now we print this in the console:
error: <EXPR>:1:11: error: use of undeclared type '$__lldb_context'
extension $__lldb_context {
^~~~~~~~~~~~~~~
error: <EXPR>:19:5: error: use of unresolved identifier '$__lldb_injected_self'
$__lldb_injected_self.$__lldb_wrapped_expr_0(
^~~~~~~~~~~~~~~~~~~~~
Which is probably not ideal either.
So what is actually happening inside GetSwiftType
is that SwiftASTContext::ReconstructType
doesn't find the type, and return a swift::Type
whose pointer is null.
While the crash is fixed, I think I should try to print a better error message, but I don't know the exact way to go about it.
The first thing I noticed is that the method SwiftASTContext::ReconstructType
receives a Status
variable, but if a type is found in the negative cache, we don't set the Status
to anything, while the first time a type is not found the error is set. Is that intentional?
if (m_negative_type_cache.Lookup(mangled_cstr)) {
LOG_PRINTF(LIBLLDB_LOG_TYPES, "(\"%s\") -- found in the negative cache",
mangled_cstr);
return {}; // Status variable not set to some error when found inside negative cache
}
I thought it would be a good idea to set the status here as well, but if I do that I get some new output that wasn't happening before:
(lldb) run NIOTests.ChannelTests/testHalfClosure
Process 9175 launched: '/home/augusto/Developer/swift-nio/.build/debug/swift-nioPackageTests.xctest' (x86_64)
Test Suite 'Selected tests' started at 2020-03-27 18:05:51.987
Test Suite 'ChannelTests' started at 2020-03-27 18:05:51.988
Test Case 'ChannelTests.testHalfClosure' started at 2020-03-27 18:05:51.988
/home/augusto/Developer/swift-nio/.build/debug/swift-nioPackageTests.xctest: Cannot load Swift type information; AST validation error in "NIO": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOConcurrencyHelpers": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOConcurrencyHelpersTests": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOFoundationCompat": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOHTTP1": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOHTTP1Tests": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOTLS": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOTLSTests": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOTestUtils": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOTestUtilsTests": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOTests": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOWebSocket": The module file format is too old to be used by this version of the debugger.AST validation error in "NIOWebSocketTests": The module file format is too old to be used by this version of the debugger.AST validation error in "swift_nioPackageTests": The module file format is too old to be used by this version of the debugger.
So I'm not sure if this is desirable or not.
If you guys think it's desirable, my second question is how to actually go about reporting this to the user. Function TypeSystemSwiftTypeRef::GetSwiftType
(SwiftASTContext
) passes a Status
reference to ReconstructType
, but doesn't check it at all.
...
Status error;
// FIXME: Suboptimal performance, because the ConstString is looked up again.
ConstString mangled_name(
reinterpret_cast<const char *>(compiler_type.GetOpaqueQualType()));
return ts->m_swift_ast_context->ReconstructType(mangled_name, error); // error ignored
ScanContext
also receives a reference to a Status
, so I tested passing it down from ScanContext
all the way to ReconstructType
, and that kinda works:
error: warning: type for typename "$s3NIO19SelectableEventLoopCD" was not found
error: <EXPR>:1:11: error: use of undeclared type '$__lldb_context'
extension $__lldb_context {
^~~~~~~~~~~~~~~
error: <EXPR>:19:5: error: use of unresolved identifier '$__lldb_injected_self'
$__lldb_injected_self.$__lldb_wrapped_expr_0(
^~~~~~~~~~~~~~~~~~~~~
Maybe that's not correct though because we still get those $__lldb
errors. I noticed that StoringDiagnosticConsumer::PrintDiagnostics
only includes those messages if a better error message was not found:
// In general, we don't want to see diagnostics from outside of
// the source text range of the actual user expression. But if we
// didn't find any diagnostics in the text range, it's probably
// because the source range was not specified correctly, and we
// don't want to lose legit errors because of that. So in that
// case we'll add them all here:
if (!added_one_diagnostic) {
// This will report diagnostic errors from outside the
// expression's source range. Those are not interesting to
// users, so we only emit them in debug builds.
for (const RawDiagnostic &diagnostic : m_diagnostics) {
// adds diagnostics here
}
}
So my second idea was to check the error inside TypeSystemSwiftTypeRef::GetSwiftType
, and somehow convert it to a RawDiagnostic
and store it so it's dealt in PrintDiagnostics
later.
...
Status error;
// FIXME: Suboptimal performance, because the ConstString is looked up again.
ConstString mangled_name(
reinterpret_cast<const char *>(compiler_type.GetOpaqueQualType()));
auto type = ts->m_swift_ast_context->ReconstructType(mangled_name, error);
if (!error.Success()) {
m_diagnostics.push_back(RawDiagnostic(error)); // This constructor doesn't exist, just an example
}
return type;
But thinking a little bit more, this might not be a good idea, since GetSwiftType
can potentially be called multiple times, so we'd add duplicate error messages if we did this the way I was thinking.
So this is where I'm at right now, and I'm not sure in what way I should proceed to give the user a better error message. What do you guys think?