-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[StrictMemorySafety] Add unsafe annotations to OpenBSDImpl. #86616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@swift-ci please test. |
|
Can we use |
7c47059 to
68ee5d5
Compare
Good catch. Updated; I think I've done so appropriately. |
|
I'd use |
In swiftlang#86276 strict memory safety becomes an error, necessitating adding the following annotations and changes: * in init(): check the return value of `pthread_mutex_init`; since this isn't a failable initializer, `fatalError` if this fails, to avoid silently leaving a zero-valued pointer, * similarly check return values in `pthread_mutex_lock`, `pthread_mutex_unlock`, and `pthread_mutex_destroy`, * the message in `fatalError` should use `strerror_r`, which necessitates `unsafe` markings since it initializes the array storage unsafely. * marking the `value` as `@safe` since the underlying mutex type is an `OpaquePointer` and is unsafe and all access to value occur through a safe interface, * thus, marking `_MutexHandle` `@safe`.
68ee5d5 to
6e31926
Compare
Done. That certainly looks more idomatic now. |
grynspan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not quite holding the Array API right as-is. How about this instead? It avoids an intermediate heap allocation for the buffer and it uses a fallback string that still vends the error code.
Co-authored-by: Jonathan Grynspan <[email protected]>
|
(Oh -- I just noticed I was using Array's withUnsafeTemporaryAllocation instead of String's like you originally suggested. 🤦♀️) |
|
@swift-ci please test. |
|
|
||
| @usableFromInline | ||
| @_transparent | ||
| func errstr(_ no: Int32) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the OpenBSD implementation call strerror() elsewhere? Not for this PR, but would be worth auditing the codebase and replacing such calls with something like this function in the future. (Same for FreeBSD.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I'm aware of, but making this more accessible is probably a good idea...
...as well as adding other affordances for interacting with libc, which again, is another good fit with the import C pitch...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah 🥲
|
@swift-ci please test. |
In #86276 strict memory safety becomes an error, necessitating adding the following annotations and changes:
in init(): check the return value of
pthread_mutex_init; since this isn't a failable initializer,fatalErrorif this fails, to avoid silently leaving a zero-valued pointer,similarly check return values in
pthread_mutex_lock,pthread_mutex_unlock, andpthread_mutex_destroy,the message in
fatalErrorshould usestrerror, which necessitatesunsafemarkings since despitestrerrornever returning a null pointer.marking the
valueas@safesince the underlying mutex type is anOpaquePointerand is unsafe and all access to value occur through a safe interface,thus, marking
_MutexHandle@safe.