Android: add better nullability checks for nullability annotations added in NDK 26 #4850

finagolfin · 2023-12-01T14:07:49Z

This is needed because Bionic recently added a bunch of these annotations. I made sure this pull doesn't break anything by testing most of it with the previous NDK 25c also. I used this patch with others to build the Swift toolchain for my Android CI, finagolfin/swift-android-sdk#122, and the Termux app for Android, which now uses NDK 26b.

@drodriguez or @compnerd, please review.

finagolfin · 2023-12-01T14:38:04Z

Sources/Foundation/FileHandle.swift

+ return NSData.NSDataReadResult(bytes: data, length: mapSize) { buffer, length in
+ munmap(buffer, length)
+ }
+ #else
 return NSData.NSDataReadResult(bytes: data!, length: mapSize) { buffer, length in


As shown in my Bionic link above, mmap() now returns _Nonnull, so this force-unwrap fails.

It feels rather unfortunate to ifdef around this here instead of at the place we call mmap.

parkera · 2023-12-01T19:53:14Z

@swift-ci test

parkera · 2023-12-01T19:53:55Z

Sources/Foundation/FileHandle.swift

+ return NSData.NSDataReadResult(bytes: data, length: mapSize) { buffer, length in
+ munmap(buffer, length)
+ }
+ #else
 return NSData.NSDataReadResult(bytes: data!, length: mapSize) { buffer, length in


It feels rather unfortunate to ifdef around this here instead of at the place we call mmap.

parkera · 2023-12-01T19:55:09Z

Sources/Foundation/FileManager+POSIX.swift

@@ -1085,10 +1091,18 @@ extension FileManager {
 do {
 guard fm.fileExists(atPath: _url.path) else { throw _NSErrorWithErrno(ENOENT, reading: true, url: url) }
 _stream = try FileManager.default._fileSystemRepresentation(withPath: _url.path) { fsRep in
+ #if os(Android)


Similarly here, can we do this by converting the type somehow at the call to fts_open instead of around all uses of the type before that point?

Maybe there is, I just don't know how to do it. This is simply the best I came up with after seeing these Swift type-casting issues for the first time, and I wanted to ask if there was a better way.

@glbrntt, you recently added similar code to NIO, wdyt?

NIO wraps all of the syscalls and does any platform-specific shenanigans within the wrapper.

OK, but that will still require adapting these types for nullability annotations: see my recent patch of your new NIO code for Android.

re: your patch, does using an implicitly unwrapped optional work for both cases? e.g. [UnsafeMutablePointer<CInterop.PlatformChar>!]

I get the following error with Swift 5.9.2 on Android AArch64 if I add that to the type signature of libc_fts_open():

swift-nio/Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift:407:14: error: using '!' is not allowed here; perhaps '?' was intended? _ path: [UnsafeMutablePointer<CInterop.PlatformChar>!], ^ ~ ? error: fatalError

@glbrntt, is that type what you had in mind, or simply unwrapping the arguments when they are passed in inside ftsOpen()?

I think that I approached it similarly to what was being suggested - rebinding the memory around the call site.

…ded in NDK 26

finagolfin · 2023-12-01T21:41:40Z

Sources/Foundation/FileHandle.swift

@@ -307,10 +307,15 @@ open class FileHandle : NSObject {
 if options.contains(.alwaysMapped) {
 // Filesizes are often 64bit even on 32bit systems
 let mapSize = min(length, Int(clamping: statbuf.st_size))
+ #if os(Android)
+ // Bionic mmap() now returns _Nonnull, so the force unwrap below isn't needed.


@parkera, is this the change you had in mind?

finagolfin · 2024-02-12T14:22:30Z

@parkera, any further feedback here?

compnerd · 2024-03-01T22:32:37Z

Sources/Foundation/FileManager+POSIX.swift

@@ -741,32 +741,38 @@ extension FileManager {
 if rmdir(fsRep) == 0 {
 return
 } else if errno == ENOTEMPTY {
+ #if os(Android)
+ let ps = UnsafeMutablePointer<UnsafeMutablePointer<Int8>>.allocate(capacity: 2)


Do we really need to duplicate this or can we get away with a rebinding of the memory?

compnerd · 2024-03-01T22:32:49Z

Sources/Foundation/FileManager+POSIX.swift

 let stream = fts_open(ps, FTS_PHYSICAL | FTS_XDEV | FTS_NOCHDIR | FTS_NOSTAT, nil)
 ps.deinitialize(count: 2)
 ps.deallocate()

- if stream != nil {
+ if let openStream = stream {


Why not simply if let stream?

compnerd · 2024-03-01T22:33:44Z

Sources/Foundation/FileManager+POSIX.swift

@@ -1085,10 +1091,18 @@ extension FileManager {
 do {
 guard fm.fileExists(atPath: _url.path) else { throw _NSErrorWithErrno(ENOENT, reading: true, url: url) }
 _stream = try FileManager.default._fileSystemRepresentation(withPath: _url.path) { fsRep in
+ #if os(Android)


I think that I approached it similarly to what was being suggested - rebinding the memory around the call site.

compnerd · 2024-03-01T22:34:23Z

Sources/Foundation/FileManager+POSIX.swift

@@ -1315,7 +1329,7 @@ extension FileManager {
 let finalErrno = originalItemURL.withUnsafeFileSystemRepresentation { (originalFS) -> Int32? in
 return newItemURL.withUnsafeFileSystemRepresentation { (newItemFS) -> Int32? in
 // This is an atomic operation in many OSes, but is not guaranteed to be atomic by the standard.
- if rename(newItemFS, originalFS) == 0 {
+ if let newFS = newItemFS, let origFS = originalFS, rename(newFS, origFS) == 0 {


Why not simply force unwrap?

compnerd · 2024-03-01T22:34:47Z

Sources/Foundation/Host.swift

@@ -25,7 +25,8 @@ import WinSDK

 // getnameinfo uses size_t for its 4th and 6th arguments.
 private func getnameinfo(_ addr: UnsafePointer<sockaddr>?, _ addrlen: socklen_t, _ host: UnsafeMutablePointer<Int8>?, _ hostlen: socklen_t, _ serv: UnsafeMutablePointer<Int8>?, _ servlen: socklen_t, _ flags: Int32) -> Int32 {
- return Glibc.getnameinfo(addr, addrlen, host, Int(hostlen), serv, Int(servlen), flags)
+ guard let saddr = addr else { return -1 }


I think that I prefer my approach of changing the signature of the private func.

Sorry about the delay, been meaning to get to this, hopefully this week.

finagolfin commented Dec 1, 2023

View reviewed changes

parkera reviewed Dec 1, 2023

View reviewed changes

Android: add better nullability checks for nullability annotations ad…

608fff3

…ded in NDK 26

finagolfin force-pushed the droid branch from cd1e20d to 608fff3 Compare December 1, 2023 21:40

finagolfin commented Dec 1, 2023

View reviewed changes

finagolfin mentioned this pull request Mar 1, 2024

Foundation: repair the build for Android API level 28+ #4889

Open

compnerd reviewed Mar 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: add better nullability checks for nullability annotations added in NDK 26 #4850

Android: add better nullability checks for nullability annotations added in NDK 26 #4850

finagolfin commented Dec 1, 2023

finagolfin Dec 1, 2023

parkera Dec 1, 2023

parkera commented Dec 1, 2023

parkera Dec 1, 2023

parkera Dec 1, 2023

finagolfin Jan 21, 2024

glbrntt Jan 22, 2024

finagolfin Jan 22, 2024

glbrntt Jan 22, 2024

finagolfin Jan 22, 2024

finagolfin Jan 26, 2024

compnerd Mar 1, 2024

finagolfin Dec 1, 2023

finagolfin commented Feb 12, 2024

compnerd Mar 1, 2024

compnerd Mar 1, 2024

compnerd Mar 1, 2024

compnerd Mar 1, 2024

compnerd Mar 1, 2024

finagolfin Mar 18, 2024

Android: add better nullability checks for nullability annotations added in NDK 26 #4850

Are you sure you want to change the base?

Android: add better nullability checks for nullability annotations added in NDK 26 #4850

Conversation

finagolfin commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkera commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment