Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
78 changes: 76 additions & 2 deletions lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilderSwift.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"

#include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
#include "llvm/DebugInfo/CodeView/TypeDeserializer.h"
#include "llvm/DebugInfo/CodeView/TypeVisitorCallbacks.h"
#include "llvm/DebugInfo/PDB/Native/TpiStream.h"
#include "llvm/Support/ErrorHandling.h"

Expand All @@ -28,6 +30,51 @@ using namespace lldb_private::npdb;
using namespace llvm::codeview;
using namespace llvm::pdb;

namespace {
// To avoid issues with uniquing, bound generics are emitted as follows in DWARF:
// - A sized outer structure with no name or identifier
// - A sizeless inner structure with the mangled name as the name and no identifier.
// The latter is an unnamed member of the former.
// CodeView deviates in two major ways:
// - Unnamed types are emitted as module name + `::<unnamed-tag>` instead of having an empty name.
// - Unnamed forward declared members are dropped entirely. To get around this, we name
// the member with the mangled name of the inner type, since this is illegal in a
// user-written type. See https://github.com/swiftlang/swift/issues/87093
struct BoundGenericVisitor : public TypeVisitorCallbacks {
TpiStream &tpi;
llvm::StringRef outer_prefix;
llvm::StringRef mangled_name;
bool seen = false;

BoundGenericVisitor(TpiStream &tpi, llvm::StringRef outer_prefix)
: tpi(tpi), outer_prefix(outer_prefix) {}

llvm::Error visitKnownMember(CVMemberRecord &,
DataMemberRecord &member) override {
if (seen) {
// More than one member = doesn't match the pattern.
mangled_name = {};
return llvm::Error::success();
}
seen = true;
CVType inner_cvt = tpi.getType(member.Type);
if (!llvm::is_contained({LF_STRUCTURE, LF_CLASS}, inner_cvt.kind()))
return llvm::Error::success();
ClassRecord inner;
if (llvm::Error err =
TypeDeserializer::deserializeAs<ClassRecord>(inner_cvt, inner))
return err;
llvm::StringRef inner_mangled = inner.Name;
if (!inner_mangled.consume_front(outer_prefix) ||
inner_mangled != member.Name ||
!swift::Demangle::isSwiftSymbol(member.Name))
return llvm::Error::success();
mangled_name = member.Name;
return llvm::Error::success();
}
};
} // namespace

PdbAstBuilderSwift::PdbAstBuilderSwift(TypeSystemSwiftTypeRef &swift_ts)
: m_swift_ts(swift_ts) {}

