[5.10] Android: add better nullability checks for nullability annotations added in NDK 26 (#444) #457
Conversation
…ded in NDK 26 (swiftlang#444) Also fix one test.
finagolfin
requested review from
abertelrud,
neonichu,
tomerd and
MaxDesiatov
as code owners
|
@swift-ci test |
|
MacOS CI failure appears spurious? |
|
@swift-ci test macos |
|
It reproduces, though I'm not sure why it passed on trunk then: Anybody using macOS who can look into this? |
|
@MaxDesiatov, you last modified this filesystem error code in #167, any idea why this would vary between macOS and linux like this? @etcwilde, maybe you have some idea why we're only seeing this failing test on macOS, could be a compiler bug? |
|
@neonichu, any idea why this would only fail on macOS with 5.10 but pass on trunk? |
|
@bnbarham any chance this could be considered for 5.10 at this point? |
|
@swift-ci test macos |
|
It's a confirmed macOS failure at this point. I find it quite strange that it passed on trunk but failed on this 5.10 branch, particularly since |
|
Oh, I see, I suppose #460 is the fix for new versions of macOS? I guess that will need to be merged into 5.10 first then. |
|
@swift-ci test |
| let stdinStream = try LocalFileOutputByteStream(filePointer: fdopen(stdinPipe[1], "wb"), closeOnDeinit: true) | ||
| guard let fp = fdopen(stdinPipe[1], "wb") else { | ||
| throw Process.Error.stdinUnavailable | ||
| } | ||
| let stdinStream = try LocalFileOutputByteStream(filePointer: fp, closeOnDeinit: true) |
There was a problem hiding this comment.
I'd prefer to not change this on 5.10, ie. force unwrap like in the other cases.
There was a problem hiding this comment.
The whole point of adding a guard let here is that it's much safer and actually checks for the optional that the recent version of Bionic passes in. Why go back to a force unwrap then?
There was a problem hiding this comment.
Yeah, I kind of agree with keeping the guard-let here too. If it's about accepting risk, then a force-unwrap would be a worse outcome than an error message.
There was a problem hiding this comment.
The bar for 5.10 is extremely high at this point. If you'd like this change in, it needs to have zero impact outside of fixing the compilation errors. I'm all for the guard on main (and ideally, these would all be actually checks instead of force unwraps).
There was a problem hiding this comment.
I think this passes that bar: the guard-let's are obviously fine, as they only make the code safer. The two remaining force unwraps: one is of a baseAddress access, which should never fail unless someone passes in a nil to that internal function intentionally, and the other is in a test support function.
This patch was merged into the main branch a month ago and we've had zero problems with it: I think the same will be true on 5.10 and it will keep this toolchain compiling on linux with the newest versions of glibc.
There was a problem hiding this comment.
Sorry, I mixed up which thing this branch was targeting. Short of finding something like a security bug or memory issue like that, the changes can't have any impact on macOS at all. Even changing when something throws or aborts is more than what is acceptable for the 5.10 branch at this point. Sorry about that.
There was a problem hiding this comment.
I think this passes that bar too, as these were all raw accesses on macOS before, so if they were going to fail, they would fail regardless. All we're changing on macOS is the way that fails, and I don't think that qualifies as "Even changing when something throws or aborts" as technically these all fail at exactly the same point on macOS after this patch, they just give better errors and fix compilation with glibc and bionic.
| "\(root)/xbin" | ||
| "\(root)/etc" |
There was a problem hiding this comment.
What's the test change for?
There was a problem hiding this comment.
I added this case years ago, back when some Android versions had this directory, matching sbin/ below, but it looks like none do now. Updating this test to another random directory gets it to work again, and this is perfectly safe since it's scoped to just Android.
|
Can somebody make a decision on this pull before the release? I think you are being overly conservative, as this pull only makes the code safer by adding explicit checks, while not really changing much in the case of null pointers, as the current code fails almost immediately with such input anyway. The benefit is that TSC keeps compiling with the latest versions of glibc and bionic, which will keep the 5.10 toolchain building on the officially supported platform of linux, while this change is small and easy to manually review. @ddunbar, you're the 5.10 release manager for this repo, wdyt? Btw, the macOS CI failed only because the 5.10 branch is currently broken, #466. |
|
I simply tried building the latest trunk commit 6bf644243 of swift-driver on Fedora Core 39, that happens to have glibc 2.38 installed, and could not do so, because this dependency would fail to build, as it was automatically checked out to some commit from October. I went into this source checkout and pushed it to latest trunk, which contains this commit, and it built fine. I still think we should get this into 5.10 for the next patch release, as it fixes the 5.10 toolchain build on very popular linux distros and these checks only make this package safer on macOS too. |
|
Ping @shahmishal, let me know what you think. |
|
@swift-ci test macOS |
|
@kateinoigakukun, would you kick off another macOS CI run here? It should work now that the other 5.10 pull was merged. |
|
@swift-ci test macOS |
|
@shahmishal, passed all CI and has been merged in the Swift 6 branches for four months now, without any problems reported. |
Cherrypick of #444
Explanation: These null checks allow this repo to compile against the latest versions of Bionic and glibc, swiftlang/swift#70647, plus this fixes one test for Android.
Scope: small fixes to check or unwrap optionals
Issue: swiftlang/swift#70647
Risk: low, usually safer
Testing: Passed all CI on trunk
Reviewer: @neonichu
I wasn't planning on submitting these Bionic fixes for 5.10, but once I saw that they help with the latest glibc too, I figured we should get it in.