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.

These fts_open() changes were no longer needed after swiftlang/swift#74829, so I removed them in #5010.

…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?

No longer needed after your swiftlang/swift#74829, so removed in #5010.

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?

Good point, removed in #5010, you can add it in a version of #4889 for 6.0.

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?

That's better, dropped this in #5010, you can do that in a version of #4889 for 6.0.

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.

Changing the signature would work too, dropped this in #5010, you can submit it in a version of #4889 for 6.0.

finagolfin · 2024-07-17T18:28:10Z

Now that swift-foundation was merged, this pull no longer applies cleanly to that rewrite. Instead, I have modified or removed portions of this pull based on the review comments here and applied it to the release/6.0 branch in #5010 instead, where swift-foundation will presumably not be merged also.

Once that's in 6.0, I will rework this pull for swift-foundation and make the necessary changes here also. My answers to the review comments above will now address what I changed for #5010.

finagolfin · 2024-08-11T02:46:19Z

Some of these are no longer needed after the re-core and other changes, others have been submitted again in #5024 and my pull for that, hyp#1.

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

Closed

compnerd reviewed Mar 1, 2024

View reviewed changes

finagolfin mentioned this pull request Jul 17, 2024

[6.0] Android: add better nullability checks for nullability annotations added in NDK 26 #5010

Closed

finagolfin closed this Aug 11, 2024

finagolfin deleted the droid branch August 11, 2024 02:50

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 Jul 17, 2024

finagolfin Dec 1, 2023

finagolfin commented Feb 12, 2024

compnerd Mar 1, 2024

finagolfin Jul 17, 2024

compnerd Mar 1, 2024

finagolfin Jul 17, 2024

compnerd Mar 1, 2024

compnerd Mar 1, 2024

finagolfin Jul 17, 2024

compnerd Mar 1, 2024

finagolfin Mar 18, 2024

finagolfin Jul 17, 2024

finagolfin commented Jul 17, 2024

finagolfin commented Aug 11, 2024

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

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

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

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 Jul 17, 2024

finagolfin commented Aug 11, 2024