From 7e8ae7b5c66b72d9befeccf190c3f97cbf607ce0 Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Fri, 23 Oct 2020 20:16:10 +0200 Subject: [PATCH] [lldb][swift] Use dynamic types as a fallback in AddVariableInfo Since commit eaceb46661c6833f11fa98659223652f6d549f4a AddVariableInfo is always using the dynamic type if requested. The dynamic types appear to not always be usable and caused a few tests to fail(1), but as the swiftTest decorator was always skipping all Swift tests since commit 2c911bceb06ed376801251bdfd992905a66f276c this wasn't discovered before landing. This patch restores the old behaviour where possible by first trying the non-dynamic type and falling back to the dynamic type in case the non-dynamic type can't be fully realized. (1) All the failures seem to be related to us reading corrupt data from memory when using the dynamic type in the expression parser. The dynamic types itself appear to be perfectly fine, so it's not clear why the new behaviour doesn't work. --- .../Swift/SwiftExpressionParser.cpp | 55 ++++++++++++------- .../TestGenericExistentials.py | 4 +- .../TestOpaqueExistentials.py | 4 +- .../unknown_self/TestSwiftUnknownSelf.py | 1 - .../error_type/TestSwiftErrorType.py | 5 +- 5 files changed, 38 insertions(+), 31 deletions(-) diff --git a/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp index 96ed9d57c3a92..e9c48ba8fe87d 100644 --- a/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp @@ -576,6 +576,22 @@ static void AddRequiredAliases(Block *block, lldb::StackFrameSP &stack_frame_sp, } } +/// Returns the Swift type for a ValueObject representing a variable. +/// An invalid CompilerType is returned on error. +static CompilerType GetSwiftTypeForVariableValueObject( + lldb::ValueObjectSP valobj_sp, lldb::StackFrameSP &stack_frame_sp, + SwiftLanguageRuntime *runtime, bool use_dynamic_value) { + // Check that the passed ValueObject is valid. + if (!valobj_sp || valobj_sp->GetError().Fail()) + return CompilerType(); + CompilerType result = valobj_sp->GetCompilerType(); + if (use_dynamic_value) + result = runtime->BindGenericTypeParameters(*stack_frame_sp, result); + if (!result.GetTypeSystem()->SupportsLanguage(lldb::eLanguageTypeSwift)) + return CompilerType(); + return result; +} + /// Create a \c VariableInfo record for \c variable if there isn't /// already shadowing inner declaration in \c processed_variables. static llvm::Optional AddVariableInfo( @@ -602,31 +618,30 @@ static llvm::Optional AddVariableInfo( if (processed_variables.count(overridden_name)) return {}; - CompilerType var_type; - if (stack_frame_sp) { - lldb::ValueObjectSP valobj_sp = - stack_frame_sp->GetValueObjectForFrameVariable(variable_sp, - use_dynamic); + if (!stack_frame_sp) + return llvm::None; - if (!valobj_sp || valobj_sp->GetError().Fail()) { - // Ignore the variable if we couldn't find its corresponding - // value object. TODO if the expression tries to use an - // ignored variable, produce a sensible error. - return {}; - } - var_type = valobj_sp->GetCompilerType(); - if (use_dynamic > lldb::eNoDynamicValues) { - if (auto *stack_frame = stack_frame_sp.get()) - var_type = - runtime->BindGenericTypeParameters(*stack_frame, var_type); - } - } + lldb::ValueObjectSP valobj_sp = + stack_frame_sp->GetValueObjectForFrameVariable(variable_sp, + lldb::eNoDynamicValues); + + const bool use_dynamic_value = use_dynamic > lldb::eNoDynamicValues; + + CompilerType var_type = GetSwiftTypeForVariableValueObject( + valobj_sp, stack_frame_sp, runtime, use_dynamic_value); if (!var_type.IsValid()) return {}; - if (!var_type.GetTypeSystem()->SupportsLanguage(lldb::eLanguageTypeSwift)) - return {}; + // If the type can't be realized and dynamic types are allowed, fall back to + // the dynamic type. + if (!SwiftASTContext::IsFullyRealized(var_type) && use_dynamic_value) { + var_type = GetSwiftTypeForVariableValueObject( + valobj_sp->GetDynamicValue(use_dynamic), stack_frame_sp, runtime, + use_dynamic_value); + if (!var_type.IsValid()) + return {}; + } Status error; CompilerType target_type = ast_context.ImportType(var_type, error); diff --git a/lldb/test/API/lang/swift/generic_existentials/TestGenericExistentials.py b/lldb/test/API/lang/swift/generic_existentials/TestGenericExistentials.py index 932d598b0babf..e0768afb05784 100644 --- a/lldb/test/API/lang/swift/generic_existentials/TestGenericExistentials.py +++ b/lldb/test/API/lang/swift/generic_existentials/TestGenericExistentials.py @@ -1,6 +1,4 @@ import lldbsuite.test.lldbinline as lldbinline from lldbsuite.test.decorators import * -lldbinline.MakeInlineTest(__file__, globals(), decorators=[swiftTest, - expectedFailureAll #FIXME: This regressed silently due to 2c911bceb06ed376801251bdfd992905a66f276c - ]) +lldbinline.MakeInlineTest(__file__, globals(), decorators=[swiftTest]) diff --git a/lldb/test/API/lang/swift/opaque_existentials/TestOpaqueExistentials.py b/lldb/test/API/lang/swift/opaque_existentials/TestOpaqueExistentials.py index 932d598b0babf..e0768afb05784 100644 --- a/lldb/test/API/lang/swift/opaque_existentials/TestOpaqueExistentials.py +++ b/lldb/test/API/lang/swift/opaque_existentials/TestOpaqueExistentials.py @@ -1,6 +1,4 @@ import lldbsuite.test.lldbinline as lldbinline from lldbsuite.test.decorators import * -lldbinline.MakeInlineTest(__file__, globals(), decorators=[swiftTest, - expectedFailureAll #FIXME: This regressed silently due to 2c911bceb06ed376801251bdfd992905a66f276c - ]) +lldbinline.MakeInlineTest(__file__, globals(), decorators=[swiftTest]) diff --git a/lldb/test/API/lang/swift/unknown_self/TestSwiftUnknownSelf.py b/lldb/test/API/lang/swift/unknown_self/TestSwiftUnknownSelf.py index 9cb54f875ca4a..378dcdc8a0620 100644 --- a/lldb/test/API/lang/swift/unknown_self/TestSwiftUnknownSelf.py +++ b/lldb/test/API/lang/swift/unknown_self/TestSwiftUnknownSelf.py @@ -33,7 +33,6 @@ def check_class(self, var_self, broken): self.expect("fr v self", substrs=["hello", "world"]) - @skipIf #FIXME: This regressed silently due to 2c911bceb06ed376801251bdfd992905a66f276c @skipIf(bugnumber="SR-10216", archs=['ppc64le']) @swiftTest def test_unknown_self_objc_ref(self): diff --git a/lldb/test/API/lang/swift/variables/error_type/TestSwiftErrorType.py b/lldb/test/API/lang/swift/variables/error_type/TestSwiftErrorType.py index 269dcb7e53910..fece1dd2df708 100644 --- a/lldb/test/API/lang/swift/variables/error_type/TestSwiftErrorType.py +++ b/lldb/test/API/lang/swift/variables/error_type/TestSwiftErrorType.py @@ -12,7 +12,4 @@ import lldbsuite.test.lldbinline as lldbinline from lldbsuite.test.decorators import * -lldbinline.MakeInlineTest(__file__, globals(), decorators=[ -#FIXME: This regressed silently due to 2c911bceb06ed376801251bdfd992905a66f276c - expectedFailureAll, - swiftTest]) +lldbinline.MakeInlineTest(__file__, globals(), decorators=[swiftTest])