Automatically mark objc2-metal as safe

crates/header-translator/src/method.rs

@@ -603,7 +603,10 @@ impl Method {
         let mut safety = arguments
             .iter()
             .fold(SafetyProperty::Safe, |mut safety, (arg_name, arg_ty)| {
-                if default_safety.not_bounds_affecting && is_likely_bounds_affecting(arg_name) {
+                if default_safety.not_bounds_affecting
+                    && is_likely_bounds_affecting(arg_name)
+                    && arg_ty.can_affect_bounds()
+                {
                     any_argument_bounds_affecting = true;
                     safety = safety.merge(SafetyProperty::new_unknown(format!(
                         "`{arg_name}` might not be bounds-checked"
@@ -613,9 +616,13 @@ impl Method {
             })
             .merge(result_type.safety_in_fn_return());
 
+        // Probably overly conservative
         if default_safety.not_bounds_affecting
             && !any_argument_bounds_affecting
             && is_likely_bounds_affecting(&selector)
+            && arguments
+                .iter()
+                .any(|(_, arg_ty)| arg_ty.can_affect_bounds())
         {
             safety = safety.merge(SafetyProperty::new_unknown(
                 "This might not be bounds-checked",
@@ -888,7 +895,10 @@ impl Method {
                     safety =
                         safety.merge(SafetyProperty::new_unknown("This might not be thread-safe"));
                 };
-                if default_safety.not_bounds_affecting && is_likely_bounds_affecting(&selector) {
+                if default_safety.not_bounds_affecting
+                    && is_likely_bounds_affecting(&selector)
+                    && ty.can_affect_bounds()
+                {
                     safety = safety.merge(SafetyProperty::new_unknown(
                         "This might not be bounds-checked",
                     ));

crates/header-translator/src/name_translation.rs

@@ -459,6 +459,10 @@ pub(crate) fn is_likely_bounds_affecting(name: &str) -> bool {
         || name.contains("range")
         || name.contains("offset")
         || name.contains("count")
+        || name.contains("stride")
+        || name.contains("size")
+    // Probably not necessary?
+    // || name.contains("length")
 }
 
 fn lowercase_words(s: &str) -> impl Iterator<Item = String> + '_ {

crates/header-translator/src/rust_type.rs

@@ -1138,6 +1138,72 @@ impl PointeeTy {
                 {
                     TypeSafety::unknown_in_argument("should be of the correct type")
                 }
+                // Passing `MTLFunction` is spiritually similar to passing an
+                // `unsafe` function pointer; we can't know without inspecting
+                // the function (or it's documentation) whether it has special
+                // safety requirements. Example:
+                //
+                // ```metal
+                // constant float data[5] = { 1.0, 2.0, 3.0, 4.0, 5.0 };
+                //
+                // // Safety: Must not be called with an index < 5.
+                // kernel void add_static(
+                //     device const float* input,
+                //     device float* result,
+                //     uint index [[thread_position_in_grid]]
+                // ) {
+                //     if (5 <= index) {
+                //         // For illustration purposes.
+                //         __builtin_unreachable();
+                //     }
+                //     result[index] = input[index] + data[index];
+                // }
+                // ```
+                [(protocol, _)]
+                    if protocol.is_subprotocol_of("MTLFunction")
+                        || protocol.is_subprotocol_of("MTLFunctionHandle") =>
+                {
+                    TypeSafety::unknown_in_argument("must be safe to call").merge(
+                        TypeSafety::unknown_in_argument(
+                            "must have the correct argument and return types",
+                        ),
+                    )
+                }
+                // Access to the contents of a resource has to be manually
+                // synchronized using things like `didModifyRange:` (CPU side)
+                // or `synchronizeResource:`, `useResource:usage:` and
+                // `MTLFence` (GPU side).
+                [(protocol, _)] if protocol.is_subprotocol_of("MTLResource") => {
+                    let safety = TypeSafety::unknown_in_argument("may need to be synchronized");
+
+                    // Additionally, resources in a command buffer must be
+                    // kept alive by the application for as long as they're
+                    // used. If this is not done, it is possible to encounter
+                    // use-after-frees with:
+                    // - `MTLCommandBufferDescriptor::setRetainedReferences(false)`.
+                    // - `MTLCommandQueue::commandBufferWithUnretainedReferences()`.
+                    // - All `MTL4CommandBuffer`s.
+                    let safety = safety.merge(TypeSafety::unknown_in_argument(
+                        "may be unretained, you must ensure it is kept alive while in use",
+                    ));
+
+                    // TODO: Should we also document the requirement for
+                    // resources to be properly bound? What exactly are the
+                    // requirements though, and when does Metal automatically
+                    // bind resources?
+
+                    // `MTLBuffer` is effectively a `Box<[u8]>` stored on the
+                    // GPU (and depending on the storage mode, optionally also
+                    // on the CPU). Type-safety of the contents is left
+                    // completely up to the user.
+                    if protocol.id.name == "MTLBuffer" {
+                        safety.merge(TypeSafety::unknown_in_argument(
+                            "contents should be of the correct type",
+                        ))
+                    } else {
+                        safety
+                    }
+                }
                 // Other `ProtocolObject<dyn MyProtocol>`s are treated as
                 // proper types. (An example here is delegate protocols).
                 [_] => TypeSafety::SAFE,
@@ -3982,6 +4048,43 @@ impl Ty {
         }
     }
 
+    /// Whether the type could in theory affect the bounds of the receiver.
+    ///
+    /// This is meant to catch `NSInteger`, `NSRange`, `MTL4BufferRange`, `MTLGPUAddress` and
+    /// similar constructs.
+    pub(crate) fn can_affect_bounds(&self) -> bool {
+        match self.through_typedef() {
+            Self::Pointer { pointee, .. } | Self::IncompleteArray { pointee, .. } => {
+                pointee.can_affect_bounds()
+            }
+            Self::Array { element_type, .. } => element_type.can_affect_bounds(),
+            Self::Primitive(prim) | Self::Simd { ty: prim, .. } => matches!(
+                prim,
+                // 32-bit and 64-bit integers.
+                Primitive::I32
+                    | Primitive::I64
+                    | Primitive::Int
+                    | Primitive::Long
+                    | Primitive::ISize
+                    | Primitive::NSInteger
+                    | Primitive::U32
+                    | Primitive::U64
+                    | Primitive::UInt
+                    | Primitive::ULong
+                    | Primitive::USize
+                    | Primitive::NSUInteger
+                    | Primitive::PtrDiff
+            ),
+            Self::Struct { fields, .. } | Self::Union { fields, .. } => {
+                fields.iter().any(|field| field.can_affect_bounds())
+            }
+            // Enumerations are intentionally not bounds-affecting (e.g. not
+            // `MTLIndexType`).
+            Self::Pointee(_) | Self::Enum { .. } | Self::Sel { .. } => false,
+            Self::TypeDef { .. } => unreachable!("using through_typedef"),
+        }
+    }
+
     fn into_pointee(self) -> Option<PointeeTy> {
         match self {
             Self::Pointee(pointee) => Some(pointee),

crates/header-translator/src/stmt.rs

@@ -1864,6 +1864,7 @@ impl Stmt {
                     .fold(SafetyProperty::Safe, |mut safety, (arg_name, arg_ty)| {
                         if default_safety.not_bounds_affecting
                             && is_likely_bounds_affecting(arg_name)
+                            && arg_ty.can_affect_bounds()
                         {
                             any_argument_bounds_affecting = true;
                             safety = safety.merge(SafetyProperty::new_unknown(format!(
@@ -1877,6 +1878,9 @@ impl Stmt {
                 if default_safety.not_bounds_affecting
                     && !any_argument_bounds_affecting
                     && is_likely_bounds_affecting(&c_name)
+                    && arguments
+                        .iter()
+                        .any(|(_, arg_ty)| arg_ty.can_affect_bounds())
                 {
                     safety =
                         safety.merge(SafetyProperty::new_unknown("Might not be bounds-checked"));

crates/objc2/src/topics/FRAMEWORKS_CHANGELOG.md

@@ -16,11 +16,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 * Added `IOKit` "serial" submodule.
 * Marked a bunch of functions safe in:
   - `AppKit` / `objc2-app-kit`.
-  - `CoreGraphics` / `objc2-core-graphics`.
   - `CoreFoundation` / `objc2-core-foundation`.
+  - `CoreGraphics` / `objc2-core-graphics`.
   - `CoreVideo` / `objc2-core-video`.
   - `Foundation` / `objc2-foundation`.
   - `IOKit` / `objc2-io-kit`.
+  - `Metal` / `objc2-metal`.
   - `QuartzCore` / `objc2-quartz-core`.
   - `UIKit` / `objc2-ui-kit`.
   - `UniformTypeIdentifiers` / `objc2-uniform-type-identifiers`.

examples/metal/default_xcode_game/renderer.rs

@@ -167,7 +167,7 @@ fn load_pipeline_state(
 }
 
 fn load_vertex_descriptor() -> Retained<MTLVertexDescriptor> {
-    let vertex_descriptor = unsafe { MTLVertexDescriptor::new() };
+    let vertex_descriptor = MTLVertexDescriptor::new();
 
     unsafe {
         let attributes = vertex_descriptor.attributes();
@@ -201,7 +201,7 @@ fn load_vertex_descriptor() -> Retained<MTLVertexDescriptor> {
 fn load_depth_state(
     device: &ProtocolObject<dyn MTLDevice>,
 ) -> Retained<ProtocolObject<dyn MTLDepthStencilState>> {
-    let depth_state_desc = unsafe { MTLDepthStencilDescriptor::new() };
+    let depth_state_desc = MTLDepthStencilDescriptor::new();
     depth_state_desc.setDepthCompareFunction(MTLCompareFunction::Less);
     depth_state_desc.setDepthWriteEnabled(true);
     device

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

@@ -23,6 +23,58 @@
     not(feature = "MTLDevice"),
     doc = "[`MTLCreateSystemDefaultDevice`]: #needs-MTLDevice-feature"
 )]
+//!
+//! # Safety considerations
+//!
+//! Metal allows running arbitrary code on the GPU. We treat memory safety
+//! issues on the GPU as just as unsafe as that which applies to the CPU. A
+//! few notes on this below.
+//!
+//! ## Shaders
+//!
+//! Shaders are (often) written in an unsafe C-like language.
+//!
+//! Loading them (via `MTLLibrary`, function stitching etc.) is perfectly
+//! safe, it is similar to dynamic linking. The restrictions that e.g.
+//! `libloading::Library::new` labours under do not apply, since there are no
+//! ctors in [the Metal Shading Language][msl-spec] (see section 4.2).
+//!
+//! Similarly, getting individual shaders (`MTLFunction`) is safe, we can
+//! model this as the same as calling `dlsym` (which just returns a pointer).
+//!
+//! _Calling_ functions though, is not safe. Even though they can have their
+//! parameter and return types checked at runtime, they may have additional
+//! restrictions not present in the signature (e.g. `__builtin_unreachable()`
+//! is possible in MSL, so is out-of-bounds accesses). If you view
+//! `MTLFunction` as essentially just an `unsafe fn()` pointer, this should be
+//! apparent.
+//!
+//! [msl-spec]: https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf
+//!
+//! ## Bounds checks
+//!
+//! It is yet unclear whether Metal APIs are bounds-checked on the CPU side or
+//! not, so APIs that take offsets / lengths are often unsafe.
+//!
+//! ## Synchronization
+//!
+//! `MTLResource` subclasses such as `MTLBuffer` and `MTLTexture` require
+//! synchronization between the CPU and the GPU, or between different threads
+//! on the GPU itself, so APIs taking these are often unsafe.
+//!
+//! ## Memory management and lifetimes
+//!
+//! Resources used in `MTL4CommandBuffer`s or command buffers with created
+//! with one of:
+//! - `MTLCommandBufferDescriptor::setRetainedReferences(false)`.
+//! - `MTLCommandQueue::commandBufferWithUnretainedReferences()`.
+//!
+//! Must be kept alive for as long as they're used.
+//!
+//! ## Type safety
+//!
+//! `MTLBuffer` is untyped (in a similar manner as a `[u8]` slice), you must
+//! ensure that any usage of it is done with valid types.
 #![recursion_limit = "256"]
 #![allow(non_snake_case)]
 #![no_std]