[Runtime][Reflection][swift-inspect] Add facilities for tracking and examining backtraces for metadata allocations. #32244

mikeash · 2020-06-08T19:24:58Z

rdar://problem/63674755

mikeash · 2020-06-08T20:34:16Z

@swift-ci please test

swift-ci · 2020-06-08T20:47:42Z

Build failed
Swift Test OS X Platform
Git Sha - a8e4e5375be6f9bec3ea2c4e81aac27ac4128537

swift-ci · 2020-06-08T21:11:54Z

Build failed
Swift Test Linux Platform
Git Sha - a8e4e5375be6f9bec3ea2c4e81aac27ac4128537

mikeash · 2020-06-09T14:34:21Z

@swift-ci please test

swift-ci · 2020-06-09T14:36:54Z

Build failed
Swift Test OS X Platform
Git Sha - a8e4e5375be6f9bec3ea2c4e81aac27ac4128537

swift-ci · 2020-06-09T14:37:09Z

Build failed
Swift Test Linux Platform
Git Sha - a8e4e5375be6f9bec3ea2c4e81aac27ac4128537

include/swift/Reflection/ReflectionContext.h

stdlib/public/runtime/Metadata.cpp

include/swift/Reflection/ReflectionContext.h

include/swift/SwiftRemoteMirror/SwiftRemoteMirror.h

stdlib/public/SwiftRemoteMirror/SwiftRemoteMirror.cpp

compnerd · 2020-06-10T23:05:00Z

stdlib/public/SwiftRemoteMirror/SwiftRemoteMirror.cpp