Expand All @@ -48,9 +95,36 @@ CompilerType PdbAstBuilderSwift::CreateType(PdbTypeSymId type,
"Failed to deserialize ClassRecord: {0}");
return {};
}
if (!cr.hasUniqueName())
if (cr.hasUniqueName()) {
decorated = cr.UniqueName;
} else if (cr.Name.ends_with("::<unnamed-tag>") &&
!cr.FieldList.isNoneType()) {
// See comment at BoundGenericVisitor for details.
CVType field_list_cvt = tpi.getType(cr.FieldList);
if (field_list_cvt.kind() != LF_FIELDLIST)
return {};
FieldListRecord field_list;
if (auto err = TypeDeserializer::deserializeAs<FieldListRecord>(
field_list_cvt, field_list)) {
LLDB_LOG_ERROR(GetLog(LLDBLog::Symbols), std::move(err),
"Failed to deserialize FieldListRecord: {0}");
return {};
}
// Grab the outer prefix so we can make sure member name matches the inner type name.
llvm::StringRef outer_prefix =
cr.Name.drop_back(llvm::StringRef("<unnamed-tag>").size());
BoundGenericVisitor visitor(tpi, outer_prefix);
if (auto err = visitMemberRecordStream(field_list.Data, visitor)) {
LLDB_LOG_ERROR(GetLog(LLDBLog::Symbols), std::move(err),
"Failed to walk bound generic field list: {0}");
return {};
}
if (visitor.mangled_name.empty())
return {};
decorated = visitor.mangled_name;
} else {
return {};
decorated = cr.UniqueName;
}
break;
}
case LF_ENUM: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,12 @@ static bool IsSwiftType(PdbTypeSymId type_id, PdbIndex& index) {
return false;
ClassRecord cr;
llvm::cantFail(TypeDeserializer::deserializeAs<ClassRecord>(cvt, cr));
return cr.hasUniqueName() && swift::Demangle::isSwiftSymbol(cr.UniqueName);
if (cr.hasUniqueName())
return swift::Demangle::isSwiftSymbol(cr.UniqueName);
// https://github.com/swiftlang/swift/issues/87093
// FIXME: This is more of a heuristic than a definitive check, but we would need to do a
// second field-list walk to be sure.
return cr.Name.ends_with("::<unnamed-tag>");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given the example you gave here: swiftlang/swift#87093 (comment), could MSVC emit ::<unnamed-tag> for a C/C++ type?

If tha'ts the case, thisIsSwiftType would return true on a non Swift type.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If this is possible, we should:

  • Avoid false positives by checking that the outer type is a swift type.
  • Add a negative test in swift-frame-var.test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yup, I can wire the field-list walk up twice. My thinking for not doing it was the perf gain and of not doing it is worth the edge case (no unique name, no name C/C++ type that nevertheless has something useful that TypeSystemClang can squeeze out of it) and cleaner code. I think probably both the perf hit is small and the edge case is not likely to matter though.

What would a good negative case look like?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I tried to build a C++ type that has ::<unnamed-tag> in its name and does not have a unique name but I could not manage to do it. That would have been a good negative test but it does not seem to be possible.

My concern was unfounded.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yep, AFAICT Clang can't produce it, but maybe another compiler can? I added the full check to IsSwiftType since the perf hit isn't that bad and the cognitive overhead is probably worth us being sure.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yep, AFAICT Clang can't produce it, but maybe another compiler can?

I tried with MSVC but couldn't figure it out either.

#else
return false;
#endif
Expand Down
26 changes: 24 additions & 2 deletions lldb/test/Shell/SymbolFile/NativePDB/swift-frame-var.test
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ enum Direction {
case south
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since this is a NativePDB test, is it running on macOS? Asking since the lldb swift tests are currently not enabled on Windows.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As of #12610 yes, unless something changed (I know because I had to remove the XFAIL on this test to land it!)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Wait, I misread the question. It's not running on macOS (requires system-windows) is running on Windows last I checked.

}

struct Box<T> {
var value: T
}

enum Pair<A, B> {
case first(A)
case second(B)
}

func main() {
var myInt32: Int32 = 42
var myBool: Bool = true
Expand All @@ -27,20 +36,27 @@ func main() {
var myString: String = "hello"
var myPoint: Point = Point(x: 10, y: 20)
var myDirection: Direction = .north
var myOptional: Int32? = 7
var myArray: [Int32] = [1, 2, 3]
var myOptionalArray: [Int32]? = [4, 5]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

does this generalize to a nested type like Int32???

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, added a test case

var myResult: Result<Int, Error> = .success(42)
var myBox: Box<Int32> = Box(value: 99)
var myPair: Pair<Int, String> = .first(7)
let constInt: Int = 100
let constInt8: Int8 = -42
let constUInt16: UInt16 = 1000
let constBool: Bool = false
let constString: String = "world"
print(myInt32, myBool, myUInt64, myFloat, myDouble,
myString, myPoint, myDirection,
myString, myPoint, myDirection, myOptional, myArray,
myOptionalArray, myResult, myBox, myPair,
constInt, constInt8, constUInt16, constBool, constString)
}

main()

#--- commands.input
b main.swift:25
b main.swift:40
run
frame variable

Expand All @@ -54,6 +70,12 @@ frame variable
# CHECK: (String) myString = "hello"
# CHECK: (main.Point) myPoint = (x = 10, y = 20)
# CHECK: (main.Direction) myDirection = north
# CHECK: (Int32?) myOptional = 7
# CHECK: ([Int32]) myArray = 3 values
# CHECK: ([Int32]?) myOptionalArray = 2 values
# CHECK: (Result<Int, Error>) myResult = success
# CHECK: (main.Box<Int32>) myBox = (value = 99)
# CHECK: (main.Pair<Int, String>) myPair = first
# Local constants
# CHECK: (Int) constInt = 100
# CHECK: (Int8) constInt8 = -42
Expand Down