[SR-4328] Dropping @_transparent breaks the world

[dabrahams]

Previous ID	SR-4328
Radar	None
Original Reporter	@dabrahams
Type	Bug

Additional Detail from JIRA

| | | |------------------|-----------------| |Votes | 0 | |Component/s | Compiler | |Labels | Bug | |Assignee | @swiftix | |Priority | Medium | md5: 573003bea8ca1e5172797e8d4f0e90ce

Issue Description:

With 415736bd873 from github/master, change the _isUnique function in stdlib/public/core/Builtin.swift from @_versioned @_transparent internal to public and rebuild. Then compile and run this file for a "Illegal instruction: 4". If you compile with debug symbols, lldb won't even be able to attach to the process.

let sample = "a"
  + "გთხოვთ ახლავე გაიაროთ რეგისტრაცია Unicode-ის მეათე საერთაშორისო\n"
  + "x"

Other hashes

clang ec61374e55 Merge remote-tracking branch 'origin/swift-4.0-branch' into stable
cmark d875488 Merge pull request #4 from llvm-beanz/generate-cmark-exports
compiler-rt 572336a0b Merge remote-tracking branch 'origin/swift-4.0-branch' into stable
llbuild 68b2954 Merge pull request #131 from ddunbar/directory-contents-recomputation-take-2
lldb 9ca9758f9 Merge pull request #149 from bitjammer/swift-typealias-equal-sourceloc
llvm a95654d8878 Merge remote-tracking branch 'origin/swift-4.0-branch' into stable
ninja 0b0374e Merge pull request #1255 from tchajed/bind-localhost
swift 415736bd873 Merge pull request #8295 from swiftix/wip-gsb-layout-constrains-fixes
swift-corelibs-foundation ba5134d Fix for [SR-1250] NSJSONSerialization emits non-floating-point numbers as Double (#914)
swift-corelibs-libdispatch d137aa4 Makes the DispatchIO initializer that accepts a path failable, reflecting the fact that a relative or non-existent path is invalid.
swift-corelibs-xctest 50cc074 XCTestAssertNoThrow (#184)
swift-integration-tests 8ae3586 Merge pull request #18 from apple/disable-lldb-test
swift-xcode-playground-support 9e980f2 Temporary disable test to get the incremental bot blue again
swiftpm a70a3da3 [PackageLoading] Remove sandbox dir requirement from manifest loader

[dabrahams]

Just repro'd this on my machine at home using 73bc62c5370 from github/master (a later commit). I'm going to try a clean build too, FWIW.

[dabrahams]

A clean build breaks too.

Note that this is not important because I particularly want to drop @_transparent, but because I am reproducing similar problems with much more interesting test cases (no diagnostics, but program exits with code 254 before lldb can even attach). This is just a much simpler reproducer and I think it indicates a real deep reliability problem.

[dabrahams]

Testing on the bots to make sure this isn't specific to the toolchains I've got installed (Xcode-8W109m).

Okay, the bots didn't reproduce it. Now installing what the bots use, Xcode 8.3 beta 5 (8E161), locally.

[dabrahams]

Well, I reproduced it again with the latest tools (and a fresh build).

I'd be indebted if someone else would try this with the same build configuration I'm using:

build-script --skip-build-ios-device --skip-build-tvos-device --debug-swift-stdlib --skip-build-benchmarks --release --swift-stdlib-assertions --swift-stdlib-build-type=Debug --

[dabrahams]

Note: it doesn't reproduce with a ReleaseAssert build:

build-script --skip-build-ios-device --skip-build-tvos-device --release

[dabrahams]

It also doesn't reproduce with a release build of the standard library with assertions turned on:

build-script --skip-build-ios-device --skip-build-tvos-device --release --swift-stdlib-assertions

[dabrahams]

assigning to @ematejska for delegation because I think I've reached the limits of my understanding on this one.

[atrick]

Not sure if this is helpful but...

I just built using your stdlib debug options:

./utils/build-script --skip-build-ios-device --skip-build-tvos-device --debug-swift-stdlib --skip-build-benchmarks --release --swift-stdlib-assertions --swift-stdlib-build-type=Debug --

swift github/master: 415736bd873a

I used ToT github/master for all the other repos. (variable #1)

Toolchain: 8E103 (variable #2)

 /// Returns `true` if `object` is uniquely referenced.
-@_versioned
-@_transparent
-internal func _isUnique<T>(_ object: inout T) -> Bool {
+//@_versioned
+//@_transparent
+//internal
+public func _isUnique<T>(_ object: inout T) -> Bool {
}

Your sample program compiles and runs fine for me, and works with lldb.

I'm rebuilding now with all your github hashes and 8W109m.

[atrick]

Ok. I'm reproducing the problem now. It is not actually sensitive to the non-swift hashes or toolchain.

With direct execution I get a backtrace:

3  libswiftCore.dylib       0x000000011364cc6c std::__1::pair<swift::MetadataCache<(anonymous namespace)::GenericCacheEntry>::Entry*, bool> swift::ConcurrentMap<swift::MetadataCache<(anonymous namespace)::GenericCacheEntry>::Entry, false, swift::MetadataAllocator>::getOrInsert<swift::MetadataCache<(anonymous namespace)::GenericCacheEntry>::Key>(swift::MetadataCache<(anonymous namespace)::GenericCacheEntry>::Key) + 1292
4  libswiftCore.dylib       0x0000000113319451 _T0s11_HeapBufferV20isUniquelyReferencedSbyF + 49
5  libswiftCore.dylib       0x000000011349acad _T0s13_StringBufferV4growSbs5RangeVySVG9oldBounds_Si12newUsedCounttF + 1309

In lldb I get:

  * frame #&#8203;0: 0x00000001001055dc libswiftCore.dylib`Swift._isUnique <A> (inout A) -> Swift.Bool + 12
    frame #&#8203;1: 0x0000000100215451 libswiftCore.dylib`Swift._HeapBuffer.isUniquelyReferenced () -> Swift.Bool + 49
    frame #&#8203;2: 0x0000000100396cad libswiftCore.dylib`Swift._StringBuffer.grow (oldBounds : Swift.Range<Swift.UnsafeRawPointer>, newUsedCount : Swift.Int) -> Swift.Bool + 1309
    frame #&#8203;3: 0x000000010039a928 libswiftCore.dylib`Swift._StringCore._claimCapacity (Swift.Int, minElementWidth : Swift.Int) -> (Swift.Int, Swift.Optional<Swift.UnsafeMutableRawPointer>) + 1880
    frame #&#8203;4: 0x000000010039ab5b libswiftCore.dylib`Swift._StringCore._growBuffer (Swift.Int, minElementWidth : Swift.Int) -> Swift.UnsafeMutableRawPo

[atrick]

Dave, I don't know why you had a problem with lldb.

The trap that I'm hitting is a safety mechanism that ensures _isUnique is working as intended. It needs to be `@transparent` so the compiler can properly handle the bridged pointer checks. That's why we can't expose _isUnique at this level to users. Your object type needs to statically conform to AnyObject for the compiler to generate the right code.

Note that changing the attributes on _isUnique does not affect code gen at all. The purely generic version of that code simply should never be called.

[dabrahams]

Thanks, Andy, this is very helpful indeed.

Well, apparently lldb not working is sensitive to toolchain, because with the new one I can debug it. To hear that static conformance to AnyObject is crucial is worrisome, because it would mean there's no way to build the CoW optimization on top of an enum, which is part of our plan for String. We need to be able to call this method on a reference-sized enum that can contain at least one reference payload.

[dabrahams]

By the way, I'm reproducing this with

let sample = "a" + "გ" + "x"

I thought literal string concatenations were supposed to be a mandatory optimization, but this appears to demonstrate that the optimization doesn't kick in in all cases.

[atrick]

aschwaighofer@apple.com (JIRA User)is this a known issue with optimizing non-ASCI String's or does it merit a new bug?

[aschwaighofer]

I have not worked on string concatenation but from quickly browsing the source I would say we intend to do this and run it as part of diagnostic constant propagation.

[aschwaighofer]

Does it reproduce with a stdlib build with optimization and without asserts? That is the only configuration we usually guarantee for semantic optimizations to really work.

[aschwaighofer]

Hmm, does not work with a recent toolchain. So I would say: yes that warrants a bug

[aschwaighofer]

https://bugs.swift.org/browse/SR-4348

[atrick]

@dabrahams: of course, we also allow _isUnique(Builtin.BridgeObject). If you want CoW checks on something enum-like, you should probably do something like _BridgeStorage. If that's not good enough, we could always change the compiler to handle single-payload reference enums, but I'm not sure that's as robust. Either way, that doesn't solve the underlying problem here that _isUnique must be inlined to the point at which the compiler knows it's static type is compatible with the runtime call that it wants to emit. We don't want to support fully generic codegen for _isUnique, because calling the generic version would never be the intended behavior.

[atrick]

aschwaighofer@apple.com (JIRA User): Thanks! I just assumed you did all String optimization things without checking.

[665a6372-7905-4d90-89bd-872a3b8a2537]

The fix for String concatenation is here: https://github.com/apple/swift/pull/8325

[dabrahams]

@swiftix Should this be closed now?