[android] fix the android build #999

hyp · 2024-10-23T20:07:05Z

Platform.swift regressed after 71eefee

hyp · 2024-10-23T20:07:15Z

hyp · 2024-10-23T21:30:52Z

build failures without the fix:

:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:168:35: error: cannot find 'group' in scope
166 |     
167 |     static func gid(forName name: String) -> uid_t? {
168 |         withUserGroupBuffer(name, group(), sizeProperty: Int32(_SC_GETGR_R_SIZE_MAX), operation: getgrnam_r) {
    |                                   `- error: cannot find 'group' in scope
169 |             $0.gr_gid
170 |         }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:168:98: error: cannot find 'getgrnam_r' in scope
166 |     
167 |     static func gid(forName name: String) -> uid_t? {
168 |         withUserGroupBuffer(name, group(), sizeProperty: Int32(_SC_GETGR_R_SIZE_MAX), operation: getgrnam_r) {
    |                                                                                                  `- error: cannot find 'getgrnam_r' in scope
169 |             $0.gr_gid
170 |         }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:168:98: error: converting non-escaping value to '(String, UnsafeMutablePointer<Output>, UnsafeMutablePointer<CChar>, Int, UnsafeMutablePointer<UnsafeMutablePointer<Output>?>) -> Int32' (aka '(String, UnsafeMutablePointer<Output>, UnsafeMutablePointer<Int8>, Int, UnsafeMutablePointer<Optional<UnsafeMutablePointer<Output>>>) -> Int32') may allow it to escape
166 |     
167 |     static func gid(forName name: String) -> uid_t? {
168 |         withUserGroupBuffer(name, group(), sizeProperty: Int32(_SC_GETGR_R_SIZE_MAX), operation: getgrnam_r) {
    |                                                                                                  `- error: converting non-escaping value to '(String, UnsafeMutablePointer<Output>, UnsafeMutablePointer<CChar>, Int, UnsafeMutablePointer<UnsafeMutablePointer<Output>?>) -> Int32' (aka '(String, UnsafeMutablePointer<Output>, UnsafeMutablePointer<Int8>, Int, UnsafeMutablePointer<Optional<UnsafeMutablePointer<Output>>>) -> Int32') may allow it to escape
169 |             $0.gr_gid
170 |         }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:175:32: error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')
173 |     static func name(forUID uid: uid_t) -> String? {
174 |         withUserGroupBuffer(uid, passwd(), sizeProperty: Int32(_SC_GETPW_R_SIZE_MAX), operation: getpwuid_r) {
175 |             String(cString: $0.pw_name)
    |                                |- error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')
    |                                |- note: coalesce using '??' to provide a default when the optional value contains 'nil'
    |                                `- note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
176 |         }
177 |     }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:181:32: error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')
179 |     static func fullName(forUID uid: uid_t) -> String? {
180 |         withUserGroupBuffer(uid, passwd(), sizeProperty: Int32(_SC_GETPW_R_SIZE_MAX), operation: getpwuid_r) {
181 |             String(cString: $0.pw_gecos)
    |                                |- error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')
    |                                |- note: coalesce using '??' to provide a default when the optional value contains 'nil'
    |                                `- note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
182 |         }
183 |     }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:186:34: error: cannot find 'group' in scope
184 |     
185 |     static func name(forGID gid: gid_t) -> String? {
186 |         withUserGroupBuffer(gid, group(), sizeProperty: Int32(_SC_GETGR_R_SIZE_MAX), operation: getgrgid_r) {
    |                                  `- error: cannot find 'group' in scope
187 |             String(cString: $0.gr_name)
188 |         }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:186:97: error: cannot find 'getgrgid_r' in scope
184 |     
185 |     static func name(forGID gid: gid_t) -> String? {
186 |         withUserGroupBuffer(gid, group(), sizeProperty: Int32(_SC_GETGR_R_SIZE_MAX), operation: getgrgid_r) {
    |                                                                                                 `- error: cannot find 'getgrgid_r' in scope
187 |             String(cString: $0.gr_name)
188 |         }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:186:97: error: converting non-escaping value to '(gid_t, UnsafeMutablePointer<Output>, UnsafeMutablePointer<CChar>, Int, UnsafeMutablePointer<UnsafeMutablePointer<Output>?>) -> Int32' (aka '(UInt32, UnsafeMutablePointer<Output>, UnsafeMutablePointer<Int8>, Int, UnsafeMutablePointer<Optional<UnsafeMutablePointer<Output>>>) -> Int32') may allow it to escape
184 |     
185 |     static func name(forGID gid: gid_t) -> String? {
186 |         withUserGroupBuffer(gid, group(), sizeProperty: Int32(_SC_GETGR_R_SIZE_MAX), operation: getgrgid_r) {
    |                                                                                                 `- error: converting non-escaping value to '(gid_t, UnsafeMutablePointer<Output>, UnsafeMutablePointer<CChar>, Int, UnsafeMutablePointer<UnsafeMutablePointer<Output>?>) -> Int32' (aka '(UInt32, UnsafeMutablePointer<Output>, UnsafeMutablePointer<Int8>, Int, UnsafeMutablePointer<Optional<UnsafeMutablePointer<Output>>>) -> Int32') may allow it to escape
187 |             String(cString: $0.gr_name)
188 |         }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:193:32: error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')
191 |     static func homeDirectory(forUserName userName: String) -> String? {
192 |         withUserGroupBuffer(userName, passwd(), sizeProperty: Int32(_SC_GETPW_R_SIZE_MAX), operation: getpwnam_r) {
193 |             String(cString: $0.pw_dir)
    |                                |- error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')
    |                                |- note: coalesce using '??' to provide a default when the optional value contains 'nil'
    |                                `- note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
194 |         }
195 |     }

D:/r/_work/swift-build/swift-build/SourceCache/swift-foundation/Sources/FoundationEssentials/Platform.swift:199:32: error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')
197 |     static func homeDirectory(forUID uid: uid_t) -> String? {
198 |         withUserGroupBuffer(uid, passwd(), sizeProperty: Int32(_SC_GETPW_R_SIZE_MAX), operation: getpwuid_r) {
199 |             String(cString: $0.pw_dir)
    |                                |- error: value of optional type 'UnsafeMutablePointer<CChar>?' (aka 'Optional<UnsafeMutablePointer<Int8>>') must be unwrapped to a value of type 'UnsafeMutablePointer<CChar>' (aka 'UnsafeMutablePointer<Int8>')

    |                                |- note: coalesce using '??' to provide a default when the optional value contains 'nil'

    |                                `- note: force-unwrap using '!' to abort execution if the optional value contains 'nil'

200 |         }

201 |     }

[571/577] Building CXX object _deps/swiftfoundationicu-build/icuSources/CMakeFiles/_FoundationICU.dir/common/icu_packaged_data.cpp.o
ninja: build stopped: subcommand failed.

hyp · 2024-10-23T21:31:38Z

we need to import full Android module to import sub-module grp to access group APIs, and also the const char * properties of users/groups are nullable on Android.

jmschonfeld · 2024-10-23T21:36:46Z

Sources/FoundationEssentials/Platform.swift

@@ -172,31 +171,46 @@ extension Platform {

    static func name(forUID uid: uid_t) -> String? {
        withUserGroupBuffer(uid, passwd(), sizeProperty: Int32(_SC_GETPW_R_SIZE_MAX), operation: getpwuid_r) {
-            String(cString: $0.pw_name)
+            // Android's pw_name `char *`` is nullable, so always coerce to a nullable pointer


Just to confirm: is this property nullable on Android because it has different semantics than the Darwin/Glibc definition (i.e. it is valid to be null whereas it's not valid on Darwin/Linux)? Or is this a potential issue with the Android module/swift overlay that we could address there to make it non-optional like Darwin/Linux?

If the difference is intentional then this seems like the best way to account for it, just want to make sure we shouldn't try to make this change to the Android module before we add complexity to this code that we don't have much unit testing for

Good question! I looked over the sources for Android's libc and it looks like the situation is mixed:

pw_name doesn't seem to be nullable , so I think it should be _Nonnull instead.

pw_gecos is typically null, so it's correct for it to be _Nullable.

gr_name doesn't seem to be nullable, so I think it should be _Nonnull instead.

pw_dir doesn't seem to be nullable, so I think it should be _Nonnul linstead.

I'll update the patch to only account for nullability in pw_gecos in that case, and will try to re-annotate the other fields using API notes instead.

Also pw_gecos isn't available on 32 bit android, as it's a macro in headers, so I'll handle that case too.

the trouble with fields like this is "when are making your claim?". even if one particular function guarantees the field won't be null, other code can pass you an instance of this struct with a null field. there's no prohibition against user code creating/mutating instances of any libc type (nor could there be, in C).

tbh, i don't think there's any particularly satisfactory answer to this question. afaik no other libc has tried adding annotations, so we can't even compare notes to be "at least consistently wrong"...

i'm listening, but this feels "WAI" to me, given that insufficient expressivity of the existing annotations.

(and if you think this is bad, check out the places where we've shrugged and said __BIONIC_COMPLICATED_NULLNESS where the answer is "it depends on the values of other arguments" :-( )

hyp · 2024-10-24T20:14:56Z

@jmschonfeld unfortunately, it looks like Clang's API Notes do not support changing the nullability of a field in a C struct :( , and I would be unable to use API notes to fix the nullability for fields I mentioned above. I'm going to file an issue on the NDK so that they can fix it in source, but in the mean time, do you think it would be ok to land this instead, with fixed up comments that mention that we're waiting on the NDK fix?

jmschonfeld · 2024-10-25T17:12:06Z

Sources/FoundationEssentials/Platform.swift

        }
    }

    static func fullName(forUID uid: uid_t) -> String? {
        withUserGroupBuffer(uid, passwd(), sizeProperty: Int32(_SC_GETPW_R_SIZE_MAX), operation: getpwuid_r) {
-            String(cString: $0.pw_gecos)
+#if os(Android) && (arch(i386) || arch(arm))


Small comment: should we use _pointerBitWidth(_32) here just so we don't have to maintain a list of 32bit architectures if this ever changes?

jmschonfeld · 2024-10-25T17:13:52Z

@jmschonfeld unfortunately, it looks like Clang's API Notes do not support changing the nullability of a field in a C struct :( , and I would be unable to use API notes to fix the nullability for fields I mentioned above. I'm going to file an issue on the NDK so that they can fix it in source, but in the mean time, do you think it would be ok to land this instead, with fixed up comments that mention that we're waiting on the NDK fix?

Unfortunate but yeah this is probably the best resolution to keep Android building in the meantime. Left one small comment but otherwise this is looking good to me based on that.

Platform.swift regressed after 71eefee

hyp · 2024-10-29T18:38:17Z

@jmschonfeld Updated the PR as requested, and linked to the NDK issue.

hyp · 2024-10-29T18:38:23Z

@swift-ci please test

jmschonfeld

Awesome, thanks!

Platform.swift regressed after 71eefee

hyp requested review from compnerd and jmschonfeld October 23, 2024 20:07

jmschonfeld reviewed Oct 23, 2024

View reviewed changes

hyp force-pushed the eng/fix-android-platform branch from d504ec7 to dfc3d9d Compare October 24, 2024 20:13

jmschonfeld reviewed Oct 25, 2024

View reviewed changes

hyp mentioned this pull request Oct 29, 2024

[BUG] r27c NDK: incorrect nullability on fields in group and passwd structs android/ndk#2098

Closed

[android] fix the android build

dacaf23

Platform.swift regressed after 71eefee

hyp force-pushed the eng/fix-android-platform branch from dfc3d9d to dacaf23 Compare October 29, 2024 18:37

jmschonfeld approved these changes Oct 29, 2024

View reviewed changes

hyp merged commit ab00100 into swiftlang:main Oct 29, 2024
3 checks passed

cthielen pushed a commit to cthielen/swift-foundation that referenced this pull request Nov 8, 2024

[android] fix the android build (swiftlang#999)

308c112

Platform.swift regressed after 71eefee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[android] fix the android build #999

[android] fix the android build #999

Uh oh!

hyp commented Oct 23, 2024

Uh oh!

hyp commented Oct 23, 2024

Uh oh!

hyp commented Oct 23, 2024

Uh oh!

hyp commented Oct 23, 2024

Uh oh!

jmschonfeld Oct 23, 2024

Uh oh!

hyp Oct 23, 2024

Uh oh!

hyp Oct 23, 2024

Uh oh!

enh-google Oct 29, 2024

Uh oh!

hyp commented Oct 24, 2024

Uh oh!

jmschonfeld Oct 25, 2024

Uh oh!

jmschonfeld commented Oct 25, 2024

Uh oh!

hyp commented Oct 29, 2024

Uh oh!

hyp commented Oct 29, 2024

Uh oh!

jmschonfeld left a comment

Uh oh!

Uh oh!

Uh oh!

[android] fix the android build #999

[android] fix the android build #999

Uh oh!

Conversation

hyp commented Oct 23, 2024

Uh oh!

hyp commented Oct 23, 2024

Uh oh!

hyp commented Oct 23, 2024

Uh oh!

hyp commented Oct 23, 2024

Uh oh!

jmschonfeld Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

hyp Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

hyp Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

enh-google Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

hyp commented Oct 24, 2024

Uh oh!

jmschonfeld Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

jmschonfeld commented Oct 25, 2024

Uh oh!

hyp commented Oct 29, 2024

Uh oh!

hyp commented Oct 29, 2024

Uh oh!

jmschonfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!