Introducing C++ Swift Interop #25602
Conversation
|
EWS run on previous version of this PR (hash 169039f) |
webkit-ews-buildbot
added
the
merging-blocked
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
169039f
to
71c1c00
Compare
|
EWS run on previous version of this PR (hash 71c1c00) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
71c1c00
to
54e19f1
Compare
|
EWS run on previous version of this PR (hash 54e19f1) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
54e19f1
to
fe25d96
Compare
|
EWS run on previous version of this PR (hash fe25d96) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
2 times, most recently
from
f9f277e
to
74a8fda
Compare
|
EWS run on previous version of this PR (hash f9f277e) |
|
EWS run on previous version of this PR (hash 74a8fda) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
74a8fda
to
4f73aa5
Compare
|
EWS run on previous version of this PR (hash 4f73aa5) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
4f73aa5
to
119a2b9
Compare
|
EWS run on previous version of this PR (hash 119a2b9) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
119a2b9
to
9c41113
Compare
|
EWS run on previous version of this PR (hash 9c41113) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
9c41113
to
a61133e
Compare
|
EWS run on previous version of this PR (hash a61133e) |
|
EWS run on previous version of this PR (hash cab7587) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
cab7587
to
16b0bfd
Compare
|
EWS run on previous version of this PR (hash 16b0bfd) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
16b0bfd
to
cd5a7b9
Compare
|
EWS run on previous version of this PR (hash cd5a7b9) |
nmahendru
changed the title
|
EWS run on previous version of this PR (hash 96eec02) |
|
EWS run on previous version of this PR (hash 8b9ae93) |
| extension PALBytes: ContiguousBytes { | ||
| borrowing public func withUnsafeBytes<R>(_ body: (UnsafeRawBufferPointer) throws -> R) rethrows -> R { | ||
| try body(UnsafeRawBufferPointer(start:self.__dataUnsafe(), count: self.size())) |
There was a problem hiding this comment.
Do we have any options to reduce our use of unsafety here? Or do we at least understand what safety might look like in the future?
There was a problem hiding this comment.
The underlying CryptoKitAPI eventually needs an UnsafeBufferPointer. Now matter how we wrap it we will have an unsafe somewhere, if not at this level then inside the wrapper around the API that expects UnsafeBufferPointer.
Source/WebCore/PAL/pal/PALSwift.h
Outdated
| #include <cstdint> | ||
| #include <wtf/Vector.h> | ||
|
|
||
| using PALBytes = WTF::Vector<uint8_t>; |
There was a problem hiding this comment.
Let's call this something like VectorUInt8. We want to be clear that all we've done is write out the template specialization for Vector<uint8_t>, and we want to avoid naming conflict with future specializations, which will be required because Swift only understands specializations and not templates.
| let nonce = try AES.GCM.Nonce( | ||
| data: UnsafeBufferPointer(start: iv, count: Int(ivSize))) |
There was a problem hiding this comment.
Our goal is to write Swift code that is at least consistent with some future vision for safety, even if Swift is not yet safe. If Nonce() is expressed as an unsafe pointer API, I think we want an explicit wrapper or extension or replacement for it, so that our code is expressed in terms of safety. Maybe we need a separate file called "CryptoKitUnsafeAPIWrappers" or something like that, to hold all these abstractions.
There was a problem hiding this comment.
This PR is only for Digest. I am going to cleanup the AESGCM, AESKW wrappers in a future PRs.
| sealedBox.tag.copyBytes( | ||
| to: cipherText + Int(plainTextSize), count: sealedBox.tag.count) |
There was a problem hiding this comment.
No bounds check here, so we need a wrapper that expresses the bounds check.
| using: SymmetricKey( | ||
| data: UnsafeBufferPointer(start: key, count: Int(keySize)))) |
There was a problem hiding this comment.
Need a wrapper that expresses a safe way to construct a key.
There was a problem hiding this comment.
There are other cases like this too, but I won't comment on each one. One way to get a close approximation is to search for "unsafe" in the code; though some unsafe pointers will instantiate implicitly.
nmahendru
force-pushed
the
eng/nitin-interop
branch
2 times, most recently
from
9214182
to
e949499
Compare
| @@ -53,7 +53,8 @@ FRAMEWORK_SEARCH_PATHS[sdk=macosx*] = $(WK_QUOTED_OVERRIDE_FRAMEWORKS_DIR); | |||
|
|
|||
| SYSTEM_FRAMEWORK_SEARCH_PATHS = $(inherited) $(WK_PRIVATE_SDK_DIR)$(SYSTEM_LIBRARY_DIR)/PrivateFrameworks $(SDK_DIR)$(SYSTEM_LIBRARY_DIR)/Frameworks; | |||
|
|
|||
| HEADER_SEARCH_PATHS = PAL ForwardingHeaders /usr/include/libxslt /usr/include/libxml2 $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore $(BUILT_PRODUCTS_DIR)$(WK_LIBRARY_HEADERS_FOLDER_PATH) $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) $(ANGLE_HEADER_SEARCH_PATHS) $(LIBWEBRTC_HEADER_SEARCH_PATHS) $(OBJROOT)/PAL.build/PAL.build/DerivedSources $(OBJROOT)/PAL.build/$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)/PAL.build/DerivedSources $(HEADER_SEARCH_PATHS) $(SRCROOT); | |||
| HEADER_SEARCH_PATHS = PAL ForwardingHeaders /usr/include/libxslt /usr/include/libxml2 $(BUILT_PRODUCTS_DIR)/DerivedSources/WebCore $(BUILT_PRODUCTS_DIR)$(WK_LIBRARY_HEADERS_FOLDER_PATH) $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) $(ANGLE_HEADER_SEARCH_PATHS) $(LIBWEBRTC_HEADER_SEARCH_PATHS) $(OBJROOT)/PAL.build/PAL.build/DerivedSources $(OBJROOT)/PAL.build/$(CONFIGURATION)$(EFFECTIVE_PLATFORM_NAME)/PAL.build/DerivedSources $(HEADER_SEARCH_PATHS) $(SRCROOT) $(SRCROOT)/PAL; | |||
|
|
|||
There was a problem hiding this comment.
Did you remove the line above (L55)? I was asking about below (L57) :P
Basically, this file should have no changes to it.
nmahendru
force-pushed
the
eng/nitin-interop
branch
2 times, most recently
from
bac4d0a
to
229a025
Compare
|
EWS run on previous version of this PR (hash 229a025) |
Source/WebCore/PAL/pal/PALSwift.h
Outdated
| @@ -28,7 +28,7 @@ | |||
| #include <cstdint> | |||
| #include <wtf/Vector.h> | |||
|
|
|||
| using VectorUInt8 = WTF::Vector<uint8_t>; | |||
| using VectorUInt8 = WTF::Vector<uint8_t, 0, CrashOnOverflow, 16, WTF::VectorBufferMalloc, WTF::IsCopyable::No>; | |||
There was a problem hiding this comment.
Can't we just pass std::spans to Swift? Why do we need to pass vectors to these functions?
There was a problem hiding this comment.
If we'd pass a std::span<const uint8_t>, we wouldn't care if Swift copies it since it is cheap to copy
There was a problem hiding this comment.
Removed the nonCopyable and using spans instead.
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
229a025
to
cb30967
Compare
|
EWS run on previous version of this PR (hash cb30967) |
Source/WebCore/PAL/pal/PALSwift.h
Outdated
| #include <wtf/Vector.h> | ||
|
|
||
| using VectorUInt8 = WTF::Vector<uint8_t>; | ||
| using SpanUInt8 = std::span<const uint8_t>; |
There was a problem hiding this comment.
Let's call this SpanConstUInt8, since const is a part of the type.
| extension HashFunction { | ||
| mutating func update(data: SpanUInt8) { | ||
| self.update( | ||
| bufferPointer: UnsafeRawBufferPointer( | ||
| start: data.__dataUnsafe(), count: data.size())) | ||
| } |
There was a problem hiding this comment.
To demonstrate our discipline of programming using safe constructs, and our expectation that future APIs will expose safe constructs, I think it's good for overlays that shim unsafe APIs like this to live in a separate file. Maybe "CryptoKitOverlay.swift", or we can put them all together in "UnsafeOverlays.swift".
If you'd like, you can make this change in the follow-up patch that removes the other unsafe pointers from this file.
| hasher.update(data: data) | ||
| let digest = hasher.finalize() | ||
| var returnValue = VectorUInt8(T.Digest.byteCount) | ||
| returnValue.fill(0) |
There was a problem hiding this comment.
No need to fill with zeroes here.
| let digest = hasher.finalize() | ||
| var returnValue = VectorUInt8(T.Digest.byteCount) | ||
| returnValue.fill(0) | ||
| var counter = 0 |
There was a problem hiding this comment.
We usually call this variable "index".
|
EWS run on previous version of this PR (hash da3b1dd) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
da3b1dd
to
6a483d1
Compare
|
EWS run on previous version of this PR (hash 6a483d1) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
6a483d1
to
d9864dd
Compare
|
EWS run on previous version of this PR (hash d9864dd) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
d9864dd
to
5149a6d
Compare
|
EWS run on previous version of this PR (hash 5149a6d) |
nmahendru
force-pushed
the
eng/nitin-interop
branch
from
5149a6d
to
be30731
Compare
|
EWS run on current version of this PR (hash be30731) |
achristensen07
added
unsafe-merge-queue
https://bugs.webkit.org/show_bug.cgi?id=271263 rdar://123331949 Reviewed by Alex Christensen, Geoffrey Garen and Elliott Williams. This is an examplar of the C++ Swift interop feature. We have randomly chosen the Digest Implementation to be the first one to adopt this. The Compiler feature is only available for newer platforms to code has been guarded with ifdefs. The changes in AbortWithReasonSPI.h are to allow the Swift Generator to work. It, at the moment cannot handle headers included inside `extern C` blocks. Input parameteres to Swift have been intentionally chosen as spans as swift, at the moment my make copies of inputs for safety guarantees. Copying a span is cheap so that is Okie. On the return path, Swift calls the WTF::Vector move constructor(confirmed in debugger). * Source/WTF/wtf/PlatformHave.h: * Source/WTF/wtf/spi/darwin/AbortWithReasonSPI.h: * Source/WebCore/Configurations/WebCore.xcconfig: * Source/WebCore/PAL/Configurations/PAL.xcconfig: * Source/WebCore/PAL/PAL.xcodeproj/project.pbxproj: * Source/WebCore/PAL/pal/PALSwift/CryptoKitShim.swift: (AesGcm.test(_:)): (AesKwRV.errCode): (AesKwRV.outputSize): (HashFunction.update(_:)): (Digest.sha1(_:)): (Digest.sha256(_:)): (Digest.sha384(_:)): (Digest.sha512(_:)): (AesKwReturnValue.errCode): Deleted. (AesKwReturnValue.outputSize): Deleted. * Source/WebCore/PAL/pal/PALSwift/UnsafeOverlays.swift: Added. (HashFunction.update(_:)): * Source/WebCore/PAL/pal/PALSwiftModule.h: Copied from Source/WTF/wtf/spi/darwin/AbortWithReasonSPI.h. * Source/WebCore/PAL/pal/crypto/commoncrypto/CryptoDigestCommonCrypto.cpp: Renamed from Source/WebCore/PAL/pal/crypto/commoncrypto/CryptoDigestCommonCrypto.mm. (PAL::CryptoDigest::computeHash): * Source/WebCore/PAL/pal/crypto/openssl/CryptoDigestOpenSSL.cpp: (PAL::createCryptoDigest): (PAL::CryptoDigest::computeHash): * Source/WebCore/PAL/pal/module.modulemap: Added. * Source/WebCore/SourcesCocoa.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/crypto/CryptoAlgorithm.cpp: (WebCore::CryptoAlgorithm::dispatchDigest): * Source/WebCore/crypto/cocoa/CryptoAlgorithmAESGCMMac.cpp: Renamed from Source/WebCore/crypto/cocoa/CryptoAlgorithmAESGCMMac.mm. (WebCore::encryptCryptoKitAESGCM): (WebCore::CryptoAlgorithmAESGCM::platformEncrypt): * Source/WebCore/crypto/cocoa/CryptoAlgorithmAESKWMac.cpp: Renamed from Source/WebCore/crypto/cocoa/CryptoAlgorithmAESKWMac.mm. (WebCore::wrapKeyAESKWCryptoKit): (WebCore::unwrapKeyAESKWCryptoKit): * Source/WebCore/crypto/cocoa/CryptoUtilitiesCocoa.cpp: Canonical link: https://commits.webkit.org/276651@main
webkit-commit-queue
force-pushed
the
eng/nitin-interop
branch
from
be30731
to
e88f01a
Compare
|
Committed 276651@main (e88f01a): https://commits.webkit.org/276651@main Reviewed commits have been landed. Closing PR #25602 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
e88f01a
be30731
🧪 api-mac🛠 tv🛠 watch🧪 jsc-armv7-tests