Skip to content

Commit fc58be2

Browse files
madsmtmmadsmtm
authored
Merge pull request #792 from madsmtm/metal-safe
Automatically mark `objc2-metal` as safe
2 parents ad2767b + 51ca607 commit fc58be2

File tree

9 files changed

+409
-844
lines changed

9 files changed

+409
-844
lines changed

‎crates/header-translator/src/method.rs‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -603,7 +603,10 @@ impl Method {
603603
let mut safety = arguments
604604
.iter()
605605
.fold(SafetyProperty::Safe, |mut safety, (arg_name, arg_ty)| {
606-
if default_safety.not_bounds_affecting && is_likely_bounds_affecting(arg_name) {
606+
if default_safety.not_bounds_affecting
607+
&& is_likely_bounds_affecting(arg_name)
608+
&& arg_ty.can_affect_bounds()
609+
{
607610
any_argument_bounds_affecting = true;
608611
safety = safety.merge(SafetyProperty::new_unknown(format!(
609612
"`{arg_name}` might not be bounds-checked"
@@ -613,9 +616,13 @@ impl Method {
613616
})
614617
.merge(result_type.safety_in_fn_return());
615618

619+
// Probably overly conservative
616620
if default_safety.not_bounds_affecting
617621
&& !any_argument_bounds_affecting
618622
&& is_likely_bounds_affecting(&selector)
623+
&& arguments
624+
.iter()
625+
.any(|(_, arg_ty)| arg_ty.can_affect_bounds())
619626
{
620627
safety = safety.merge(SafetyProperty::new_unknown(
621628
"This might not be bounds-checked",
@@ -888,7 +895,10 @@ impl Method {
888895
safety =
889896
safety.merge(SafetyProperty::new_unknown("This might not be thread-safe"));
890897
};
891-
if default_safety.not_bounds_affecting && is_likely_bounds_affecting(&selector) {
898+
if default_safety.not_bounds_affecting
899+
&& is_likely_bounds_affecting(&selector)
900+
&& ty.can_affect_bounds()
901+
{
892902
safety = safety.merge(SafetyProperty::new_unknown(
893903
"This might not be bounds-checked",
894904
));

‎crates/header-translator/src/name_translation.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,10 @@ pub(crate) fn is_likely_bounds_affecting(name: &str) -> bool {
459459
|| name.contains("range")
460460
|| name.contains("offset")
461461
|| name.contains("count")
462+
|| name.contains("stride")
463+
|| name.contains("size")
464+
// Probably not necessary?
465+
// || name.contains("length")
462466
}
463467

464468
fn lowercase_words(s: &str) -> impl Iterator<Item = String> + '_ {

‎crates/header-translator/src/rust_type.rs‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1138,6 +1138,72 @@ impl PointeeTy {
11381138
{
11391139
TypeSafety::unknown_in_argument("should be of the correct type")
11401140
}
1141+
// Passing `MTLFunction` is spiritually similar to passing an
1142+
// `unsafe` function pointer; we can't know without inspecting
1143+
// the function (or it's documentation) whether it has special
1144+
// safety requirements. Example:
1145+
//
1146+
// ```metal
1147+
// constant float data[5] = { 1.0, 2.0, 3.0, 4.0, 5.0 };
1148+
//
1149+
// // Safety: Must not be called with an index < 5.
1150+
// kernel void add_static(
1151+
// device const float* input,
1152+
// device float* result,
1153+
// uint index [[thread_position_in_grid]]
1154+
// ) {
1155+
// if (5 <= index) {
1156+
// // For illustration purposes.
1157+
// __builtin_unreachable();
1158+
// }
1159+
// result[index] = input[index] + data[index];
1160+
// }
1161+
// ```
1162+
[(protocol, _)]
1163+
if protocol.is_subprotocol_of("MTLFunction")
1164+
|| protocol.is_subprotocol_of("MTLFunctionHandle") =>
1165+
{
1166+
TypeSafety::unknown_in_argument("must be safe to call").merge(
1167+
TypeSafety::unknown_in_argument(
1168+
"must have the correct argument and return types",
1169+
),
1170+
)
1171+
}
1172+
// Access to the contents of a resource has to be manually
1173+
// synchronized using things like `didModifyRange:` (CPU side)
1174+
// or `synchronizeResource:`, `useResource:usage:` and
1175+
// `MTLFence` (GPU side).
1176+
[(protocol, _)] if protocol.is_subprotocol_of("MTLResource") => {
1177+
let safety = TypeSafety::unknown_in_argument("may need to be synchronized");
1178+
1179+
// Additionally, resources in a command buffer must be
1180+
// kept alive by the application for as long as they're
1181+
// used. If this is not done, it is possible to encounter
1182+
// use-after-frees with:
1183+
// - `MTLCommandBufferDescriptor::setRetainedReferences(false)`.
1184+
// - `MTLCommandQueue::commandBufferWithUnretainedReferences()`.
1185+
// - All `MTL4CommandBuffer`s.
1186+
let safety = safety.merge(TypeSafety::unknown_in_argument(
1187+
"may be unretained, you must ensure it is kept alive while in use",
1188+
));
1189+
1190+
// TODO: Should we also document the requirement for
1191+
// resources to be properly bound? What exactly are the
1192+
// requirements though, and when does Metal automatically
1193+
// bind resources?
1194+
1195+
// `MTLBuffer` is effectively a `Box<[u8]>` stored on the
1196+
// GPU (and depending on the storage mode, optionally also
1197+
// on the CPU). Type-safety of the contents is left
1198+
// completely up to the user.
1199+
if protocol.id.name == "MTLBuffer" {
1200+
safety.merge(TypeSafety::unknown_in_argument(
1201+
"contents should be of the correct type",
1202+
))
1203+
} else {
1204+
safety
1205+
}
1206+
}
11411207
// Other `ProtocolObject<dyn MyProtocol>`s are treated as
11421208
// proper types. (An example here is delegate protocols).
11431209
[_] => TypeSafety::SAFE,
@@ -3982,6 +4048,43 @@ impl Ty {
39824048
}
39834049
}
39844050

4051+
/// Whether the type could in theory affect the bounds of the receiver.
4052+
///
4053+
/// This is meant to catch `NSInteger`, `NSRange`, `MTL4BufferRange`, `MTLGPUAddress` and
4054+
/// similar constructs.
4055+
pub(crate) fn can_affect_bounds(&self) -> bool {
4056+
match self.through_typedef() {
4057+
Self::Pointer { pointee, .. } | Self::IncompleteArray { pointee, .. } => {
4058+
pointee.can_affect_bounds()
4059+
}
4060+
Self::Array { element_type, .. } => element_type.can_affect_bounds(),
4061+
Self::Primitive(prim) | Self::Simd { ty: prim, .. } => matches!(
4062+
prim,
4063+
// 32-bit and 64-bit integers.
4064+
Primitive::I32
4065+
| Primitive::I64
4066+
| Primitive::Int
4067+
| Primitive::Long
4068+
| Primitive::ISize
4069+
| Primitive::NSInteger
4070+
| Primitive::U32
4071+
| Primitive::U64
4072+
| Primitive::UInt
4073+
| Primitive::ULong
4074+
| Primitive::USize
4075+
| Primitive::NSUInteger
4076+
| Primitive::PtrDiff
4077+
),
4078+
Self::Struct { fields, .. } | Self::Union { fields, .. } => {
4079+
fields.iter().any(|field| field.can_affect_bounds())
4080+
}
4081+
// Enumerations are intentionally not bounds-affecting (e.g. not
4082+
// `MTLIndexType`).
4083+
Self::Pointee(_) | Self::Enum { .. } | Self::Sel { .. } => false,
4084+
Self::TypeDef { .. } => unreachable!("using through_typedef"),
4085+
}
4086+
}
4087+
39854088
fn into_pointee(self) -> Option<PointeeTy> {
39864089
match self {
39874090
Self::Pointee(pointee) => Some(pointee),

‎crates/header-translator/src/stmt.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1864,6 +1864,7 @@ impl Stmt {
18641864
.fold(SafetyProperty::Safe, |mut safety, (arg_name, arg_ty)| {
18651865
if default_safety.not_bounds_affecting
18661866
&& is_likely_bounds_affecting(arg_name)
1867+
&& arg_ty.can_affect_bounds()
18671868
{
18681869
any_argument_bounds_affecting = true;
18691870
safety = safety.merge(SafetyProperty::new_unknown(format!(
@@ -1877,6 +1878,9 @@ impl Stmt {
18771878
if default_safety.not_bounds_affecting
18781879
&& !any_argument_bounds_affecting
18791880
&& is_likely_bounds_affecting(&c_name)
1881+
&& arguments
1882+
.iter()
1883+
.any(|(_, arg_ty)| arg_ty.can_affect_bounds())
18801884
{
18811885
safety =
18821886
safety.merge(SafetyProperty::new_unknown("Might not be bounds-checked"));

‎crates/objc2/src/topics/FRAMEWORKS_CHANGELOG.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
1616
* Added `IOKit` "serial" submodule.
1717
* Marked a bunch of functions safe in:
1818
- `AppKit` / `objc2-app-kit`.
19-
- `CoreGraphics` / `objc2-core-graphics`.
2019
- `CoreFoundation` / `objc2-core-foundation`.
20+
- `CoreGraphics` / `objc2-core-graphics`.
2121
- `CoreVideo` / `objc2-core-video`.
2222
- `Foundation` / `objc2-foundation`.
2323
- `IOKit` / `objc2-io-kit`.
24+
- `Metal` / `objc2-metal`.
2425
- `QuartzCore` / `objc2-quartz-core`.
2526
- `UIKit` / `objc2-ui-kit`.
2627
- `UniformTypeIdentifiers` / `objc2-uniform-type-identifiers`.

‎examples/metal/default_xcode_game/renderer.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ fn load_pipeline_state(
167167
}
168168

169169
fn load_vertex_descriptor() -> Retained<MTLVertexDescriptor> {
170-
let vertex_descriptor = unsafe { MTLVertexDescriptor::new() };
170+
let vertex_descriptor = MTLVertexDescriptor::new();
171171

172172
unsafe {
173173
let attributes = vertex_descriptor.attributes();
@@ -201,7 +201,7 @@ fn load_vertex_descriptor() -> Retained<MTLVertexDescriptor> {
201201
fn load_depth_state(
202202
device: &ProtocolObject<dyn MTLDevice>,
203203
) -> Retained<ProtocolObject<dyn MTLDepthStencilState>> {
204-
let depth_state_desc = unsafe { MTLDepthStencilDescriptor::new() };
204+
let depth_state_desc = MTLDepthStencilDescriptor::new();
205205
depth_state_desc.setDepthCompareFunction(MTLCompareFunction::Less);
206206
depth_state_desc.setDepthWriteEnabled(true);
207207
device

‎framework-crates/objc2-metal/src/lib.rs‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,58 @@
2323
not(feature = "MTLDevice"),
2424
doc = "[`MTLCreateSystemDefaultDevice`]: #needs-MTLDevice-feature"
2525
)]
26+
//!
27+
//! # Safety considerations
28+
//!
29+
//! Metal allows running arbitrary code on the GPU. We treat memory safety
30+
//! issues on the GPU as just as unsafe as that which applies to the CPU. A
31+
//! few notes on this below.
32+
//!
33+
//! ## Shaders
34+
//!
35+
//! Shaders are (often) written in an unsafe C-like language.
36+
//!
37+
//! Loading them (via `MTLLibrary`, function stitching etc.) is perfectly
38+
//! safe, it is similar to dynamic linking. The restrictions that e.g.
39+
//! `libloading::Library::new` labours under do not apply, since there are no
40+
//! ctors in [the Metal Shading Language][msl-spec] (see section 4.2).
41+
//!
42+
//! Similarly, getting individual shaders (`MTLFunction`) is safe, we can
43+
//! model this as the same as calling `dlsym` (which just returns a pointer).
44+
//!
45+
//! _Calling_ functions though, is not safe. Even though they can have their
46+
//! parameter and return types checked at runtime, they may have additional
47+
//! restrictions not present in the signature (e.g. `__builtin_unreachable()`
48+
//! is possible in MSL, so is out-of-bounds accesses). If you view
49+
//! `MTLFunction` as essentially just an `unsafe fn()` pointer, this should be
50+
//! apparent.
51+
//!
52+
//! [msl-spec]: https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf
53+
//!
54+
//! ## Bounds checks
55+
//!
56+
//! It is yet unclear whether Metal APIs are bounds-checked on the CPU side or
57+
//! not, so APIs that take offsets / lengths are often unsafe.
58+
//!
59+
//! ## Synchronization
60+
//!
61+
//! `MTLResource` subclasses such as `MTLBuffer` and `MTLTexture` require
62+
//! synchronization between the CPU and the GPU, or between different threads
63+
//! on the GPU itself, so APIs taking these are often unsafe.
64+
//!
65+
//! ## Memory management and lifetimes
66+
//!
67+
//! Resources used in `MTL4CommandBuffer`s or command buffers with created
68+
//! with one of:
69+
//! - `MTLCommandBufferDescriptor::setRetainedReferences(false)`.
70+
//! - `MTLCommandQueue::commandBufferWithUnretainedReferences()`.
71+
//!
72+
//! Must be kept alive for as long as they're used.
73+
//!
74+
//! ## Type safety
75+
//!
76+
//! `MTLBuffer` is untyped (in a similar manner as a `[u8]` slice), you must
77+
//! ensure that any usage of it is done with valid types.
2678
#![recursion_limit = "256"]
2779
#![allow(non_snake_case)]
2880
#![no_std]

0 commit comments

Comments
 (0)