This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers] Teach the internal demangler about Swift names
ClosedPublic

Authored by zaks.anna on Apr 14 2016, 3:31 PM.

Details

Summary

Add support for Swift names when symbolicating sanitizer traces. This is now relevant since TSan and ASan support have been added to Swift on OS X.

The swift demangler will return null if we are dealing with a non-Swift string, which allows to chain the CXX demangler. I am using swift_demangle function implemented here: https://github.com/apple/swift/pull/2169/commits/4ba7e418cb30e9fd10908bcbb6e9c91d28b8069c

(The test is only testing that no regressions are introduced to the non-Swift demangling since the Swift runtime is not accessible from compiler-rt. I plan on adding further testing of this in the Swift codebase.)

Diff Detail

Event Timeline

zaks.anna updated this revision to Diff 53795.Apr 14 2016, 3:31 PM
zaks.anna retitled this revision from to [sanitizers] Teach the internal demangler about Swift names.
zaks.anna updated this object.
zaks.anna added reviewers: samsonov, kubamracek.
zaks.anna added a subscriber: llvm-commits.
kcc added a subscriber: kcc.

Anna,
Sadly, Alexey has left our team (he may still want to comment, but I wouldn't count on it).
I'll let Mike review this further.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
77

does this mean that we call dlsym every time we are trying to demangle something?

lib/sanitizer_common/tests/sanitizer_symbolizer_test.cc
59

So, is a swift-specific test possible?

zaks.anna added inline comments.Apr 15 2016, 10:46 AM
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc
77

Yes. I'll add a check for the Swift name prefix before calling dlsym.

lib/sanitizer_common/tests/sanitizer_symbolizer_test.cc
59

The dlsym will fail since compiler-rt does not depend on Swift runtime that provides the demangler. The test below is a Swift name, but we are only checking that we don't crash or turn it into something else here.

We should be able to add a test to the Swift repo, which is next on my list. (That is blocked on making sure the Swift buildbots checkout compiler-rt.)

zaks.anna updated this revision to Diff 53914.Apr 15 2016, 10:48 AM

Only call dlsym if we have a Swift name.

zaks.anna marked an inline comment as done.Apr 15 2016, 10:48 AM

Addressed Kostya's comments.

aizatsky added inline comments.Apr 15 2016, 11:04 AM
lib/sanitizer_common/sanitizer_symbolizer_internal.h
31

should probably be DemangleSwiftAndCXX because of the demangling order.

lib/sanitizer_common/tests/sanitizer_symbolizer_test.cc
59

Let's have two separate test cases: DemangleCXX and DemangleSwift.

Let's also make the comment a statement.
// Swift names are not demangled in default llvm build because Swift runtime is not linked in.

zaks.anna added inline comments.Apr 15 2016, 11:24 AM
lib/sanitizer_common/tests/sanitizer_symbolizer_test.cc
59

Let's have two separate test cases: DemangleCXX and DemangleSwift.

It's important to test DemangleCXXAndSwift specifically since it's the only API that others will call and testing it ensures that the chaining works. (DemangleCXX and DemangleSwift are not exposed in the header and should not be.)

aizatsky added inline comments.Apr 15 2016, 11:28 AM
lib/sanitizer_common/tests/sanitizer_symbolizer_test.cc
59

I mean this:

TEST(Symbolizer, DemangleCXX) {
  EXPECT_STREQ("f1(char*, int)", DemangleCXXAndSwift("_Z2f1Pci"));
  EXPECT_STREQ("foo", DemangleCXXAndSwift("foo"));
  EXPECT_STREQ("", DemangleCXXAndSwift(""));
}

TEST(Symbolizer, DemangleSwift) {
  // ...
  EXPECT_STREQ("_TtSd", DemangleCXXAndSwift("_TtSd"));
}
zaks.anna added inline comments.Apr 15 2016, 2:19 PM
lib/sanitizer_common/tests/sanitizer_symbolizer_test.cc
59

But the following are not testing either Swift or CXX:

EXPECT_STREQ("foo", DemangleCXXAndSwift("foo"));
EXPECT_STREQ("", DemangleCXXAndSwift(""));

The point of this whole test is to check that the combined function works. We are not testing CXX or Swift detangling.. I can change it but I do not think that would be best.

aizatsky accepted this revision.Apr 15 2016, 3:29 PM
aizatsky edited edge metadata.
This revision is now accepted and ready to land.Apr 15 2016, 3:29 PM
This revision was automatically updated to reflect the committed changes.