Skip to content

[lldb] Fix infinite recursion in ReconstructType #7023

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 1 commit into from
Jun 23, 2023
Merged

[lldb] Fix infinite recursion in ReconstructType #7023

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 32 additions & 26 deletions lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3958,33 +3958,39 @@ swift::TypeBase *SwiftASTContext::ReconstructType(ConstString mangled_typename,
.getPointer();
assert(!found_type || &found_type->getASTContext() == ast_ctx);

// Objective-C classes sometimes have private subclasses that are invisible to
// the Swift compiler because they are declared and defined in a .m file. If
// we can't reconstruct an ObjC type, walk up the type hierarchy until we find
// something we can import, or until we run out of types
while (!found_type) {
CompilerType clang_type = GetAsClangType(mangled_typename);
if (!clang_type)
break;
// If the typeref type system is disabled GetAsClangType will eventually call
// ReconstructType again, eventually leading to a stack overflow.
if (ModuleList::GetGlobalModuleListProperties()
Copy link

Choose a reason for hiding this comment

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

Instead of relying on that implicit contract, would it be possible to make that explicit? For example by marking the type as already having gone through this?

Copy link
Author

Choose a reason for hiding this comment

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

#7020 implements the fix by keeping a set of types currently being processed, is that what you mean?

Copy link

Choose a reason for hiding this comment

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

I think Jonas is suggesting something like a wrapper type instead of having all these functions deal in a currency of strings. I like your point Jonas, but the way I picture it, it would be "messy".

For alternatives, I also suggested the following:

  1. Having TypeSystemSwiftTypeRef::IsImportedType ignore the symbols.use-swift-typeref-typesystem setting
  2. Removing symbols.use-swift-typeref-typesystem altogether, I am not aware of the who uses it, I think it was a to disable TypeSystemSwiftTypeRef during initial transition – but I don't know if it's still needed

to the latter point, the existence of this infinite recursion could indicate nobody is using this setting.

Copy link

Choose a reason for hiding this comment

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

Yeah, basically what Dave is describing. The most naive implementation being something like:

struct TypeName {
  std::string mangled_typename;
  bool seen = false; 
}

and then you set seen to true after you've seen the type the first time to avoid the recursion.

Copy link
Author

Choose a reason for hiding this comment

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

@JDevlieghere if you mean changing the argument from a string to something like TypeName, I think that'd be a lot of work to support something that can be solved cleanly inside this function. We'd need to change the opaque type of TypeSystemSwiftTypeRef from a string to the new type (since the recursion path back to this function goes through TypeSystemSwiftTypeRef), and I don't think that's worth doing, especially because we have 2 solutions that are less invasive.

Having TypeSystemSwiftTypeRef::IsImportedType ignore the symbols.use-swift-typeref-typesystem setting

Personally I'm not in favor of this because the user explicitly turned on the setting (it's defaulted to off, so if they turned it on they probably have a good reason for it), and it's possible this would change the answer for some types (like the ones the compiler special cases, or types we have shims for).

Removing symbols.use-swift-typeref-typesystem altogether, I am not aware of the who uses it, I think it was a to disable TypeSystemSwiftTypeRef during initial transition – but I don't know if it's still needed

I think this has some issues too:
1 - we just don't know how many people are relying on this setting (I agree with you that's probably a very small minority, but we shouldn't break them if we don't have to).
2 - it's a useful setting for us, to validate if there's a bug in the typeref typesystem or not (I use it every so often when investigating bugs which I think might be typesystem related).

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation. Sounds like something we want to keep in mind in case we need to make changes here in the future, but well beyond the scope of this patch.

.GetUseSwiftTypeRefTypeSystem()) {
// Objective-C classes sometimes have private subclasses that are invisible
// to the Swift compiler because they are declared and defined in a .m file.
// If we can't reconstruct an ObjC type, walk up the type hierarchy until we
// find something we can import, or until we run out of types
while (!found_type) {
CompilerType clang_type = GetAsClangType(mangled_typename);
if (!clang_type)
break;

auto clang_ctx =
clang_type.GetTypeSystem().dyn_cast_or_null<TypeSystemClang>();
if (!clang_ctx)
break;
auto *interface_decl = TypeSystemClang::GetAsObjCInterfaceDecl(clang_type);
if (!interface_decl)
break;
auto *super_interface_decl = interface_decl->getSuperClass();
if (!super_interface_decl)
break;
CompilerType super_type = clang_ctx->GetTypeForDecl(super_interface_decl);
if (!super_type)
break;
auto super_mangled_typename = super_type.GetMangledTypeName();
found_type = swift::Demangle::getTypeForMangling(
*ast_ctx, super_mangled_typename.GetStringRef())
.getPointer();
assert(!found_type || &found_type->getASTContext() == ast_ctx);
auto clang_ctx =
clang_type.GetTypeSystem().dyn_cast_or_null<TypeSystemClang>();
if (!clang_ctx)
break;
auto *interface_decl =
TypeSystemClang::GetAsObjCInterfaceDecl(clang_type);
if (!interface_decl)
break;
auto *super_interface_decl = interface_decl->getSuperClass();
if (!super_interface_decl)
break;
CompilerType super_type = clang_ctx->GetTypeForDecl(super_interface_decl);
if (!super_type)
break;
auto super_mangled_typename = super_type.GetMangledTypeName();
found_type = swift::Demangle::getTypeForMangling(
*ast_ctx, super_mangled_typename.GetStringRef())
.getPointer();
assert(!found_type || &found_type->getASTContext() == ast_ctx);
}
}

if (found_type) {
Expand Down