Skip to content
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.

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

[SR-4328] Dropping @_transparent breaks the world #46911

Open
dabrahams opened this issue Mar 24, 2017 · 21 comments
Open

[SR-4328] Dropping @_transparent breaks the world #46911

dabrahams opened this issue Mar 24, 2017 · 21 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@dabrahams
Copy link
Collaborator

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 415736b 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 415736b 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 a70a3da [PackageLoading] Remove sandbox dir requirement from manifest loader

@dabrahams
Copy link
Collaborator Author

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

@dabrahams
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

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

@dabrahams
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@atrick
Copy link
Member

atrick commented Mar 24, 2017

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: 415736b

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
Copy link
Member

atrick commented Mar 24, 2017

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
Copy link
Member

atrick commented Mar 24, 2017

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Member

atrick commented Mar 24, 2017

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

@aschwaighofer
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@aschwaighofer
Copy link
Member

@atrick
Copy link
Member

atrick commented Mar 24, 2017

@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
Copy link
Member

atrick commented Mar 24, 2017

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

@swiftix
Copy link
Mannequin

swiftix mannequin commented Mar 25, 2017

The fix for String concatenation is here: #8325

@dabrahams
Copy link
Collaborator Author

@swiftix Should this be closed now?

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Development

No branches or pull requests

3 participants