+  auto Error = Context->iterateMetadataAllocationBacktraces([&]
+    (auto AllocationPtr, auto Count, auto Ptrs) {
+    std::vector<swift_reflection_ptr_t> ConvertedPtrs{&Ptrs[0], &Ptrs[Count]};
+    Call(AllocationPtr, Count, ConvertedPtrs.data(), ContextPtr);


This now makes me wonder ... ConvertedPtrs.data() is effectively just Ptrs. I think that this call is effectively:

Call(AllocationPtr, Count, Ptrs, ContextPtr);

That's true only when swift_reflection_ptr_t and StoredPointer are the same. When they aren't (as is the case for 32-bit targets) then this is needed to convert the values.

Ah, right, the foreign case. A comment about that would be nice!

You got it!

compnerd · 2020-06-11T03:21:28Z

tools/swift-inspect/Sources/swift-inspect/Backtrace.swift

+  func symbolicatedLong(inspector: Inspector) -> String {
+    return ptrs.reversed().enumerated().map {
+      let indent = String(repeating: "  ", count: $0 + 1)
+      return indent + symbolString(ptr: $1, inspector: inspector)


Another option would be to pass indentation to the symbolString and have the two just be different separators.

compnerd · 2020-06-11T03:22:26Z

tools/swift-inspect/Sources/swift-inspect/Inspector.swift

@@ -67,11 +67,19 @@ class Inspector {
  }

  func getAddr(symbolName: String) -> swift_addr_t {
-    let symbol = CSSymbolOwnerGetSymbolWithMangledName(swiftCore, symbolName)
+    let symbol = CSSymbolOwnerGetSymbolWithMangledName(swiftCore,
+                                                       "_" + symbolName)


Doesn't this need to take into account the platform before adding a user label prefix? Linux, android, Windows x86_64 and ARM do not use _ (Windows x86, MIPS, and Darwin do).

This is Darwin-specific code, as the CS* calls are only available there. Support for other OSes will need a new Inspector implementation that uses the appropriate calls for that OS.

Fair enough. Though, it raises the question of whether this should be inside CS* or outside still.

tools/swift-inspect/Sources/swift-inspect/RemoteMirrorExtensions.swift

compnerd · 2020-06-11T03:25:19Z

tools/swift-inspect/Sources/swift-inspect/main.swift

    }
    print(metadata.name)
+    if let allocation = metadata.allocation {
+      if let style = backtraceStyle {


Could probably pull this bit out into an @inline(__always) function.

compnerd · 2020-06-11T03:26:02Z

tools/swift-inspect/Sources/swift-inspect/symbolication.swift

+
+func CSSymbolOwnerGetName(_ sym: CSTypeRef) -> String? {
+  let name = Sym.CSSymbolOwnerGetName(sym)
+  return name.map({ String(cString: $0) })


Unnecessary parens

Suggested change

return name.map({ String(cString: $0) })

func CSSymbolOwnerGetName(_ sym: CSTypeRef) -> String? {

Sym.CSSymbolOwnerGetName(sym)

.map(String.init(cString:))

compnerd

Probably should address Ben's comment before merging. Thanks for the iteration on this.

…examining backtraces for metadata allocations. rdar://problem/63674755

mikeash · 2020-06-11T18:50:31Z

@compnerd I made Ben's suggested change as well. Always a pleasure to get good feedback.

mikeash · 2020-06-11T18:50:48Z

@swift-ci please test

swift-ci · 2020-06-11T18:53:33Z

Build failed
Swift Test Linux Platform
Git Sha - bff26ed0d33865e92456d3e390562f21e8e1f303

swift-ci · 2020-06-11T18:53:35Z

Build failed
Swift Test OS X Platform
Git Sha - bff26ed0d33865e92456d3e390562f21e8e1f303

airspeedswift · 2020-06-12T01:53:08Z

To be clear, my comment was mostly for my own idle amusement rather than an important change...

Can't wait to use this!

mikeash · 2020-06-12T13:59:59Z

@airspeedswift I think you just really hate the return keyword. 😄

mikeash · 2020-06-12T14:05:18Z

@swift-ci please test

airspeedswift · 2020-06-12T16:58:55Z

@mikeash you're not wrong there

davezarzycki · 2020-06-13T11:24:50Z

Hi @mikeash – Git bisect has revealed this as breaking my Fedora 32 (linux, x86_64) machine. Can we revert this or commit a quick fix?

FAIL: Swift(linux-x86_64) :: stdlib/symbol-visibility-linux.test-sh (12690 of 13369)
******************** TEST 'Swift(linux-x86_64) :: stdlib/symbol-visibility-linux.test-sh' FAILED ********************
Script:
--
: 'RUN: at line 14';   rm -rf "/home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp" && mkdir -p "/home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp"
: 'RUN: at line 16';   /home/dave/b/u/t/bin/llvm-nm --defined-only --extern-only /home/dave/b/u/t/lib/swift/linux/x86_64/libswiftCore.so    | grep -v -e _ZNSt6vectorIjSaIjEE6insertEN9__gnu_cxx17__normal_iteratorIPKjS1_EERS4_              -e _ZNSt6vectorIjSaIjEE13_M_insert_auxIJRKjEEEvN9__gnu_cxx17__normal_iteratorIPjS1_EEDpOT_              -e _ZNSt6vectorIjSaIjEE13_M_insert_auxIJjEEEvN9__gnu_cxx17__normal_iteratorIPjS1_EEDpOT_              -e _ZNSt6vectorISt10unique_ptrIKvSt8functionIFvPS1_EEESaIS6_EE19_M_emplace_back_auxIJS6_EEEvDpOT_              -e _ZNSt6vectorISt10unique_ptrIKvSt8functionIFvPS1_EEESaIS6_EE17_M_realloc_insertIJS6_EEEvN9__gnu_cxx17__normal_iteratorIPS6_S8_EEDpOT_              -e _ZN9__gnu_cxx12__to_xstringINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEcEET_PFiPT0_mPKS8_P13__va_list_tagEmSB_z              -e _ZZNSt19_Sp_make_shared_tag5_S_tiEvE5__tag              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEDnEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA1_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA8_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA24_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA32_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA40_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA88_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA104_cEEEvv    > /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftCore-all.txt
: 'RUN: at line 33';   /home/dave/b/u/t/bin/llvm-nm --defined-only --extern-only --no-weak /home/dave/b/u/t/lib/swift/linux/x86_64/libswiftCore.so > /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftCore-no-weak.txt
: 'RUN: at line 34';   diff -u /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftCore-all.txt /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftCore-no-weak.txt
: 'RUN: at line 36';   /home/dave/b/u/t/bin/llvm-nm --defined-only --extern-only /home/dave/b/u/t/lib/swift/linux/x86_64/libswiftRemoteMirror.so    | grep -v -e _ZNSt6vectorIjSaIjEE6insertEN9__gnu_cxx17__normal_iteratorIPKjS1_EERS4_              -e _ZNSt6vectorIjSaIjEE13_M_insert_auxIJRKjEEEvN9__gnu_cxx17__normal_iteratorIPjS1_EEDpOT_              -e _ZNSt6vectorIjSaIjEE13_M_insert_auxIJjEEEvN9__gnu_cxx17__normal_iteratorIPjS1_EEDpOT_              -e _ZNSt6vectorISt10unique_ptrIKvSt8functionIFvPS1_EEESaIS6_EE19_M_emplace_back_auxIJS6_EEEvDpOT_              -e _ZNSt6vectorISt10unique_ptrIKvSt8functionIFvPS1_EEESaIS6_EE17_M_realloc_insertIJS6_EEEvN9__gnu_cxx17__normal_iteratorIPS6_S8_EEDpOT_              -e _ZN9__gnu_cxx12__to_xstringINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEcEET_PFiPT0_mPKS8_P13__va_list_tagEmSB_z              -e _ZZNSt19_Sp_make_shared_tag5_S_tiEvE5__tag              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEDnEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA1_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA8_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA24_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA32_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA40_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA88_cEEEvv              -e _ZSt16__once_call_implISt12_Bind_simpleIFPFvPvEPA104_cEEEvv    > /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftRemoteMirror-all.txt
: 'RUN: at line 53';   /home/dave/b/u/t/bin/llvm-nm --defined-only --extern-only --no-weak /home/dave/b/u/t/lib/swift/linux/x86_64/libswiftRemoteMirror.so > /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftRemoteMirror-no-weak.txt
: 'RUN: at line 54';   diff -u /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftRemoteMirror-all.txt /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftRemoteMirror-no-weak.txt
--
Exit Code: 1

Command Output (stdout):
--
--- /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftRemoteMirror-all.txt  2020-06-13 07:16:26.364449497 -0400
+++ /home/dave/b/u/t/tools/swift/test-linux-x86_64/stdlib/Output/symbol-visibility-linux.test-sh.tmp/swiftRemoteMirror-no-weak.txt 2020-06-13 07:16:26.384449714 -0400
@@ -1,9 +1,3 @@
-000000000008f880 V _ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_13basic_istreamIcS2_EE
-000000000008f808 V _ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE0_NS_14basic_iostreamIcS2_EE
-000000000008f8d0 V _ZTCNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE16_NS_13basic_ostreamIcS2_EE
-000000000008f7b8 V _ZTTNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
-000000000008f920 V _ZTVNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEE
-000000000008f740 V _ZTVNSt3__118basic_stringstreamIcNS_11char_traitsIcEENS_9allocatorIcEEEE
 000000000002bbd0 T swift_reflection_addImage
 000000000002b7e0 T swift_reflection_addReflectionInfo
 0000000000031380 T swift_reflection_allocationMetadataPointer

--

davezarzycki · 2020-06-15T13:15:49Z

It looks like this test was added recently but nothing (as far as I can tell) in the Swift build system forces the GNU C++ standard library to be used, so if the host compiler defaults to LLVM's C++ library, then this test is at risk of failing.

I've also been using LLVM's C++ standard library for a long time, so this breakage is unfortunate.

davezarzycki · 2020-06-15T13:41:24Z

@mikeash – So I'm not a python expert. How hard would it be to get the Swift lit setup to run ldd against the stdlib and see which C++ runtime is being used so that we can add a REQUIRES: libstdc++ to the test?

davezarzycki · 2020-06-15T13:50:41Z

CC: @compnerd because he wrote the brittle test.

compnerd · 2020-06-15T14:43:14Z

There’s definitely some assumptions around the C++ runtime on different platforms. I don’t think that the test would fail on libc++ though. The filtering there is to work around the libstdc++ forcing exported symbols.

The diff appears to indicate that you’re building with RTTI? The standard disables RTTI which I think is causing some undesired (weak) exports of the type information.

davezarzycki · 2020-06-15T15:19:05Z

Weird. My setup is not explicitly disabling RTTI and I wouldn’t knowingly enable RTTI. I’d wager that either the runtime isn’t consistently disabling RTTI or this is pull request is exposing a latent bug in LLVM’s libc++.

tbkka · 2020-06-15T16:24:37Z

If we add the symbols you found to that test, would that make the test pass for both C++ libraries?

davezarzycki · 2020-06-15T16:31:11Z

@tbkka – I'd imagine so, but I didn't write the test and I'm not sure if that is the "right" solution.

@compnerd – What do you think?

compnerd · 2020-06-16T06:03:27Z

Yeah, that’s probably a good way to get the tests passing again. Though, we should figure out why libc++ is injecting the vtables in the runtime (sounds like we’re messing up some flags).

Pull request swiftlang#32244 introduced the use of `std::stringstream` but that causes vtables to be generated and we don't want that.

davezarzycki · 2020-06-16T12:10:37Z

So I debugged this a bit. The problem is std::stringstream. This is probably some kind of bug in LLVM's C++ standard library, but I'm not enough of a C++ expert to file a bug. I'm going to workaround this using snprintf. See pull request: #32406

davezarzycki · 2020-06-16T15:28:31Z

https://bugs.llvm.org/show_bug.cgi?id=46351

mikeash requested review from airspeedswift and tbkka and removed request for airspeedswift June 8, 2020 19:26

mikeash force-pushed the metadata-allocation-backtrace-inspection branch from a8e4e53 to bff26ed Compare June 9, 2020 14:34

mikeash requested a review from compnerd June 10, 2020 14:43

compnerd reviewed Jun 10, 2020

View reviewed changes

mikeash force-pushed the metadata-allocation-backtrace-inspection branch from bff26ed to b89894c Compare June 10, 2020 19:38

compnerd reviewed Jun 10, 2020

View reviewed changes

mikeash force-pushed the metadata-allocation-backtrace-inspection branch from b89894c to 65765ab Compare June 10, 2020 21:07

compnerd reviewed Jun 10, 2020

View reviewed changes

compnerd reviewed Jun 11, 2020

View reviewed changes

tools/swift-inspect/Sources/swift-inspect/RemoteMirrorExtensions.swift Show resolved Hide resolved

compnerd reviewed Jun 11, 2020

View reviewed changes

mikeash force-pushed the metadata-allocation-backtrace-inspection branch from 65765ab to 0a6c805 Compare June 11, 2020 16:44

compnerd approved these changes Jun 11, 2020

View reviewed changes

[Runtime][Reflection][swift-inspect] Add facilities for tracking and …

cd624bf

…examining backtraces for metadata allocations. rdar://problem/63674755

mikeash force-pushed the metadata-allocation-backtrace-inspection branch from 0a6c805 to cd624bf Compare June 11, 2020 18:50

mikeash merged commit 6527a21 into swiftlang:master Jun 12, 2020

davezarzycki mentioned this pull request Jun 16, 2020

[Reflection] NFC: Workaround LLVM C++ standard library weirdness #32406

Merged

mikeash mentioned this pull request Jun 19, 2020

stdlib: add a utility to print what and where dynamic metadata is created. #32467

Closed

[Runtime][Reflection][swift-inspect] Add facilities for tracking and examining backtraces for metadata allocations. #32244

[Runtime][Reflection][swift-inspect] Add facilities for tracking and examining backtraces for metadata allocations. #32244

Uh oh!

Conversation

mikeash commented Jun 8, 2020

Uh oh!

mikeash commented Jun 8, 2020

Uh oh!

swift-ci commented Jun 8, 2020

Uh oh!

swift-ci commented Jun 8, 2020

Uh oh!

mikeash commented Jun 9, 2020

Uh oh!

swift-ci commented Jun 9, 2020

Uh oh!

swift-ci commented Jun 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

mikeash commented Jun 11, 2020

Uh oh!

mikeash commented Jun 11, 2020

Uh oh!

swift-ci commented Jun 11, 2020

Uh oh!

swift-ci commented Jun 11, 2020

Uh oh!

airspeedswift commented Jun 12, 2020

Uh oh!

mikeash commented Jun 12, 2020

Uh oh!

mikeash commented Jun 12, 2020

Uh oh!

airspeedswift commented Jun 12, 2020

Uh oh!

davezarzycki commented Jun 13, 2020

Uh oh!

davezarzycki commented Jun 15, 2020

Uh oh!

davezarzycki commented Jun 15, 2020

Uh oh!

davezarzycki commented Jun 15, 2020

Uh oh!

compnerd commented Jun 15, 2020

davezarzycki commented Jun 16, 2020 •

edited

Loading